Opinions about programming styles

[wl3]

I have written a program as a beginner, which makes extensive use of void functions, and uses a lot of global variables. I'm going to rewrite the program, add a GUI, and try to make the programming style more worthy of showing somebody. I know that most people say global variables should be avoided as much as possible.

The problem with my program is that I use a lot of bool variables which are checked by many functions, so I have a whole bunch of global bools. I also have settings and parameters which need to be know by many functions. Also I have a bunch of gnome clutter variables global so that they can be modified by different functions like motion and click events. And the others are mostly just global out of convenience but could very easily be made local.

What do you think about code like this. Should I go back and try and restructure my code so that I no longer need any global variables? Should I not worry about it too much if some are global when it makes it much more convenient etc.

Also, maybe kind of a nit picky thing, but whats your opinion of this type of format,

 
void
clicked_cb (ClutterClickAction *action,
            ClutterActor       *actor,
            gpointer            user_data)

vs,

void clicked_cb (ClutterClickAction* action, ClutterActor* actor, gpointer  user_data)

I find the first to be annoying because of how much extra space it takes up, unless the function has a lot of arguments and it gets too long. If you were writing code which will be seen by your teachers etc, would you be concerned about following any of these particular format?

[laserlight]

Should I go back and try and restructure my code so that I no longer need any global variables?

Yes. You may or may not find that a select few variables do make sense as global variables, in which case you could also consider if applying the singleton pattern makes sense.

Also, maybe kind of a nit picky thing, but whats your opinion of this type of format,

It is just subjective style; it does not really matter either way, except that scrolling horizontally can be irritating, so keeping the line length to say, under 120 characters may be a good idea. (For example, in Python, the standard convention is a maximum of 79 characters per line, excluding the newline sequence itself.)

[JohnW@Wessex]

My personal preference is to format like this.

void clicked_cb(ClutterClickAction* action,
                ClutterActor*       actor, 
                gpointer            user_data)

Should I go back and try and restructure my code so that I no longer need any global variables?

That would be a worthwhile goal. Lots of global variables can have a huge impact on the readability, maintainability and extendability of an application. As the application grows it becomes harder and harder to add features and maintain bug free code.

[Moschops]

The problem with my program is that I use a lot of bool variables which are checked by many functions, so I have a whole bunch of global bools.

Sounds like they're begging to be put inside a single structure named something like currentState which is then passed by reference where needed.

I find the first to be annoying because of how much extra space it takes up,

Space is free. The compiler charges nothing.

This:

void clicked_cb (ClutterClickAction* action, ClutterActor* actor, gpointer  user_data)

is already harder to read and understand than this:

void clicked_cb(ClutterClickAction* action,
                ClutterActor*       actor, 
                gpointer            user_data)

The second is very clearly a function taking three variables; the types are arranged in an easy list on the left, the names on the right. Easy to read and understand quickly. Then I decide to add comments and this happens:

void clicked_cb(ClutterClickAction* action, // must not be null
                ClutterActor*       actor,  
                gpointer            user_data) //Note that this is a pointer

Compare with this:

void clicked_cb (ClutterClickAction* action /*must not be null*/, ClutterActor* actor , gpointer  user_data /*Note that this is a pointer */ )

This was a function with only three parameters. When you end up with a function taking a dozen, or an optional number of parameters, the single line approach extends hundreds of characters off the right hand side of the screen, forcing the reader to pan right and left, never able to see the whole function prototype at once.

The compiler doesn't care at all; the layout is there to make it easier for humans to read and understand.

[laserlight]

Compare with this:

I think that is a rather contrived counter-example though: in many cases I see that doxygen-style comments are used to document the parameters, in which case having all of them on one line is okay so long as horizontal scrolling is not required.

When you end up with a function taking a dozen

As a matter of design rather than just style, I suggest that your functions take fewer than a dozen parameters.

[Moschops]

is okay so long as horizontal scrolling is not required.

Which matches my comment exactly. If the code carries a guarantee that it will never be read by a human, you can do whatever you like with it. I am yet to encounter any code for which such a guarantee exists, but for all I know you do everyday.

As a matter of design rather than just style, I suggest that your functions take fewer than a dozen parameters.

I suppose one could split such functions into laborious multi-function call behemoths, or jam all the parameters into structures to be broken out again on the other side, but that would add complexity for no good reason.

Sometimes the good design uses lots of parameters, and to insist on some kind of limit is as religious as denying that there is ever a good reason to use goto or any of the other common programming articles of faith. If there is ever a good reason to put the entire function prototype on one very long line, then it should be done. I maintain that as a general rule of thumb, making the code readable is more important than putting it all on one line.

[JohnW@Wessex]

... is as religious as denying that there is ever a good reason to use goto

Sorry, I can't resist
I used goto once in C/C++ 25 years ago, and even then it wasn't necessary.

(Lights the blue touch paper and stands back)

[superbonzo]

