Coding style question

[KimB]

Those snippets are ancient. They are off the MSDN CD that came with Visual C 6!

Well, that's good to know, at least

And the header they're including there is certainly not the C++/STL string class, but the C string processing stuff.

Well, I knew that much

But I do have to comment on one thing: those variables should not be defined as global (in the first example). That's just bad programming practice. I make it a point that the only global variables I ever define are constants.

 

[ERP]

Well, that's good to know, at least


Well, I knew that much

But I do have to comment on one thing: those variables should not be defined as global (in the first example). That's just bad programming practice. I make it a point that the only global variables I ever define are constants.

Nothing wrong with globals, in some cases they're basically unavoidable. And like goto's they are nor inherently evil, you just need to use common sense as to where you use them.

I've seen a lot of code that goes to a great deal of trouble to define registration mechanisms that replace globals. But semantically the registered items are still global, so why not just admit it and declare them. Now construction of interdependant globals is a different issue and regstration mechanisms can provide value in this case.

 

[aaaaa00]

Here's an example from Wikipedia: a_crszkvc30LastNameCol.

a_crszkvc30LastNameCol is just bad Hungarian.

Ironically that's exactly the reason I avoid Hungarian notation. Something like "fileName" is much more clearly a string holding the name of a file than "szFile". The latter could be a sentence in the file or something. Dunno really, it doesn't tell me anything useful. And "szFileName" would really start me doubting whether it's actually a string and make me check the declaration.

I disagree.

fileName alone doesn't convey a lot of information.

Is this a nullterminated string or is it a counted string?
Is it a Unicode string or ANSI?
Is it a pointer to a string or an array of chars?
Do I need to free it after I'm done with it?

pwszFileName tells me a lot of information.

p tells me this is a pointer, not an array of chars. So likely it's not a fixed length array and was probably malloc'd somewhere.
p tells me I need to think about where I got it from and think about if I need to free it.
w tells me this is a Unicode string. This means I should not mix it with ANSI functions.
sz tells me this is a string, and that it is null terminated. This means it's safe to use C runtime functions with it and I don't need to look somewhere else for the length of the string.

When I was first introduced to Hungarian, I absolutely hated it. I was annoyed at how it forced me to pay attention to the types of things, and to rename variables when they changed.

But after getting some experience developing, I've changed my mind. It really does help once you get into the habit of prefixing the variables.

Yes, it requires discipline to make it work. You must enforce this across all your team members. You must choose a small, concise, and simple set of prefixes and strictly enforce the use of it for EVERYONE on the team.

In my experience, when properly used Hungarian notation is a significant aid in dealing with large code bases written in languages like C or C++. It's less useful for managed code or higher level languages.

 

[Nom De Guerre]

I'm just curious. It seems like the overwhelming majority of code that I look at uses the following convention:

function foo(float x) {
  if (x = 1) {
    //do stuff
  }
  else {
    //do other stuff
  }
}

But I've always thought that the following arrangement of the brackets was vastly more readable:

function foo(float x)
{
  if (x = 1)
  {
    //do stuff
  }
  else
  {
    //do other stuff
  }
}

So, the question is, if anybody here prefers the above format, is there any specific reason?

I'm sure a number have given their reasons (I haven't read all of this thread) but personally I used to do the second, but I converted to the first many years ago. You get more code on a given screen size with minimal impact on readability, for a nett win in most people's code comprehension ability.

 

[K.I.L.E.R]

I want to show off the manliness of my code.
I just broke a piece of my new computer graphics assignment.
Anyway 'rate my code', so to speak.

bool VideoInitialiser::changeRes(bool isFullscreen, int refresh, int bpp, int width, int height){
    bool isSuccessful = false;
    Uint32 flags = (isFullscreen == true) ? SDL_OPENGL | SDL_FULLSCREEN : SDL_OPENGL;

    this->isFullscreen = isFullscreen;
    this->refreshRate = refresh;
    this->bpp = bpp;
    this->width = width;
    this->height = height;
    
    if(SDL_VideoModeOK(width, height, bpp, flags) != 0){
        surface = SDL_SetVideoMode(width, height, bpp, flags);
        isSuccessful = true;
    }

    return isSuccessful;
}

 

[Humus]

For style it's OK. I'd put a space after the if though.

Comparing a boolean to true is pretty pointless though, and just makes the code needlessly verbose. And since you're not modifying the arguments, it's best to declare them as const.

 

[K.I.L.E.R]

Thanks.
I changed it to:

const Uint32 flags = (isFullscreen) ? (SDL_OPENGL | SDL_FULLSCREEN) : (SDL_OPENGL);

I like to keep the condition visible either by '==' and '()' or just using '()'.
It's a readability thing.

For style it's OK. I'd put a space after the if though.

Comparing a boolean to true is pretty pointless though, and just makes the code needlessly verbose. And since you're not modifying the arguments, it's best to declare them as const.

 

[Rodéric]

Same here, I don't like implicit tests, I prefer to put them in () and put the == true, even though I'm very well aware that I don't need to.
I still find this much more clear, it's especially useful when you do nasty things like that :

bool bRes;
if ( (bRes = readStuff()) == true )
{

}

It's still readable.

I just used a simplified Hungarian, as I find that notation far too cumbersome otherwise, stuff like "m_acBuffer", "m_pTexture", "strName" (for a std::string)
In the end you have to be consitent and make your code easy to read.

Oh ! BTW, noone talked about comments yet, but I try to keep them in a good ratio to make everything clear quickly, for I hate it when I come back to some class and can't get what I did in a matter of seconds.

 

[Bjorn]

I would argue that this

if (x == 1){
    ...
} else {
    ...
}

