One of my little things when coding is that I always feel ashamed when my code is not clean. This means I try very hard to make my code simple and easy to understand. I don't always succeed (not even close), but I try.
I wasn't always like that, mind you. As a teenager, my code was just a horrible mess of spaghetti code. But Pablo Orduña showed me long ago the value of easily understandable code, and since then I always try my best to code in a clear and non-hackish way.
Anyway, these days I've found myself working with code from other people. And, at that, people that didn't try to create readable code. I'm sure they did know how to do it. They just weren't in the mood. You can just read it in the code: "if it works, ship it!"
As a small example (there are much much worse things than this), here's a slightly modified version of a function I found the other day:
In fact,the whole structure of the function allows us to remove most of the 9 (yes, 9) indentation levels in that code. We just have to reverse the ifs and return early. Like this:
It takes less than a minute, it's safe to do, the meaning of the program is exactly the same... and it's much cleaner and easy to understand! And as a bonus, it isn't even longer than the previous version!!
There are even clever people that have said this before me several times!
So please, do try. I know it seems easier and faster not to, but it's only in the short run. Some day (when you have to go back to try to understand your own code) you'll regret it.
Still unconvinced? Now try imagining this with 700 loc functions (and yes, I have a few like that one here...)
I wasn't always like that, mind you. As a teenager, my code was just a horrible mess of spaghetti code. But Pablo Orduña showed me long ago the value of easily understandable code, and since then I always try my best to code in a clear and non-hackish way.
Anyway, these days I've found myself working with code from other people. And, at that, people that didn't try to create readable code. I'm sure they did know how to do it. They just weren't in the mood. You can just read it in the code: "if it works, ship it!"
As a small example (there are much much worse things than this), here's a slightly modified version of a function I found the other day:
bool RetrieveProperty(object_t * object, string propName, VARIANT* pPropertyValue)
{
PropertyReader * pPropertyReader = NULL;
object->FromInterface(IID_IPropertyReader, (void**)&pPropertyReader);
if (pPropertyReader)
{
Properties * pProperties = NULL;
pPropertyReader->get_Properties(&pProperties);
if (pProperties)
{
long propCount = 0;
if (pProperties->get_Count(&propCount) == OK)
{
for (long i = 0; i < propCount; i++)
{
Property * pProperty = NULL;
pProperties->get_Item(i, &pProperty);
if (pProperty)
{
PropertyDictionary * pPropertyDictionary = NULL;
pProperty->FromInterface(IID_PropertyDictionary, (void**)&pPropertyDictionary);
if (pPropertyDictionary)
{
string bDictionaryName;
pPropertyDictionary->get_Name(&bDictionaryName);
if (!strcmp((char*)bDictionaryName.c_str(), "MY_DICTIONARY"))
{
HRESULT hr;
if ((hr = pPropertyDictionary->get_Item(PropertyName, pPropertyValue)) == OK)
return true;
}
}
}
}
}
}
}
return false;
}
Now, did anyone notice something odd? The nested ifs and loops? They make it unnecessarily hard to follow the program flow and they don't buy us anything in return!
In fact,the whole structure of the function allows us to remove most of the 9 (yes, 9) indentation levels in that code. We just have to reverse the ifs and return early. Like this:
bool RetrieveProperty(object_t * object, string propName, VARIANT* pPropertyValue)
{
PropertyReader * pPropertyReader = NULL;
object->FromInterface( IID_IPropertyReader, (void**)&pPropertyReader);
if (!pPropertyReader)
return false;
Properties * pProperties = NULL;
pPropertyReader->get_Properties(&pProperties);
if (!pProperties)
return false;
long propCount = 0;
if (pProperties->get_Count(&propCount) != OK)
return false;
for (long i = 0; i < propCount; i++)
{
Property * pProperty = NULL;
pProperties->get_Item(i, &pProperty);
if (!pProperty)
continue;
PropertyDictionary * pPropertyDictionary = NULL;
pProperty->FromInterface(IID_PropertyDictionary, (void**)&pPropertyDictionary);
if (!pPropertyDictionary)
continue;
string bDictionaryName;
pPropertyDictionary->get_Name(&bDictionaryName);
if (strcmp((char*)bDictionaryName.c_str(), "MY_DICTIONARY"))
continue;
HRESULT hr; // why do we need this, I say?
if (hr = pPropertyDictionary->get_Item(PropertyName, pPropertyValue) == OK)
return true;
}
return false;
}
Max indentation: 3 levels.
It takes less than a minute, it's safe to do, the meaning of the program is exactly the same... and it's much cleaner and easy to understand! And as a bonus, it isn't even longer than the previous version!!
There are even clever people that have said this before me several times!
So please, do try. I know it seems easier and faster not to, but it's only in the short run. Some day (when you have to go back to try to understand your own code) you'll regret it.
Still unconvinced? Now try imagining this with 700 loc functions (and yes, I have a few like that one here...)
I agree with you about the not-hackish-way is the correct programming way. In fact, I always write my code thinking that the next programmer to maintain it is a serial killer - and has my address.
But I don't agree with you in this concrete example. In fact, its quite more hackish to use continue than nested if-else expressions. In your way, you need to read the code to find out where is the flow changing, but in the original it's really easy to find the set of conditions that will make the method return true - even if this conditions are horribly written!
Okay, in your code the flow-changing expressions are also really easy to find, but only because of the pretty-coloured-printer that uses this bright blue color to show them - but that doesn't the code clean itself, because you are depending on the editor.
IMHO the correct way to solve this is to encapsulate the each if { ... } expression in another method - maybe even with the same name and different signature. Little example:
bool RetrieveProperty(object_t * object, string propName, VARIANT* pPropertyValue) {
PropertyReader * pPropertyReader = NULL;
object->FromInterface( IID_IPropertyReader, (void**)&pPropertyReader);
if (!pPropertyReader)
return false;
else
return RetriveProperty(object, propName, pPropertyValue, pPropertyReader);
}
I usually prefer to use a lot of small methods rather than a big one, even if they are not called more than in one place. This way the code is readable and if you do a nice work with the method name and signature, they are self-commented.
I must clearify - I like your code much more than the other one, but I cannot help myself, I love the hackish coding way :D
Anyway, I think this topic is pretty subjective, but I enjoy discussing about it a lot :D
Hi Diego!
Of course the best way to do it is to extract functionality into smaller methods. That's in fact the next step (I never said the refactoring was complete ;-)
About the readability, think about that same method, but with just one line of code after each closing bracket. If you then add a few else clauses for some of the ifs, It's absolutely impossible to follow the program flow :-(
I've got code here that goes to 13 levels of indentation. Try to follow that, when half of those ifs have an else clause!
Hehehe I agree, is totally unreadable (admit it, you use a 13'3 inch screen, that's because! - kidding :D)... but still, I think that continue its pretty hackish, and I also think that putting a return sentence and continue coding just under it is not clean at all.
I think that if you continue the refactoring from your code, probably the final code will still have those things. On the other hand, if you start with a well-nested if-else block (of course, I agree that the first example neither achieve that :D) and then extract to methods, you'll avoid them.
Anyway if I found both codes, sure I would hate the first one programmer, and thought yours is neat - but it's a little C-ish to my official taste (my internal taste love that! although I can never put it in practice :).
Personally, I tend to go with the first version (nested ifs), but even further: I only use "return" once in a function.
Type function(parameters...) { Type result = default;if foo
{
if bar
{
if baz
{
if foobar
{
result = qux;
}
result += 2;
}
}
else
{
result = quux;
}
}
return result;
}
In my experience, introducing GOTOs in the code (a return is a wrapped GOTO) is rarely useful.
Imagine if you wanted to print the result before returning: either add a printf before each return, or add a printf after each function call, only if the call is assigned to some variable for later retrieval).
Or imagine if you're working with bare pointers, and you have to free memory before returning.
Remember vim shortcuts like '%', which help figure out which condition the bracket belongs to.
The alternative (appart from refactoring condition checks into separate functions), is to use exceptions, which take care of all the problems nicely.
Still, I'm not fully comfortable with my approach either :-) Sometimes, code simply is complex, and there's little you can do to improve readability, other than switching to a different language ;-)
The thing about returns is that, technically, yes, they are a form of GOTO; but conceptually they're not.
If you see a return statement you _know_ what's going to happen next: the function ends and returns the value indicated in that statement. With a GOTO, however, you never know until you go looking at the label pointed by the sentence.
So returns are more simple and straightforward than GOTOs, I think we'll both agree on this. The question now is if returns are simpler to understand than heavily nested conditional statements.
You're right in that, sometimes, it's impossible to simplify the code, and its structure reflects the complexity of the task it must accomplish. However, this is not always the case.
In my experience, just sitting back 5 minutes and thinking some alternative solutions almost always produces a simpler, more understandable structure than the one you already have.
Taking your code as an example, here's what I'd do with it:
In my eyes, this looks much simpler than your version and functionally it's exactly the same code ;-)
Oh, and about the "Need to do something after the function ends", there are several ways. One is to call that function from a wrapper that calls the necessary things after the function call.
Another is to use a RAII system (to close files and clean up things, for example) in which you embed the method in a class which destructor does all these things when the object is destroyed.
Of course, all of this adds more code, so you have to think if the added code will improve or decrease readability. There's always a trade-off here :-S