🔒 Locked

Code Formatting

Started by zasz Jul 13, 2004 at 9:58 AM 8 replies 700+ views

Original Post

Jul 13, 2004 at 9:58 AM

I'm just writing some functions that require alot of logical operations. Some of the code has started to take up a huge amount of columns and starting a new line looks just as bad. So, I'm wondering how you guys would format the following code:


   if ((pm.x1 == pm.x2) && ((pm.y1 + iMove) == pm.y2)) {
      isValid = true;
   } else if ((pm.x1 == pm.x2) && ((pm.y1 + (2 * iMove)) == pm.y2) && (pm.y1 == 1)) {
      isValid = true;
   } else if (((pm.x1 == pm.x2 + 1)||(pm.x1 == pm.x2 - 1)) && ((pm.y1 + iMove) == pm.y2) 
    && (ppbuBoard[pm.x2][pm.y2].iPlayer == iEnemy)) {
      isValid = true;
   } else {
      isValid = false;
   }

Puzzler183

Jul 13, 2004 at 10:00 AM

I would just put in occasional newlines as required. Break up things at &&'s or ||'s (if they are the highest level in the expression).

zasz

Jul 13, 2004 at 10:04 AM

Could you give an example? I tried that and it looked particulary ugly =P.

acraig

Jul 13, 2004 at 10:04 AM

Without actually reading the code to see if some statements can be removed/optimized here is how I would format it. Each line represents a statement that has to be true for the main if statement to be true.

if ( (pm.x1 == pm.x2) &&      ((pm.y1 + iMove) == pm.y2) ) {      isValid = true;} else if ( (pm.x1 == pm.x2) &&           ((pm.y1 + (2 * iMove)) == pm.y2) &&           (pm.y1 == 1) ) {      isValid = true;} else if ( ((pm.x1 == pm.x2 + 1)||(pm.x1 == pm.x2 - 1)) &&           ((pm.y1 + iMove) == pm.y2)     &&           (ppbuBoard[pm.x2][pm.y2].iPlayer == iEnemy) ) {      isValid = true;} else{    isValid = false;}

RFLAGG

Jul 13, 2004 at 10:09 AM

First, I would add comments explaint what each If checks, because it's all too easy to forget after a month what it does.

Since there's no hope (imo) to really make them readable, I would stick each thing on one line and remove whitespace. e.g.

if((pm.x1==pm.x2)&&((pm.y1+iMove)==pm.y2)){isValid=true;}
else if((pm.x1==pm.x2)&&((pm.y1+(2*iMove))==pm.y2)&&(pm.y1==1)){ isValid = true;}

of course everyone I've met hates that method, but it makes it easier for me.

EDIT: Heh acraig's is alot prettier, I vote for his instead :)

zasz

Jul 13, 2004 at 10:24 AM

I ended up going with agraig's method. Thanks for the help, rating up for all of you =).

David ONeil

Jul 13, 2004 at 10:24 AM

I prefer:

   if ((pm.x1 == pm.x2) && ((pm.y1 + iMove) == pm.y2)) {      isValid = true;      }   else if ((pm.x1 == pm.x2) && ((pm.y1 + (2 * iMove)) == pm.y2) && (pm.y1 == 1)) {      isValid = true;      }   else if (((pm.x1 == pm.x2 + 1)||(pm.x1 == pm.x2 - 1)) && ((pm.y1 + iMove) == pm.y2) &&               (ppbuBoard[pm.x2][pm.y2].iPlayer == iEnemy)) {      isValid = true;      }   else {      isValid = false;      }

DigitalDelusion

Jul 13, 2004 at 10:29 AM

I might have messed this up but why not:

if( pm.x1 == pm.x2){      isValid = ((pm.y1 + iMove) == pm.y2) || ((pm.y1 + (2 * iMove)) == pm.y2) && (pm.y1 == 1);}else if( (pm.x1 == pm.x2 + 1) || (pm.x1 == pm.x2 - 1)){	isValid = ((pm.y1 + iMove) == pm.y2) && (ppbuBoard[pm.x2][pm.y2].iPlayer == iEnemy);}else {		isValid = false;}

with nice comments on what the isValid tests are supposed to check for.

Qw3r7yU10p!

Jul 13, 2004 at 10:30 AM

Give the comparisions good names and move them into seperate functions.

e.g.

if(isAgainstLeftWall(pm) || hasJustRebounded(pm) || isNextToEnemy(ppbuBoard, pm)) {    isValid = true;else    isValid = false;//or evenbool Game::isValid(PM& pm) {    return isAgainstLeftWall(pm) ||           hasJustRebounded(pm) ||           isNextToEnemy(ppbuBoard, pm));}

edit: you're quick DD

DigitalDelusion

Jul 13, 2004 at 10:32 AM

Quote:
Original post by petewood
Give the comparisions good names and move them into seperate functions.

then why not take the full step?

isValid = isAgainstLeftWall(pm) || hasJustRebounded(pm) || isNextToEnemy(ppbuBoard, pm);

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.