Just a matter of style.. [modified]

bigdenny200

Hey guys, I encountered this code at one great CP article.

BOOL CAESEncRegKey::SetKey(const BYTE *cbKey, UINT nLength)
{
BOOL bResult = FALSE;

ASSERT( CryptoPP::AES::DEFAULT\_KEYLENGTH == nLength );
ASSERT( NULL != cbKey );

if( CryptoPP::AES::DEFAULT\_KEYLENGTH == nLength && NULL != cbKey ) {

    \_EncKey = cbKey;

    bResult = TRUE;
}

return bResult;

}

I just wonder whether rewriting it in the following style, looks better ?

BOOL CAESEncRegKey::SetKey(const BYTE *cbKey, UINT nLength)
{
if( CryptoPP::AES::DEFAULT_KEYLENGTH != nLength || NULL == cbKey )
{
ASSERT(FALSE);
return false;
}

\_EncKey = cbKey;
return true;

}

What do you think ? Any comments.. -- modified at 15:08 Thursday 4th October, 2007

Maximilien

more efficient ? probably not; it's basically the same code, keep in mind that in the debug version, the first version will do additional comparisons, but we really don't care about efficiency in debug version. more readable ? who can say ? is it tomato or tomato ?

Maximilien Lincourt Your Head A Splode - Strong Bad

bigdenny200

which one do you like more ? :) just asking..

Mark Salsbery

I bet most will like the first better. One exit point. Mark

Mark Salsbery Microsoft MVP - Visual C++ :java:

David Crow

bigdenny200 wrote:

What do you think ?

I think...that the compiler (probably) couldn't care less.

"A good athlete is the result of a good and worthy opponent." - David Crow

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Laurence Sterne

bigdenny200

David, I didnt ask from the compiler point of view :)

bigdenny200

Hi Mark,

Mark Salsbery wrote:

One exit point.

Can you give an explanation why is that good? thanks

led mike

bigdenny200 wrote:

Can you give an explanation why is that good?

Don't tell him unless he knows the secret handshake

David Crow

But I responded as such. I've seen way too many people get all hung up on how pretty/tight the code looks, or how efficient they *think* it is, when it matters not one iota. Trimming a few variables/statements from an already small function just screams of wasted time. Unless a profiler was used to gather actual metrics, there's really no use in doing it.

"A good athlete is the result of a good and worthy opponent." - David Crow

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Laurence Sterne

Mark Salsbery

Hehe just in time....I was just about to reply! :)

Mark Salsbery Microsoft MVP - Visual C++ :java:

bigdenny200

So where is the reply :D

David Crow

When debugging, you only have to set one breakpoint. Otherwise, you have to set one on each return statement. With older compilers, if you returned from a function that was in the middle of a for/while loop, the stack was not properly cleaned up. That's not the case anymore.

"A good athlete is the result of a good and worthy opponent." - David Crow

"To have a respect for ourselves guides our morals; to have deference for others governs our manners." - Laurence Sterne

Mark Salsbery

Heh. Where's the secret handshake. This came up recently in a discussion here. I actually argued that if I can see all the exit points in a function easily then I don't mind. That doesn't mean it's good practice on my part. Most (if not all) others that commented stated they prefer a single exit point. (The discussion was about nested "if"s versus multiple exit points.) A problem occurs in changes later - any cleanup of new objects may need to be done at every exit point. What if you miss one? I looked at my code and found many places I used multiple returns in a function. Mostly wordy initialization stuff like DirectX. Nested ifs get too deep for my liking. I guess a goto to a common exit point works, but who likes gotos? What's the alternative, a try block using an exception handler as the common exit point? I dunno... that's a glorified goto :) Personally, in your posted code example, I thought the first one just looked better and was easier to read. I have no need to save whitespace or to minimize line count. I have plenty of harddisk space. Mark

Mark Salsbery Microsoft MVP - Visual C++ :java:

bigdenny200

>> "When debugging, you only have to set one breakpoint. Otherwise, you have to set one on each return statement." That's valid. But in this case, ASSERT's help.

bigdenny200

"..was easier to read. " I agree with that. :). // Not anymore :)) After I looked at it once again Just, for me the second looks more compact. -- modified at 17:31 Thursday 4th October, 2007

El Corazon

Mark Salsbery wrote:

A problem occurs in changes later - any cleanup of new objects may need to be done at every exit point. What if you miss one?

I have no manditory rules on this, though I do try to have a single exit point for just those cleanup reasons, I do have multiple exit points for culling determination. For instance checking a sensor for operations: sensor() { if outside range return; if outside azimuth field of view return; if outside elevation field of view return; allocate variables; process; cleanup; return; } in this case there are multipe exit points, but the idea is to reduce the processed data before the allocation of process overhead. Therefore the resources are reduced by the previous exit points, and only one exit point has to worry about cleanup. I consider that a single exit point. It could be rewritten as a single exit point: sensor() { if (inside range && inside azimuth && inside elevation) { allocate variables; process; cleanup; } return; } but when the list of checks grows significantly inside the if statement it still becomes difficult to read. In the case of the sensor comparisons, there are more checks going on there I can't show, and the list did get too lengthy, so I used the culling paradigm instead. Even in debugging it is easier to follow than a single long if.

_________________________ Asu no koto o ieba, tenjo de nezumi ga warau. Talk about things of tomorrow and the mice in the ceiling laugh. (Japanese Proverb)

Mark Salsbery

Thanks L. That's cool and I like it. What about something like this though....again I'll loosely use Direct(whatever) as an example... SetupDirect____() { create com object instance if (succeeded) create com object instance if (succeeded) create com object instance if (succeeded) ... } If any creates fail, then all the previously created objects need to be released. I always struggle on a good way to code this... Thoughts, examples, advice? Mark

Mark Salsbery Microsoft MVP - Visual C++ :java:

Michael Dunn

I totally, completely despise putting the { on the same line as the if/while/whatever. That makes it very hard to find matching braces when you're skimming through code. The { and } absolutely must be in the same column.

--Mike-- Visual C++ MVP :cool: LINKS~! Ericahist | PimpFish | CP SearchBar v3.0 | C++ Forum FAQ "That's what's great about doing user interface work. No matter what you do, people will say that what you did was idiotic." -- Raymond Chen

bigdenny200

:D That was part of my modification as well :D And I agree totally, with {} being in the same column.

John R Shaw

This is an old debate and is a matter of preference or company policy. I prefer and use the option 2, because I find it easier to read and I like to be consistent with my placement of braces. Once upon a time I used option 1, but that was when programming in C. Now days with C++ and now .NET, the length of the lines are harder to control. I do not like to have to search for the starting brace, because it interferes with my flow of thought. Life is complicated enough as it is.

INTP "Program testing can be used to show the presence of bugs, but never to show their absence."Edsger Dijkstra