Simplifying my mess :S

[fuerchter]

Hi guys
I have a class called board in which I wrote a really messy function printBoard and another one called printBDrop which do almost the same thing. I can't really find a way of simplifying them so I thought I'd ask here.

Not only does it annoy me that I have two functions that vary only in detail but the function itself is way too long and hard too read.

Take a look:
http://pastebin.com/iA6b1JMw

What can I do to clean this up? :S

//EDIT:
lol, the code i actually too long for the board ^^ (I've copied it to pastebin now)

[soranz]

Wow else ifs lol

There's a couple functions that u'r calling often and yes they are long. Since u call this one many times:
(double)(myTiles[xCount][yCount].myBuilding.health)/(double)(myTiles[xCount][yCount].myBuilding.maxHealth)
if I'm not mistaken u could call it just once on line 27 and then store that value into something much smaller just as a descriptive placeholder for readability...

The other thing in //actual board is that it looks like all those ifs and else ifs are identical except for the player # that u'r testing. So why not just loop the player # too ?

for(int xCount=0; xCount<xSize; xCount++)
         {
//colors the current tile
          if(myTiles[xCount][yCount].player==0)
          {
                SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x07);
           }
           else
           {
                 for(int j=1; j<maxplayer; j++)
                 {

                       if((myTiles[xCount][yCount].player==j &&  myTiles[xCount][yCount].myType==tile::abuilding)
                       {
                             //etc up to........
                             else if((double)(myTiles[xCount][yCount].myBuilding.health)/(double)(myTiles[xCount][yCount].myBuilding.maxHealth)>0)
                             {
                                 SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x64);
                             }
                       }
                 }

That'll cut down 12 else ifs to 4...

[fuerchter]

1. are else ifs bad? ^^

2. going to change that up with the placeholder thx ;)

3. What I do after the //actual board comment is I check for the players and whether the current tile is a building

then in the nested if i check how much health a building has left.

so you can basically think of it this way:
player1: yellow background color, health>66%: green foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x62);
then:
player1: yellow background color, health>33%: yellow foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x6E);
then:
player1: yellow background color, health>0%: red foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x64);

and so on with the different players

//EDIT:
also I'm thinking about merging the two functions by putting the printBDrop specific part (from line 167 on) into printBoard and making it accessible via a bool parameter? (does that make sense?)

//EDIT2:
and by the way how do people solve saving each version of their code (e.g. making a backup before an important change?). should i just copy the solution's folder or is there a more comfortable way of doing that?

[soranz]

1. yes when a simple loop can clean them up. less code is almost always better than more code that does the same thing...

2. cool !

3. so each of those players need to be checked against those values ? then, u should try to loop it like i illustrated...

for(int j=1; j<maxplayer; j++)
                 {

                       if((myTiles[xCount][yCount].player==j &&  myTiles[xCount][yCount].myType==tile::abuilding)
                       {

//SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x62);

//SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x6E);
                             else if((double)(myTiles[xCount][yCount].myBuilding.health)/(double)(myTiles[xCount][yCount].myBuilding.maxHealth)>0)
                             {
                                 SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x64);
                             }
                       }
                 }

Yes, it looks like the 2 functions should be 1. They both seem to basically have the same flow so unless they need to be called separately they should probably be together. That'll save a good 100 lines right there + make ur code run faster. I'm not sure what u mean by a bool parameter?

i just cycle a few flash drives for backup and backup of backup. nothing worse than losing all that hard work on a single harddisk :/

[fuerchter]

about 1. again i thought you meant by that that writing

if
{

}
else
{
if
{
}
}

for example is better style than

if
{

}
else if
{
}

I've now compressed both functions into one, made the player detection a loop, introduced vectors/iterators for the color selection and created a new variable (percHealth) to keep health/maxHealth.

http://pastebin.com/kEJUKKJa

you can see in the switch statement what I meant by the bool parameter. the bool parameter controls whether the feature of the (initially) copied function is executed.

the part after bDrop is true still doesn't appeal too much to me though, have to change that up as well (also in my main cpp file i execute almost the very same if statements).

I have some ideas to fix the remaining stuff so maybe i'm getting along alone but i'll post again if anything is too hard to solve for me :S

[ne555]

I wrote a really messy function printBoard and another one called printBDrop which do almost the same thing. I can't really find a way of simplifying them so I thought I'd ask here.

Comment their purpose. [edit] so it's easier for us to understand them [/edit]

player1: yellow background color, health>66%: green foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x62);

Avoid magic numbers. 0x62 = fGREEN | bYELLOW
You could also modularize the colour choice.

If you care about the percentage instead of the actual health, you could define a method for that.

and by the way how do people solve saving each version of their code

I use git

Edit:

and so on with the different players

¿how does it change with another player?

[fuerchter]

"how does it change with another player"

if i had to continue this:

player1: yellow background color, health>66%: green foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x62);
then:
player1: yellow background color, health>33%: yellow foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x6E);
then:
player1: yellow background color, health>0%: red foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x64);

player2: white background color, health>66%: green foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x72);
then:
player1: white background color, health>33%: yellow foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x7E);
then:
player1: white background color, health>0%: red foreground
SetConsoleTextAttribute( GetStdHandle( STD_OUTPUT_HANDLE ), 0x74);

but with the new version i changed this system anyways so there is not much to talk about anymore concerning that issue