Sorry, I can't resist
I used goto once in C/C++ 25 years ago, and even then it wasn't necessary.

(Lights the blue touch paper and stands back)

... ah you, irresponsible provocateur !

would you mind posting your goto use case ? you know, it's important to share your story of traumatic events with others, just to let other people know they’re not alone ...

serioulsy speaking,

[...]or jam all the parameters into structures to be broken out again on the other side, but that would add complexity for no good reason.

there's no need to jam and break anything; in C++, you can set up abstractions to reduce the number of arguments and the overall complexity; in this sense, functions of "dozen" paremeters may be interpreted as a symptom of poor abstraction and hence bad design.

[JohnW@Wessex]

would you mind posting your goto use case ? you know, it's important to share your story of traumatic events with others, just to let other people know they’re not alone ...

It's a period of my life that I don't like to talk about

[laserlight]

Which matches my comment exactly.

No, your comment is "use this style because of X". My suggestion is "consider X, then use an appropriate style". "X" here is readability, in particular avoiding having readers of your code to scroll horizontally (though you also add the idea of easy identification of the number and types of the parameters, but I find that debatable for a small number of parameters such as in your example).

Sometimes the good design uses lots of parameters

I have yet to encounter a case where good design requires the use of lots of parameters, though I will concede that sometimes having lots of parameters can be acceptable. In any case, I note that I did not "insist on some kind of limit"

I maintain that as a general rule of thumb, making the code readable is more important than putting it all on one line.

Indeed, making the code readable is more important than remaining consistent with a particular rule of style.

there's no need to jam and break anything; in C++, you can set up abstractions to reduce the number of arguments and the overall complexity; in this sense, functions of "dozen" paremeters may be interpreted as a symptom of poor abstraction and hence bad design.

That is precisely what I had in mind.

[wl3]

It's a period of my life that I don't like to talk about

When I first began to program, when I had only learned what if, while, goto, and for do, not having known what even a function was; I wrote a five thousand line program which included a hang man game, an audio sampler, a quiz, a bunch of math stuff, and a bunch of toy applications, without using a single function, and probably about 50 goto's. At least I had no global variables.

[Lindley]

When I first began to program, when I had only learned what if, while, goto, and for do, not having known what even a function was; I wrote a five thousand line program which included a hang man game, an audio sampler, a quiz, a bunch of math stuff, and a bunch of toy applications, without using a single function, and probably about 50 goto's. At least I had no global variables.

If you ever find that code again, good luck understanding it!

[wl3]

If you ever find that code again, good luck understanding it!

I accidentally used auto-replace or something, and lost the working version of the source code for the final version. I'll post a section of it.

...

if (guess[0] == slw[6] && f[7]==0){w--;}
if (guess[0] == slw[6]) {f[7]=0; 
cout << slw[6] << " ";
w=w+1;
//sndPlaySound(SND, SND_ASYNC);
}
else if (f[7] == 0) {cout << slw[6] << " ";}
else cout << "_ ";

if (guess[0]=='a') ch[0]='a';if (guess[0]=='b') ch[1]='b';if (guess[0]=='c') ch[2]='c';if (guess[0]=='d') ch[3]='d';
if (guess[0]=='e') ch[4]='e';if (guess[0]=='f') ch[5]='f';if (guess[0]=='g') ch[6]='g';if (guess[0]=='h') ch[7]='h';
if (guess[0]=='i') ch[8]='i';if (guess[0]=='j') ch[9]='j';if (guess[0]=='k') ch[10]='k';if (guess[0]=='l') ch[11]='l';
if (guess[0]=='m') ch[12]='m';if (guess[0]=='n') ch[13]='n';if (guess[0]=='o') ch[14]='o';if (guess[0]=='p') ch[15]='p';
if (guess[0]=='q') ch[16]='q';if (guess[0]=='r') ch[17]='r';if (guess[0]=='s') ch[18]='s';if (guess[0]=='t') ch[19]='t';
if (guess[0]=='u') ch[20]='u';if (guess[0]=='v') ch[21]='v';if (guess[0]=='w') ch[22]='w';if (guess[0]=='x') ch[23]='x';
if (guess[0]=='y') ch[24]='y';if (guess[0]=='z') ch[25]='z';

int cp=0;
cout << "                   ";
while (cp<26) {cout << ch[cp];
cp++;}
cout << endl << endl;

