I just finished writing a function that has ended up with nested code blocks something like the following:
class ... {
void Method() {
while (...) {
...
switch (...) {
while (...) {
switch (...) {
if (...) {
}
}
}
}
}
}
}
Do you find this is standard in your day-to-day coding work, or do you quickly set about trying to redesign and break up the code when you see something like this?
I do find this sometimes in my code. But when I reach this point, I decide that it's time to refactor. Refactoring is quite key because it allows for cleaner code and reduces the apparent complexity of code.
It's very hard to set any specific limit about this, as sometimes it's just the best way to do it. However, if you get something nested that deep, particularly nesting loops giving poor algorithms at times, or ifs inside switches, you can usually abstract things away.
If I got something that deep, I'd definitely refactor.
I find this day-to-day, but only because of the inherited code-base.
In my own code, I would have refactored long ago, even making methods only ever called once to express intention.
I would be very worried about the switch-within-switch construct. That should "never" happen. Especially under the light of questions like this ("using switch is bad OOP style?").
Try to avoid the nesting, because it increments the cyclomatic complexity, why dont you put those on a class, or use another strategy instead.
I remember a pofessor at my university saying "A function, should not have more than 7 lines."
And i keep that in mind :)
Hope this helps Best Regards!
I call this "isolated ugliness."
It's ugly because switches within switches are just plain ugly. They could be avoided with some nice OO techniques. But it gets the job done and takes less time to code.
Isolated because its best to code this up, write some tests for it, and then put it away and never look at it again.
But yes, its OK to do it. Probably not good if you're doing it everywhere.
I'd be more concerned with the big-O complexity of the nested while statements.
Sometimes it ends up that way. I try to pay attention to all of the exit conditions for loops and so forth to see if I can simplify them. I also try to replace switches with polymorphism (where practical) and so forth.
I also tend to keep my code as simple as possible through the inversion of logical statements (where it doesn't hurt readability).
E.g.
Instead of doing this:
public string TransformString(string _input)
{
if(!string.IsNullOrEmpty(_input))
{
// do work
// etc.
return answer;
}
return null;
}
Do this:
public string TransformString(string _input)
{
if(string.IsNullOrEmpty(_input))
return null;
// do work
// etc.
return answer;
}
It helps keep the nesting down and brace-mania away. Sometimes it's better to just keep the nested version (e.g. I don't like hitting a 'continue' keyword half way through a complicated loop body because it's easy to mess up and introduce a bug), but I generally prefer this approach :)
never more than three. as my personal philosophy. However, there are cases where you cannot do better. A typical case is: suppose you have to iterate on all the elements of a 6-indexes matrix. Not your typical case, but sometimes it happens.
So, you could refactor out, say, the innermost three loops. Good... How do you call the routine you refactor ?
In the end, you realize that the highly nested loop is the best for future understanding. Of course this is a special case. If you do have highly nested loops and switches like the one you paste, then you do have a problem, and you should consider giving meaningful names to the various parts, isolate them, tackle the switch with object-orientation, etc..
I was looking for code nesting horror stories. I just spend time refactoring a single method class with 16 levels of nesting at its deepest point.
And to add insult to injury, the whole mess was wrapped in:
try{
//1527 lines of code, 16 levels deep
} catch(Exception e) {
log.error(e.getMessage());
throw e;
}
I pulled out six inner classes implementing a common interface, a dozen private methods and an enum. The result was 500 less lines of code, negligible redundancy and only six levels of nesting in the public method.
My rule of thumb code metric has been:
A) For every level of nesting past 3, the probability that you should refactor increases by 1/3
B) For every screen-full of code in your method, the probability that you should refactor increases by 1/3
C) For every line of duplicated code in a method, the probability that you should refactor increases by 1/3
I try not to let it get any more crazy than this (5/6 levels)...sometimes it can be unavoidable. If all of my files are in the same namespace, I drop the namespace declaration and declare it in the project properties which cuts down one level.
namespace MyNamespace
{
class MyClass
{
static void MyMethod()
{
try
{
if(...)
{
for(...)
{
}
}
}
finally
{
}
}
}
}
A method should be dedicated to one thing and one thing only...where possible. There are many "rules of thumb", some can be worked to 100% of the time, but most of them while workable 90% of the time cannot cover every angle. Guidelines are just that - they shouldn't restrict you to a point you can't do your job.
Use your common sense, if your code is getting unreadable, you've got too many nests.
I try and follow these guidelines where I can: