|
Georgi, I agree with you. "Return in the middle" is an horror! 
****************************
Strong congruence
for strong people;
with a compatible behaviour.
For a semantical way of life.
|
|
|
|
|
I would tend do follow Michael Jackson's advice:
"At this point you might be tempted to introduce a flag.
Avoid such Satanic practices."
I quote from memory not having read it in the last 10 years.
Use of 'flags' to control program flow is often a bad code smell.
|
|
|
|
|
While having many return statements might be questionable, I think readability is much more important. So I would always allow for multiple return statements to avoid deep nesting. Deep nesting makes the code going out of the right side of the display and it is also harder to understand. Over time, this can become a big maintainability issue.
In your (first) example, we can do without a single return. We can be explicit and well structured at the same time like so:
bool needToDoSomething = false;
if (FlagA && FlagB && FlagC)
{
needToDoSomething = PromptUser();
}
if (needToDoSomething)
{
DoSomething();
}
else
{
DoOtherThing();
}
You should always write your code such that a potential reader can quickly understand your original intention without the need to scroll in any direction.
Another issue arises with the code above when it comes to unit testing and code coverage: We cannot setup code coverage for every single condition in if (FlagA && FlagB && FlagC) - we can do this only for the whole line. If we want to be accurate with this and setup an individual test case for every single condition, we can only do this by using a waterfall-like coding style:
bool needToDoSomething = false;
if (!needToDoSomething)
needToDoSomething |= FlagA;
if (!needToDoSomething)
needToDoSomething |= FlagB;
if (!needToDoSomething)
needToDoSomething |= FlagC;
if (needToDoSomething)
needToDoSomething = PromptUser();
if (needToDoSomething)
{
DoSomething();
}
else
{
DoOtherThing();
}
Probably not the most elegant solution and surely not the shortest one, but it is easy to read/understand and it has a much better testability than the first example. And this in my view is much more important than any other argument.
Regards
Thomas
www.thomas-weller.de
Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the Universe trying to produce bigger and better idiots. So far, the Universe is winning.
Programmer - an organism that turns coffee into software.
|
|
|
|
|
Thomas Weller wrote: but it is easy to read/understand
No it isn't, I can barely figure it out. Shouldn't needToDoSomething start off true and get changed to false if any test fails?
bool needToDoSomething = true;
if (needToDoSomething) needToDoSomething = FlagA;
if (needToDoSomething) needToDoSomething = FlagB;
if (needToDoSomething) needToDoSomething = FlagC;
if (needToDoSomething) needToDoSomething = PromptUser();
(Dang, C# doesn't have a &&= operator, that'd be useful here.)
Maybe it's a language issue, C# has an actual boolean type, C/C++ doesn't, which are you using?
|
|
|
|
|
Personally, I have no real problem with returns in the middle of a function as long as there are no cleanup considerations, and as long as the procedure remains clear and readable.
For cleanup issues, I believe C++ leads the way through RAII (Resource Acquisition Is Initialisation - RAII (Wikipedia), allowing code to be written in whichever manner makes it clearest to read - any resources are then guaranteed to be cleaned up on exit from the routine.
|
|
|
|
|
In case of complex preconditions separate those to another function.
This prevents structured-ifs and keeps the calling-function readable. If the
conditions change (in thight iterative development this always happens), adaption gets easy.
bool ShouldDoSomething()
{
if(!flagA) return false;
if(!flagB) return false;
if(!flagC) return false;
if(!PromptUser() ) return false;
return true;
}
void callingFunction()
{
if( ShouldDoSomething() )
{
DoSomething();
}
else
{
DoOtherThing();
}
}
|
|
|
|
|
Then why not:
bool ShouldDoSomething()
{
return ( flagA && flagB && flagC && PromptUser() ) ;
}
which is much easier to read? 
|
|
|
|
|
you're right... got a bit infected by the horror around.
|
|
|
|
|
Be aware of Junioria Developerus animal. Those species may decide that PromptUser() && flagA && flagB && flagC looks prettier. 
Cyclomatic complexity for && and if is the same. Some people believe that cyclomatic complexity number is directly related to code maintenance. And in this thread we did not really lower the number. Is the original code fine as is?
|
|
|
|
|
notmasteryet wrote: Junioria Developerus
I suppose that would be a reason not to distance the tests from the code that uses it.
And not to use a lot of negation.
|
|
|
|
|
Ever thought about using &&?
|
|
|
|
|
blockpoint2=BLOCKS+1;
codeblockpos=0;
addbyte(0x89); addbyte(0xFA);
addbyte(0xC1); addbyte(0xEA); addbyte(12);
addbyte(0x8B); addbyte(0x0C); addbyte(0x95);
addlong(vraddrl);
addbyte(0xF6); addbyte(0xC1); addbyte(1);
addbyte(0x75); addbyte(4);
addbyte(0x8B); addbyte(0x14); addbyte(0x39);
addbyte(0xC3);
addbyte(0x57);
addbyte(0xE8);
addlong(readmemfl-(uint32_t)(&rcodeblock[blockpoint2][codeblockpos+4]));
addbyte(0x89); addbyte(0xF9);
addbyte(0xC1); addbyte(0xE9); addbyte(12);
addbyte(0x83); addbyte(0xC4); addbyte(0x04);
addbyte(0x89); addbyte(0xC2);
addbyte(0x8B); addbyte(0x0C); addbyte(0x8D); addlong(vraddrl);
addbyte(0xC3);
There is written to memory and executed there. The only way I can think of making this worse is to remove the comments.
|
|
|
|
|
Timothy Baldwin wrote: The only way I can think of making this worse is to remove the comments.
I agree. It would serve as potential job security though.
"The clue train passed his station without stopping." - John Simmons / outlaw programmer
"Real programmers just throw a bunch of 1s and 0s at the computer to see what sticks" - Pete O'Hanlon
"Not only do you continue to babble nonsense, you can't even correctly remember the nonsense you babbled just minutes ago." - Rob Graham
|
|
|
|
|
Ohhh... so that's how you do that...
|
|
|
|
|
There are cases where I would consider such code appropriate. If the routine in question will represent 90% of the running time of the program in which it resides, writing the routine as indicated will reduce its own running time by 60%, the running time of the program will be significant, and there is no other practical way to achieve such a speedup, then such code could be reasonable.
Such code may also be reasonable in cases where one wishes to thwart disassembly. In such cases, one may have to tolerate some messiness in the source to obfuscate the machine code. If obfuscation of the machine code is a bona fide and legitimate goal, the nasty source code may be an acceptable price to pay.
The above code doesn't seem to contain any loops, so it wouldn't have much use as a speedup method. It might be designed to discourage reverse-engineering, though there would be better ways of accomplishing that.
|
|
|
|
|
The C code only executes once. It would have been better to use an assembler!
However most of this file is a JIT complier written in a similar style - a complete absence of symbolic constants. And it is slow.
As for discouraging reverse-engineering, this code is published under the GNU General Public Licence.
|
|
|
|
|
Timothy Baldwin wrote: As for discouraging reverse-engineering, this code is published under the GNU General Public Licence
If it weren't for the comments, that might explain it.
BTW, would the GPL require the release of source code for things like p-code which is interpreted by the rest of the program? What if the source code for the p-code never existed (because it was hand-generated)?
|
|
|
|
|
Too much code, too much writing. It could be done in a simple and write-only way like this:
byte mreadmem[ ] = {0x89,0xFA,0xC1,0xEA,0x0C,0x8B,...,0xC3 };
addbytes(mreadmem);
Sometimes if your code is crap it is better to asm it down to make others thinking "Uh, better not touch that".
Greetings - Gajatko
Portable.NET is part of DotGNU, a project to build a complete Free Software replacement for .NET - a system that truly belongs to the developers.
|
|
|
|
|
Here is a code i came across in my own project. Apparently I coded this when I was learning C# few years back. It gave me a good laugh now
bool isActive;
if (chkExamActive.Checked == false)
isActive = false;
else
isActive = true;
EDIT
OK I was just going through this old project of mine so here is another one in the same .cs file
ArrayList correctAnswers = new ArrayList();
correctAnswers.Add(txtPhrase.Text);
QuestionTableAdapter questionAdapter = new QuestionTableAdapter();
long QuestionID = questionAdapter.GetData()[0].QuestionID;
CorrectAnswerTableAdapter correctAnswerAdapter = new CorrectAnswerTableAdapter();
foreach (string correctAnswer in correctAnswers)
correctAnswerAdapter.Insert(correctAnswer, QuestionID);
Find the horror above 
-------------------------------------------
It's code that drives you - Shyam
modified on Monday, November 24, 2008 11:26 AM
|
|
|
|
|
That sort of thing has been posted so often it has lost its horror.
|
|
|
|
|
Yeah even I saw this horror too many times here, so I expected this to have lost its horror...
I edited the post with one more horror I found. Hope this horror is not so common
-------------------------------------------
It's code that drives you - Shyam
|
|
|
|
|
bool isActive = chkExamActive.Checked;
This would cause issue if the checkbox were IsThreeState, because it has a third state, indetermintate. The code above could therefore give isActive a null value, therefore possibly causing issues later.
However, the code you posted is still crap.
|
|
|
|
|
My mistake for not mentioning it... This is code behind for ASP.net page so I guess IsThreeState situation wouldn't make my code great 
-------------------------------------------
It's code that drives you - Shyam
|
|
|
|
|
Shyam Bharath wrote:
ArrayList correctAnswers = new ArrayList();
correctAnswers.Add(txtPhrase.Text);
QuestionTableAdapter questionAdapter = new QuestionTableAdapter();
long QuestionID = questionAdapter.GetData()[0].QuestionID;
CorrectAnswerTableAdapter correctAnswerAdapter = new CorrectAnswerTableAdapter();
foreach (string correctAnswer in correctAnswers)
correctAnswerAdapter.Insert(correctAnswer, QuestionID);
a few guesses at first glance:
0. no txtPhrase validation
1. questionAdapter will be empty since it is just only constructed (failure) -OR- it has a default, random (?) data entry or a static ID field acquiarable only by an instance method (horror).
2. [supposition:] question data is queried on each method call just to get an ID instead storing it somewhere.
3. correctAnswers contains just one element and so foreach loop is redutant.
are some of them correct?
Greetings - Gajatko
Portable.NET is part of DotGNU, a project to build a complete Free Software replacement for .NET - a system that truly belongs to the developers.
|
|
|
|
|
gajatko wrote: are some of them correct?
Cool. You got the third one correct. Let me explain others
gajatko wrote: 1. questionAdapter will be empty since it is just only constructed (failure) -OR- it has a default, random (?) data entry or a static ID field acquiarable only by an instance method (horror).
The adapter was generated by xsd tool and it fills data in the next line when I call GetData() method.
For point 2, good supposition  . Luckily I didn't do that horror
gajatko wrote: 0. no txtPhrase validation
I have put validation controls in ASP.net WebForm. I guess that should be enough
Cool that you were able to identify the for loop one. You get my 5
-------------------------------------------
It's code that drives you - Shyam
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