end:
if (w < 7 ) goto tryagain;
if (w >= 7) {
  //char*SND="C:\\tc++\\ding.WAV";
  //sndPlaySound(SND, SND_ASYNC);
  cout << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl << endl;
  SetConsoleTextAttribute(colors4,  FOREGROUND_BLUE|BACKGROUND_BLUE |BACKGROUND_RED | BACKGROUND_RED | BACKGROUND_GREEN); 
  cout <<  " you win "; 
  SetConsoleTextAttribute(colors4, BACKGROUND_INTENSITY |BACKGROUND_BLUE |BACKGROUND_RED | BACKGROUND_RED | BACKGROUND_GREEN);
if (strikes==1) score=score+0;
if (strikes==2) score=score+1500;
if (strikes==3) score=score+900;
if (strikes==4) score=score+300;
if (strikes==5) score=score+150;
if (strikes==6) score=score+85;
if (strikes==7) score=score+70;
if (strikes==8) score=score+45;
if (strikes==9) score=score+35;
if (strikes==10) score=score+30;
if (strikes==11) score=score+25;
if (strikes==12) score=score+20;
if (strikes==13) score=score+10;
if (strikes==14) score=score+5;
if (strikes==15) score=score+2;

if (times>9) goto bottom;
lose:
times++;

if (times<10) {
	SetConsoleTextAttribute(colors3, FOREGROUND_BLUE | BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
	cout << " Your score so far is:" << score;
	cout << endl << endl; 
	SetConsoleTextAttribute(colors3, FOREGROUND_GREEN);
	cout << " round:"; 
	SetConsoleTextAttribute(colors3, BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
	cout << " ";
	cout << times+1 << endl << endl;
	count++;
	goto top;}
  bottom:
SetConsoleTextAttribute(colors3, BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
  cout << endl << "                                FINAL SCORE "; 
  SetConsoleTextAttribute(colors4,FOREGROUND_GREEN |  BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
  cout << score << endl;//word guess high score tracker
  SetConsoleTextAttribute(colors4, BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
char name[22]={' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '};
  char c, acorn[25];
  char d;
  long int r;

  ifstream highscore;
  highscore.open("C:\\tc++\\highscore.txt", ios::in);  
  highscore.get(acorn,15);   // converting character stream into long integer;
  r=atol(acorn);


  if (score>r){   
  highscore.close();
  SetConsoleTextAttribute(colors3, BACKGROUND_RED | BACKGROUND_RED | BACKGROUND_GREEN);
  cout << endl << " NEW HIGH SCORE!";
  SetConsoleTextAttribute(colors4, BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
  cout << endl << endl << " name:";
  SetConsoleTextAttribute(colors3, BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
  cin >> name;

  ofstream highscoreout;
  highscoreout.open ("c:\\tc++\\highscore.txt");
  highscoreout << score;
  highscoreout.close();

  ofstream hsname;
  hsname.open ("c:\\tc++\\hsname.txt");
  hsname << name[0] << name[1] << name[2] << name[3] << name[4] << name[5] << name[6] << name[7] << name[8] << name[9] << name[10] << name[11] << name[12] << name[13] << name[14] << name[15] << name[16] << name[17] << name[18]
   << name[19] << name[20] << name[21];
  hsname.close();}

  
  highscore.close();
  SetConsoleTextAttribute(colors3, FOREGROUND_BLUE | BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
  cout << endl << " HIGH SCORE " << endl << endl;
  SetConsoleTextAttribute(colors4, FOREGROUND_GREEN | BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
  cout << " ";
  ifstream highscorein;
  highscorein.open("c:\\tc++\\highscore.txt");
  highscorein.get(c);                            
  if (!highscorein.good()) cout << " cannot find your file"; 
  while(!highscorein.eof()) {             
  cout << c;
  highscorein.get(c);}
  highscorein.close();

  SetConsoleTextAttribute(colors4, BACKGROUND_RED | BACKGROUND_BLUE | BACKGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_GREEN);
  
  cout << ":";

  ifstream hsname;
  hsname.open("C:\\tc++\\hsname.txt", ios::in);  
  hsname.get(d);                            
  if (!hsname.good()) cout << " cannot find your file"; 
  while(!hsname.eof()) {             
  cout << d;
  hsname.get(d);}
  hsname.close();

  cout <<  endl << endl;
  
 
  cout << endl << endl << "                      new game [g]   menu [m]    exit [x]" << endl << endl;


guess[0]=_getch();
if (guess[0]=='x') return 0;
if (guess[0]=='m') continue;
if (guess[0]=='g') goto start;
}}}}
...

[monarch_dodra]

The only legitimate use I have seen (and used) for goto is the "double loop" or "2D loop" break. While there are ways to avoid it by using functions and what not, sometimes it is just more convenient, easier to read, and cleaner (yes cleaner) to use the goto (IMO). Never trust someone that says "never do this".

For the original question, my only pet peeve is that whatever you do, always use correct indentation. There is NOTHING I hate more (with fiery passion), than having each argument on a separate line, yet each on a different indentation. Or just bad indentation altogether.

[Lindley]

For the original question, my only pet peeve is that whatever you do, always use correct indentation. There is NOTHING I hate more (with fiery passion), and having each argument on a separate line, yet each has a different indentation. Or just bad indentation altogether.

I'll add to this: don't mix spaces and tabs. Pick one and stick with it. Generally, spaces are a better choice because they allow more customization of indent levels.