is more readable than this

if (x == 1)
{
    ...
}
else
{
    ...
}

The latter only makes the code more verbose. Also the else-statement looks like it would be a statement on its own, rather than a continuation of the if-statement. The scope of the blocks should be obvious from the indentation anyway.

I agree, but i'm doing 99.9% of my work in Java so what do i know

 

[Bjorn]

so when i have the chance to write code by my own standards i have one simple rule:

it's the reader's responsibility to know the language of the code, it's my responsibility to explain my algorithmic decisions and conventions.

True, it's hilarious to see comments in code like f.e

// declaring someObject
SomeObject so = new SomeObject();

....
// setting attribute..
so.setAttribute("warning, stupid comments in code");

Seen it quite often actually.

 

[Xmas]

Same here, I don't like implicit tests, I prefer to put them in () and put the == true, even though I'm very well aware that I don't need to.
I still find this much more clear, it's especially useful when you do nasty things like that :

bool bRes;
if ( (bRes = readStuff()) == true )
{

}

It's still readable.

Ah, on my list of things I hate about C (-like languages), assignments having a return value is not far from the top (but C strings take the top spot by far. Biggest mistake in programming history IMO).
Why not like this:

bool bRes = readStuff();
if (bRes)    // (bRes == true) if you like
{

}

 

[Rodéric]

Ah, on my list of things I hate about C (-like languages), assignments having a return value is not far from the top (but C strings take the top spot by far. Biggest mistake in programming history IMO).
Why not like this:

bool bRes = readStuff();
if (bRes)    // (bRes == true) if you like
{

}

Cause it was in a loop in fact, AFAIR, and the bRes value was used after the loop to see if anything went wrong.
So it was more like :

bool bRes;
for ( ...; ((bRes = readStuff()) == true) && (...); ...)
{
...
}

if ( bRes )
{
...
}

Something like that, but anyway it's an example ^^

 

[arjan de lumens]

Using assignments in comparisons is not terribly nice, and usually something that is not too hard to get rid of. But still, is

int *ptr;

while( (ptr = get_some_value()) != NULL && other_condition() == true)
{
   do_something( ptr );
}

really less readable than e.g.

int *ptr;
bool cond;

do
{
   ptr = get_some_value();
   cond = other_condition();
   if(ptr != NULL && cond == true)
   {
      do_something( ptr );
   }
}
while( ptr != NULL && cond == true);

or

int *ptr;

for(;;)
{
   ptr = get_some_value();
   if(ptr != NULL && other_condition() == true)
   {
      do_something(ptr);
   }
   else   
      break;
}

 

[Remi]

Hum...

There would be a lot to say about coding standards, as Steve McConnel very correctly notice in "Code Complete", it's very easy to get religious and to only defend something because it's something you're used to.

I just would like to point out that the goal of coding standards is to make your life easier, and that it depends on the context.

For instance, hungarian notation is there to help you keep clear of type bugs, hence it's usefull in programming environements without type safety and useless in programming environements with type safety - in other words, good for C, not for C#.

What's best ?

if (condition)
{
    /* (english description) */
    do_something(with_this_stuff);
}
else
{
    /* (english description) */
    do_some_other_thing(with_this_stuff);
}

or

if (condition) {
    /* (english description) */
    do_something(with_this_stuff);
} else {
    /* (english description) */
    do_some_other_thing(with_this_stuff);
}

or even

if (condition) { do_something(with_this_stuff); }         // (description)
else           { do_some_other_thing(with_this_stuff); }  // (description)

I prefer the first style other the second one, because it turns logical blocks into visual blocks. The second one keeps it in a style that is closer to litteracy. I'm slightly more visual than auditory in the way I think and learn, therefore I prefer the first style and I assume that probably most of the people who prefer the second style tend to be the opposite (more auditory than visual).

What's the best way ? The ones that best match the way you think.

Which might not help much when it comes to team-work. I would be enclined to believe that there are more programmers on the visual side than on the auditory one - but I have no fact to backup that, it's only a feeling from experience.

In fact, after a while working on personal projects, I made up my own style, which is the last one. To me, it combines the best of both worlds - clear and dense. Of course it goes against the classical tradition of '80 characters per line max.', but maybe someone will be able to explain to its defenders that laser printer and HD-era computer screens have little in common with daisy-wheel printers and these good old 3270 IBM terminals - you know, the ones that nowadays even comes with colours! (And btw, to tell the truth, I would probably even align the function's arguments on the same vertical line).

There's no thing such as good coding standards in the absolute, they depend of the context - of course the technology but also the team of programmers...

PS: and don't forget the goal: make your life easier!

 

[Rodéric]

Using assignments in comparisons is not terribly nice, and usually something that is not too hard to get rid of. But still, is
...

I prefer the first, I don't like if in loops and a break in a for is even worse IMO.

 

[KimB]

Why use for(;; ) instead of while(true)?

 

[BRiT]

Why use for(;; ) instead of while(true)?

Some people prefer for loops over while loops, even in the simplistic form.

 

[epicstruggle]

I try my best to make sure that my for loops have a comparison to zero, instead of something else.

for ( ; x >  0 ; )

instead of

for ( ; x < arraysize ; )

just my 2 cents.

 

[KimB]

Why would you do that, Epic? I mean, the usual way of thinking about most for loops I've run into is incrementing a variable up to some maximum (like, say, stepping through an array).

 

[epicstruggle]

Why would you do that, Epic? I mean, the usual way of thinking about most for loops I've run into is incrementing a variable up to some maximum (like, say, stepping through an array).

speed optimization. comparisons to 0 are cheaper than to someother value.

epic