C++

Forum

 
Could you improve my code before it's too late?

closed account (G30oGNh0)
I thought before I continue any further with this I'd like some opinions on how to change this to become more readable/maintainable and the most efficient way possible. I don't want to keep improving it if I am going the wrong way about it. I also feel the code is over encumbered and weighty.

Some area's I would much appreciate help with:

Should buildings be in their own 'Buildings' class? 

1
2
3
4
5
 
Building Barracks;
Barracks.getUpgradeReq();
Barracks.build();
Barracks.upgradeToNext();
Barracks.buildUnit(int unitID);


Can the build queue be better? Rather than using a building ID and int's does C++ have something better to use?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
 
//displays and handles HQ
void Nation::openHQ(void)
{
	bool inHQ = true;

	while(inHQ)
	{
	cls();
	int temp;
	cout << "~~~~~~~~~~~~~~~VILLAGE HEADQUARTERS~~~~~~~~~~~~~~~" << endl;
	cout << "\nBUILDING PROGRESS\tTurns till completion\n\n\n";

	switch(buildingID) //Display the building that is in the build queue
	{
	case nobuilding:
		cout << "No buildings currently in production.";
		break;
	case HQ:
		cout << "Headquarters: " << hq + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case BARRACKS:
		cout << "Barracks: " << barracks + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case STABLES:
		cout << "Stables: " << stables + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case TIMBER:
		cout << "Timber Camp: " << timbercamp + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case CLAYP:
		cout << "Clay Pit: " << claypit + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case IRONM:
		cout << "Iron Mine: " << ironmine + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case SMITHY:
		cout << "Smithy: " << smithy + 1 << "\t\t" << buildingTurnsLeft;
		break;
	}

	//Display the buildings you can upgrade

	cout << "\n\n\n\nBUILDING(Current lvl)\tWOOD\tCLAY\tIRON\tTurns to complete";
	cout << "\n\n\n1) Headquarters(" << hq << ")\t" << hqcost[hq][WOOD] << "\t" << hqcost[hq][CLAY] << "\t" << hqcost[hq][IRON] << "\t" << hqcost[hq][TTC] << endl;
	cout << "2) Timber Camp(" << timbercamp << ")\t" << resourcecampcost[timbercamp][WOOD] << "\t" << resourcecampcost[timbercamp][CLAY] << "\t" << resourcecampcost[timbercamp][IRON] << "\t" << resourcecampcost[timbercamp][TTC] << endl;
	cout << "3) Clay Pit(" << claypit << ")\t\t" << resourcecampcost[claypit][WOOD] << "\t" << resourcecampcost[claypit][CLAY] << "\t" << resourcecampcost[claypit][IRON] << "\t" << resourcecampcost[claypit][TTC] << endl;
	cout << "4) Iron Mine(" << ironmine << ")\t\t" << resourcecampcost[ironmine][WOOD] << "\t" << resourcecampcost[ironmine][CLAY] << "\t" << resourcecampcost[timbercamp][IRON] << "\t" << resourcecampcost[ironmine][TTC] << endl;
	
	if(hq >= 2)
	{
		cout << "5) Barracks(" << barracks << ")\t\t" << "0" << "\t" << "0" << "\t" << "0" << "\t" << "0" << endl;
	}
	if(hq >= 5)
	{
		cout << "6) Smithy(" << smithy << ")\t\t" << "0" << "\t" << "0" << "\t" << "0" << "\t" << "0" << endl;
	}
	if(hq >= 10)
	{
		cout << "7) Stables(" << stables << ")\t\t" << "0" << "\t" << "0" << "\t" << "0" << "\t" << "0" << endl;
	}
	
	cout << "8) Cancel build queue" << endl;
	cout << "9) Leave Headquarters" << endl;
	cout << "\nChoice: ";
	cin >> temp;

	switch(temp)
	{
	case 1: //Upgrade HQ
		if(buildingID == nobuilding) // Can only upgrade if no building is in the build queue and you have the resources required
		{
			if(wood >= hqcost[hq][WOOD] && clay >= hqcost[hq][CLAY] && iron >= hqcost[hq][IRON])
			{
				buildingID = HQ; //Put building ID in building queue
				buildingTurnsLeft = hqcost[hq][TTC]; //Set the turn time limit

				wood -= hqcost[hq][WOOD]; //Take away the cost of resources
				clay -= hqcost[hq][CLAY];
				iron -= hqcost[hq][IRON];
			}
			else
			{
				cout << "\n\nYou do not have enough resources.";
				cin.get();
				cin.ignore();
			}
		}
		else
		{
			cout << "\n\nThe production queue is busy.";
			cin.get();
			cin.ignore();
		}
		break;
	case 2: //Upgrade Timber Camp;
		if(buildingID == nobuilding)
		{
			if(wood >= resourcecampcost[timbercamp][WOOD] && clay >= resourcecampcost[timbercamp][CLAY] && iron >= resourcecampcost[timbercamp][IRON])
			{
				buildingID = TIMBER;
				buildingTurnsLeft = resourcecampcost[timbercamp][TTC];

				wood -= resourcecampcost[timbercamp][WOOD];
				clay -= resourcecampcost[timbercamp][CLAY];
				iron -= resourcecampcost[timbercamp][IRON];
			}
			else
			{
				cout << "\n\nYou do not have enough resources.";
				cin.get();
				cin.ignore();
			}
		}
		else
		{
			cout << "\n\nThe production queue is busy.";
			cin.get();
			cin.ignore();
		}
		break;


Thanks in advance.
LB (13399)
I GuNNeR I wrote:
and the most efficient way possible.
In programming, we almost never go for the most efficient way possible. I'd even say we actually never go for the more efficient approach, ever. The only time in programming that you would actually try to make something more efficient is if by profiling your code you find that it is the bottleneck preventing your code from running any faster.

Otherwise, you go for the most elegant solution, even if you know it's not very efficient. Solve the problem first, make it faster later.
Last edited on
closed account (j3Rz8vqX)
Good Questions here. Not quoting or commenting to judge you, rather trying to answer the ideas/questions.

"readable/maintainable" - Yes you can make it more readable with:
recursion - to reduce the number of lines; possibly splitting the function operation into smaller functions. Note that this reduces performance, by using a round about procedure, but will probably make it more maintainable. (Everyone's got a different opinion, this is just mines).

"most efficient way possible" - No one will know that until the complete picture is drawn.

"I don't want to keep improving it if I am going the wrong way about it" - By improvements, I'm assuming you're to later add more buildings? If so, you should consider making things modular. (Again, my opinion here - a heavily debated topic over performance vs flexibility).

"Should buildings be in their own 'Buildings' class?" - Why not one building, with derived building classes? Polymorphism is the way to go.

"Can the build queue be better?" - Not being offensive but we cannot say for certain. If it was me, I would check my if-conditions with external functions - just me. Also primitives aren't flexible, so hopefully: 2, 5, and 10 are significant values. But then again, being primitives they are less robust with faster performance - predefined values would increase performance.

"Rather than using a building ID and int's does C++ have something better to use?" - Non-system reliant types are best for portability, but best worry about that after first achieving the complete picture - completion of building and just concern about it's maintenance.

Algorithms to consider: recursion, polymorphism, and inheritance.

--- My Questions ---

How important is the role of this function? If it's never to be greatly changed again, I would not prioritize its flexible.

How often is the function called? (It's level of impact on the performance of your program).

Will it be required to change greatly in the future? Say you added more buildings. (Modular to simplify things is the best if change is necessary).
Breaking steps down, can never hurt you - unless your suffering in performance.

What ever happens if you decide to add addition logic to requirements? Say, an ability allows you to produce two queues instead of one. (Don't think too much about this one... You if-conditions seem absolute)

Then again, you shouldn't worry so much.

You've still got headroom for "some" change if you ever decide to. Simply overloading the Nation::openHQ(void) function or possibly header files will give you a great amount of flexibility.

What happens if the base function needs change ... don't worry about it.
Production is more important than flexibility.

Too many things to think about, best produce functionality first.

Ah, I've forgotten one question:
"Rather than using a building ID and int's does C++ have something better to use?"

Portability is a huge chapter. Simply say, int's size over different platform changes greatly.

Not the best review, but a good start for.. ah whatever:
http://www.viva64.com/en/a/0004/

By the way, you shouldn't want others to "improve" your code, you may not be able to recognize it afterwards. Whats readable to them may not be readable to you.

Apologies, no productivity here, but I hope this helps you improve your own code.
Last edited on
closed account (G30oGNh0)

"readable/maintainable" - Yes you can make it more readable with:
recursion - to reduce the number of lines; possibly splitting the function operation into smaller functions. Note that this reduces performance, by using a round about procedure, but will probably make it more maintainable. (Everyone's got a different opinion, this is just mines).


Which parts of the above code would you split in to functions? I was thinking maybe:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
 
case 1: //Upgrade HQ
		if(buildingID == nobuilding) // Can only upgrade if no building is in the build queue and you have the resources required
		{
			if(wood >= hqcost[hq][WOOD] && clay >= hqcost[hq][CLAY] && iron >= hqcost[hq][IRON])
			{
				buildingID = HQ; //Put building ID in building queue
				buildingTurnsLeft = hqcost[hq][TTC]; //Set the turn time limit

				wood -= hqcost[hq][WOOD]; //Take away the cost of resources
				clay -= hqcost[hq][CLAY];
				iron -= hqcost[hq][IRON];
			}
			else
			{
				cout << "\n\nYou do not have enough resources.";
				cin.get();
				cin.ignore();
			}
		}
		else
		{
			cout << "\n\nThe production queue is busy.";
			cin.get();
			cin.ignore();
		}
		break;


Instead of being a switch statement it could be a function no? Or maybe even a vector? I haven't used them yet, maybe this is a good opportunity if it proves useful.


"I don't want to keep improving it if I am going the wrong way about it" - By improvements, I'm assuming you're to later add more buildings? If so, you should consider making things modular. (Again, my opinion here - a heavily debated topic over performance vs flexibility).


Improvements in general, I don't want to be patching up an old car with bits of iron, I'd rather just attempt to make a better car. The car here being the handling system of buildings and the upgrading of buildings, adding things to the build queue alongside error checking.

The function itself will be used often enough, if the player actually wants to get anywhere, building and upgrading structures give you a heavy advantage ( Only at Barracks level 10 && Smithy level 5 you are able to create Archers )

Side note - I haven't even got to making the units yet *shudders*.


"Should buildings be in their own 'Buildings' class?" - Why not one building, with derived building classes? Polymorphism is the way to go.


I guess it would look something like this?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
 
class Building
{
protected:
    int buildingLevel;
    void openBuilding();
};

class Barracks : public Building
{
private:
    void buildTroop(int troopID);
};

Barracks b;
b.openBuilding();
b.buildTroop(swordsman); //Enum to hold troop ID's


My response is becoming lengthy so I'll leave it here and ask/answer more questions after this part is resolved.

I really appreciate your help thanks, I am determined to actually finish a C++ project in my lifetime.
closed account (G30oGNh0)

In programming, we almost never go for the most efficient way possible. I'd even say we actually never go for the more efficient approach, ever. The only time in programming that you would actually try to make something more efficient is if by profiling your code you find that it is the bottleneck preventing your code from running any faster.

Otherwise, you go for the most elegant solution, even if you know it's not very efficient. Solve the problem first, make it faster later.


Maybe a bad choosing of word on my part. By efficient I meant efficiently wrote ( breaking it down, reducing code, adding a function etc etc) not necessarily efficiently processed. Sorry for that misinterpretation.
closed account (j3Rz8vqX)
If you had 10 cases of upgrade, that'd be a lot of coding...

To me, each case statement looks somewhat similar, with just a few arguments being different in each.

Functions can reduce the size of code, if you decide to make more building types; good idea if multiple lines of code share nearly identical functionality.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
 
switch(temp)
	{
	case 1: //Upgrade HQ
            void buildQueue(/*building_id*/,/*nobuilding*/,/*buildingTurnsLeft*/,/*vector*/,/*HQ*/./*iron*/./*wood*/./*clay*/)

	case 2: //Upgrade Timber Camp;
            void buildQueue(/*building_id*/,/*nobuilding*/,/*buildingTurnsLeft*/,/*vector*/,/*Timber_Camp*/./*iron*/./*wood*/./*clay*/)
        case 3:
            ...
        case 4::
            ...
        ...
        ...
        //Enabling many more cases with a maintainable number of lines.
            //Functions are a bit slower than hard-code
                //Function can cause heap allocation if using recursion


Hiding code doesn't improve performance and makes code actually harder to interpret/predict, but can shorten code dramatically - and sometimes improve functionality.
1
2
3
4
5
6
7
8
9
10
11
12
 

	//if(hq >= 2)
	//{
		cout << /*vector_string[2]*/ << /*vector_building[2]*/ << ")\t\t" << "0" << "\t" << "0" << "\t" << "0" << "\t" << "0" << endl;
	//}

        //requires no if condition, just prints what's in the string_vector at index 2 and vector_building at index 2.
        //example: cout << /*vector_string[5]*/ << /*vector_building[5]*/ << ")\t\t" << "0" << "\t" << "0" << "\t" << "0" << "\t" << "0" << endl;
        //example: cout << /*vector_string[10]*/ << /*vector_building[10]*/ << ")\t\t" << "0" << "\t" << "0" << "\t" << "0" << "\t" << "0" << endl;

        //Again, functions aren't as efficient as direct hard-code, but it can reduce code length. Also, in this case, it nullify the use of testing against multiple conditions.
        //If you had 10 cases here, it'd take 10 comparison before being able to check the last one, just to print.


The above reasoning can be applied to the switch below as well:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
 


	switch(buildingID) //Display the building that is in the build queue
	{
	case nobuilding:
		cout << "No buildings currently in production.";
		break;
	case HQ:
		cout << "Headquarters: " << hq + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case BARRACKS:
		cout << "Barracks: " << barracks + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case STABLES:
		cout << "Stables: " << stables + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case TIMBER:
		cout << "Timber Camp: " << timbercamp + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case CLAYP:
		cout << "Clay Pit: " << claypit + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case IRONM:
		cout << "Iron Mine: " << ironmine + 1 << "\t\t" << buildingTurnsLeft;
		break;
	case SMITHY:
		cout << "Smithy: " << smithy + 1 << "\t\t" << buildingTurnsLeft;
		break;
	}


So, your overall code may look something like this:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
 
//displays and handles HQ
void Nation::openHQ(void)
{
	bool inHQ = true;

	while(inHQ)
	{
	cls();
	int temp;
	cout << "~~~~~~~~~~~~~~~VILLAGE HEADQUARTERS~~~~~~~~~~~~~~~" << endl;
	cout << "\nBUILDING PROGRESS\tTurns till completion\n\n\n";

	if (buildingID != nobuilding)
        {
            cout << /*vector_string_buildingName[buildingID]*/ << /*building_count?*/ + 1 << "\t\t" << buildingTurnsLeft;
        }
        else
        {
            cout << "No buildings currently in production.";
        }
	//Display the buildings you can upgrade

	cout << "\n\n\n\nBUILDING(Current lvl)\tWOOD\tCLAY\tIRON\tTurns to complete";
	cout << "\n\n\n1) Headquarters(" << hq << ")\t" << hqcost[hq][WOOD] << "\t" << hqcost[hq][CLAY] << "\t" << hqcost[hq][IRON] << "\t" << hqcost[hq][TTC] << endl;
	cout << "2) Timber Camp(" << timbercamp << ")\t" << resourcecampcost[timbercamp][WOOD] << "\t" << resourcecampcost[timbercamp][CLAY] << "\t" << resourcecampcost[timbercamp][IRON] << "\t" << resourcecampcost[timbercamp][TTC] << endl;
	cout << "3) Clay Pit(" << claypit << ")\t\t" << resourcecampcost[claypit][WOOD] << "\t" << resourcecampcost[claypit][CLAY] << "\t" << resourcecampcost[claypit][IRON] << "\t" << resourcecampcost[claypit][TTC] << endl;
	cout << "4) Iron Mine(" << ironmine << ")\t\t" << resourcecampcost[ironmine][WOOD] << "\t" << resourcecampcost[ironmine][CLAY] << "\t" << resourcecampcost[timbercamp][IRON] << "\t" << resourcecampcost[ironmine][TTC] << endl;
	
        cout << /*vector_string[2]*/ << /*vector_building[2]*/ << ")\t\t" << "0" << "\t" << "0" << "\t" << "0" << "\t" << "0" << endl;
	
        //5,6,7 - will sometimes not print out, and still print the below - original design
	cout << "8) Cancel build queue" << endl;
	cout << "9) Leave Headquarters" << endl;
	cout << "\nChoice: ";
	cin >> temp;

        switch(temp)
	{
	    case 1: //Upgrade HQ
                void buildQueue(/*building_id*/,/*nobuilding*/,/*buildingTurnsLeft*/,/*vector*/,/*HQ*/./*iron*/./*wood*/./*clay*/) //You should know what build_queue does.

	    case 2: //Upgrade Timber Camp;
                void buildQueue(/*building_id*/,/*nobuilding*/,/*buildingTurnsLeft*/,/*vector*/,/*Timber_Camp*/./*iron*/./*wood*/./*clay*/) //You should know what build_queue does.
            //case 3:
                //...
            //case 4::
                //...
            //...
            //...


As programmers, we're always looking for patterns. Encapsulating data into an object organizes it and gives us the possibility of morphing it. Our generic would be the parent class which has the standard data and functionality.....

<Edited, removed essay>

A possible class below:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
 
    //class building
        //protected:
        //string building name
        //int building id
        //bool building busy?

        //public:
        //standard building member functions

    //class barrack derived(child) from building
        //protected:
        //barrack unique data
  
        //public:
        //barrack unique member functions

I'm not trying to change how you program in any way, just giving my perspective on possible improvements.

Code was done through the browsers text editor.

And as you mentioned:
"I am determined to actually finish a C++ project in my lifetime"

Learning something new is good, but it can be overwhelming and cause frustration. You should work with what you know, and finish your project. Maintenance and redesign can be implemented later, though this should've been thought through before the actual creation of the program. =D

Good Luck
Last edited on
closed account (G30oGNh0)
Dput I cannot thank you enough for this response, it has made a lot of sense and I cannot wait to start implementing your idea's!

The time you have put into this is invaluable to me. I plan to re-write most of the core engine now, and it's also opened my mind to a few idea's.

Please keep an eye on your inbox, I expect I'll need to pick your brain in a couple of days time :)
Topic archived. No new replies allowed.