Just a matter of style.. [modified]
-
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
which one do you like more ? :) just asking..
-
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
I bet most will like the first better. One exit point. Mark
Mark Salsbery Microsoft MVP - Visual C++ :java:
-
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
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 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
David, I didnt ask from the compiler point of view :)
-
I bet most will like the first better. One exit point. Mark
Mark Salsbery Microsoft MVP - Visual C++ :java:
Hi Mark,
Mark Salsbery wrote:
One exit point.
Can you give an explanation why is that good? thanks
-
I bet most will like the first better. One exit point. Mark
Mark Salsbery Microsoft MVP - Visual C++ :java:
-
David, I didnt ask from the compiler point of view :)
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
-
bigdenny200 wrote:
Can you give an explanation why is that good?
Don't tell him unless he knows the secret handshake
Hehe just in time....I was just about to reply! :)
Mark Salsbery Microsoft MVP - Visual C++ :java:
-
Hehe just in time....I was just about to reply! :)
Mark Salsbery Microsoft MVP - Visual C++ :java:
So where is the reply :D
-
Hi Mark,
Mark Salsbery wrote:
One exit point.
Can you give an explanation why is that good? thanks
When debugging, you only have to set one breakpoint. Otherwise, you have to set one on each
returnstatement. With older compilers, if you returned from a function that was in the middle of afor/whileloop, 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
-
So where is the reply :D
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:
-
When debugging, you only have to set one breakpoint. Otherwise, you have to set one on each
returnstatement. With older compilers, if you returned from a function that was in the middle of afor/whileloop, 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
>> "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.
-
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:
"..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
-
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:
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 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)
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:
-
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
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
-
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
:D That was part of my modification as well :D And I agree totally, with {} being in the same column.
-
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
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
-
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:
Mark Salsbery wrote:
Thoughts, examples, advice?
If each object is interdependant upon the creation of the later object then I have no other way of writing it. You are using the naturally stacked architecture of the compiler to build and release objects. Given I am self-taught, others may offer better solutons. Assuming no additional processing needs to be applied you simply write in the form of
setup()
{
create object1
if (success)
{
create object2
if (success)
{
.... (final statements evaluates total success)
}
cleanup object2
}
cleanup object1
return evaluaton of total success
}the clincher is when the algorithm requires post processing before cleanup. evaluatng if/else for every single object is bulky and time-consuming, and looks too busy to follow. But I have a few of those monsters I am not proud of. I will admit it. Others I have stopped and said, "can I define this differently? can I redefine it sideways/backwards or however?" redefining the problem sometimes helps. Sorry, I can't help you there.
_________________________ Asu no koto o ieba, tenjo de nezumi ga warau. Talk about things of tomorrow and the mice in the ceiling laugh. (Japanese Proverb)
-
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
I'm late getting in, but I just now read it. Two viewpoints: First: Scanning code, looking for a quick idea of what's happening. In this context, definitely the second format. Without spending 15 seconds to readjust my glasses, and beginning to mentally parse thru each symbol in the function, I couldn't make heads or tails of the first format. Second: When you KNOW this function has an issue, and you simply MUST trace thru it. Then perhaps the first format, although I didn't get that deep. I stopped after 15 seconds (since I'm not getting paid for looking thru that code). I TRY to write code knowing that non-programmers (but smart folk nonetheless) will be picking thru my code at some point in the future, and I try to write as literarily as possible. Non-literary symbols (the standard C++ operators like &, *, ::, etc.) can make the code difficult to read, even for me, and I do this stuff 8 hours/day! By the way; excellent discussion on exit points. Thanks to the contributors. David