What is Better Code in Your Opinion?

Matt Gerrans

This:

    //If the flag is on then
    if(READBIT(\*pGlobalFlags, uFlag))
    {
            //Value is set to on
            bstrValue = "ON";
    }
    else
    {
            //Value is set to off
            bstrValue = "OFF";
    }

Or this:

bstrValue = READBIT(*pGlobalFlags, uFlag) ? "ON" : "OFF";

:confused:

Matt Gerrans

PIEBALDconsult

Probably the latter. Or possibly: bstrValue = "OFF"; //If the flag is on then if(READBIT(*pGlobalFlags, uFlag)) { //Value is set to on bstrValue = "ON"; } if I'm just going to return the result.

Shog9 0

Why? No redundant comments to infuriate me, that's why.

peterchen

Three useless comments and 4 lines instead of a single line standard construct? Clearly, the second one. "More readable" is the key. Which one is depends a bit on who reads it - but frankly, if someone has troubles with the second, he's got some catching up to do. Some shops say the ternary operator is evil (for whatever obscure reason), so in this case, you would have to go for the first. Some more comments: If you follow standard conventions, READBIT would be a macro - a template can do the same job here. The comments help nothing to clarify the code. Rather, they state the same facts in different words - that's why they are IMO not only useless, but dangerous.

Developers, Developers, Developers, Developers, Developers, Developers, Velopers, Develprs, Developers!
We are a big screwed up dysfunctional psychotic happy family - some more screwed up, others more happy, but everybody's psychotic joint venture definition of CP
Linkify!|Fold With Us!

Marc Clifton

If I really wanted to do it right, I'd do something like:

bstrValue = GetBitStateAsStateString(READBIT(*pGlobalFlags, uFlag));

or, if you want to go for debuggable/more readable:

byte bit=READBIT(*pGlobalFlags, uFlag);
bstrValue = GetBitStateAsStateString(bit);

This gives you at least a fighting chance to handle internationalization and centralizes the meaning of a bit when converted to a string. After all, "True" and "False" might be equally valid, so you might have:

bstrValue = GetBitStateAsTruthString(READBIT(*pGlobalFlags, uFlag));

And no, I don't write code like that usually myself! :) Marc

Thyme In The Country

People are just notoriously impossible. --DavidCrow
There's NO excuse for not commenting your code. -- John Simmons / outlaw programmer
People who say that they will refactor their code later to make it "good" don't understand refactoring, nor the art and craft of programming. -- Josh Smith

Ashley van Gerven

Second one IMO.

"For fifty bucks I'd put my face in their soup and blow." - George Costanza

~ Web SQL Utility - asp.net app to query Access, SQL server, MySQL. Stores history, favourites.

Clickok

I started to think about, but I have noticed what is a programming question in the Lounge, then my opinion goes to soapbox... Ok, ok I'm lazy about select another forum... Here goes: The first option is better. Why? The code is easy to maintain. If I wish add some task if is true (by example), I can do this:

//If the flag is on then
if(READBIT(*pGlobalFlags, uFlag))
{
//Value is set to on
bstrValue = "ON";
// another task here
MyAnotherTaskFunction();
}
else
{
//Value is set to off
bstrValue = "OFF";
}

Notice what I just added 1 line of code, and in your another style I will need create the supressed if construct. Just my 2 cents Regards

For God so loved the world, that he gave his only begotten Son, that whosoever believeth in him should not perish, but have everlasting life.(John 3:16) :badger:

Lost User

// Return 'ON' or 'OFF' for the flag bstrValue = READBIT(*pGlobalFlags, uFlag) ? "ON" : "OFF"; No need for either extreme.

The tigress is here :-D

Kevin McFarlane

They're both fine. I think the second should be used sparingly but there are situations in which it is very elegant.

Kevin

Jorgen Sigvardsson

In my opinion, the latter. Conveys the same meaning as the former snippet, but with a whole lot less verbosity.

-- Made From Meat By-Products

Matt Gerrans

This wasn't really a programming question per se, but a programming aesthetics question, otherwise I would not have posed it here. What you are suggesting there is a nasty form of premature optimization[^] or Premature Generalization[^]. Are you saying you should write the most verbose slop possible, so that it might be easier to insert more stuff into it later? YAGNI[^]!

Matt Gerrans

Matt Gerrans

But your comment is wrong! ;P (which cements a comment by someone else in this thread. Marc, I think it was).

Matt Gerrans

Jerry Hammond

What happened to if(IsLounge) { Label1.Text="No Programming Questions"; } elseif { Label1.Text="Ask in appropriate forum"; }

A room without books is like a body without a soul. - Cicero (106 BC - 43 AD)

Anton Afanasyev

You're all wrong. The correct answer depends on how you're getting paid - by the number of lines written(then the first), or by the functionality implemented (then the second). Yup, thats it;)

:badger:

DavidNohejl

:P It's style question. People usually get away with it. IMO its equal to "Do you like code red or pitr-cola more?" kind of question.

"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus

Matt Gerrans

No, it is an aesthetics question! What programmer has style? ;)

Matt Gerrans

Matt Gerrans

That's just plain wrong (and I'm not just talking about the superfluous curlies) -- I already pointed out that this is not about programming, per se. I guess some people can't tell the difference between a question and a metaquestion. ;P

Matt Gerrans

DavidNohejl

Personally, I would do it like this, kinda taking best of both options (IMO).

// getting string value from flag
strValue = (READBIT(*pGlobalFlags, uFlag))?"ON":"OFF";

// do different stuff depending on value of strValue
if(strValue == "ON")
{
MyTaskFunction();
}
else
{
MyAnotherTaskFunction();
}

"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus

Shog9 0

Clickok wrote:

Notice what I just added 1 line of code, and in your another style I will need create the supressed if construct.

Yeah, but then you'll have your new line of code to focus on, and won't be as tempted to add useless comments to the code you're modifying...

Clickok

Matt Gerrans wrote:

This wasn't really a programming question per se, but a programming aesthetics question,

Just kidding with you ;)

Matt Gerrans wrote:

Are you saying you should write the most verbose slop possible

Not, I'm not saying this. I'm saying what I write code what...

Matt Gerrans wrote:

...might be easier to insert more stuff into it later

The comments what you added really are not necessary, because of the code is self-explanatory. But if I can code some snippet what later will be more easy to customize (like never hard-code strings, if possible declarate variables in beginning of procedures, etc), I will do. It's my habit. And really really turn the things more easy to me :)

For God so loved the world, that he gave his only begotten Son, that whosoever believeth in him should not perish, but have everlasting life.(John 3:16) :badger: