On 25/11/2010 21:26, James Kanze wrote:
On Nov 24, 12:51 am, "Daniel T."<danie...@earthlink.net> wrote:
mojmir<svobod...@gmail.com> wrote:
I'd like to know you professional opinion on following coding rule
freshly imposed by my beloved employer: "Thou shalt not have multiple
returns from function"
Sounds a little too dogmatic the way you word it.
Personally i hardly understand that one, apart from "readability"
argument which i would hardly qualify as sufficent to impose such
rule. When i think about it i found the exact opposite true, that
multiple returns improve readability :)
In theory it shouldn't matter either way, but I personally find that in
practice it is far easier to test if there is only one return to deal
with.
Test, or even reason about. In my own code, about the only time
you're find multiple returns is if the entire function is a
switch, and every case ends with a return (or a throw, or an
abort). Other than that, when I find code with returns all over
the place, the first thing I'll do is clean it up, to make it
readable. Which in practice results in a single return at the
end of the function.
--
James Kanze
Having just done a quick look through my code, I'd say most of my
functions have a single return as well. I found a small number which
used multiple returns though -- just out of curiosity, how would you
refactor something like this to use a single return? (Without worrying
too much about what it does...) I can think of ways of doing it without
too much trouble, but most seem to make it less rather than more readable..
Modification deselect_node_impl(const PFNodeID& node, int commandDepth)
{
Modification modification;
// Case 1: The node itself is in the representation.
if(in_representation(node))
{
erase_node(node, modification);
m_listeners->node_was_deselected(node, commandDepth);
return modification;
}
// Case 2: An ancestor of the node is in the representation.
std::stack<PFNodeID> trail;
PFNodeID ancestor = find_ancestor_in_representation(node, trail);
if(ancestor != PFNodeID::invalid())
{
split_selection(trail, modification);
erase_node(node, modification);
m_listeners->node_was_deselected(node, commandDepth);
return modification;
}
// Case 3: One or more descendants of the node are in the
// representation.
std::list<PFNodeID> descendants = descendants_in_representation(node);
if(!descendants.empty())
{
for(std::list<PFNodeID>::const_iterator it=descendants.begin(),
iend=descendants.end(); it!=iend; ++it)
{
erase_node(*it, modification);
}
m_listeners->node_was_deselected(node, commandDepth);
return modification;
}
// Case 4: Neither the node nor any of its ancestors or
// descendants is in the representation.
return modification;
}
Cheers,
Stu