attack of silly coding standard?

[Ian Collins]

It's easier to place a single breakpoint than half-a-dozen breakpoints.

I thought most debuggers allow a breakpoint on the closing brace of a
function to catch any return?

I've been down this road on this group before. I'm not dogmatic about
it, but I find that I practice single-exit more than most people, sorry
if you take exception to such a thing.

I don't take exception to single returns, I'm just not concerned about them.

I also differentiate between testing and debugging.

 

[Goran]

Hello Everyone,

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"

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

This is quite common, is typical for C, and is even part of MISRA
recommendations.

In C (or anywhere else where you have no RAII), and as soon as there
are function-local resources, multiple returns are dangerous, hence
the rule.

In C++, things are much better in that respect, for two reasons:
there's RAII, so function-local resources are much more easy to
handle, and, there's exceptions, who pretty much force the code to use
RAII and eliminate by far the biggest reason to use multiple returns.

So... Yeah, your rule isn't pretty, but isn't all too bad for C. In
good C++, multiple returns are just much, much rarer, so there, rule
IMHO isn't relevant.

Goran.

 

[Stuart Golodetz]

How do you test inside a function? Boxes come in all shapes and sizes.

Well one way is to step through it in the debugger and make sure it's
doing the right thing for a particular input. That's not an automated
test, of course, but neither is it really debugging, if you don't happen
to have reason to believe that the code is broken before you do the testing.

I'm not using this as an argument for or against single returns,
incidentally (I happen personally not to like the rule in question, but
that's a side issue). Just observing that there's more to testing than
checking a function's output for a small sample of all the possible
inputs -- as I'm sure you already appreciate. Testing *how* things get
returned can be important.

Cheers,
Stu

 

[Richard]

[Please do not mail me a copy of your followup]

gwowen <[email protected]> spake the secret code

IMHO: The complexity metric comes done, clarity rarely increases.

I'd be curious to know which complexity metrics go down when you
refactor "return" into a "goto" as the number of possible code paths
remain the same.

 

[Richard]

[Please do not mail me a copy of your followup]

mojmir <[email protected]> spake the secret code

I'd like to know you professional opinion on following coding rule
freshly imposed by my beloved employer: [...]

I'll add a different opinion:

Since your employer is paying you to write code, you should write it
the way they want it written. It is their intellectual property, so
if the code is not to your personal tastes, that doesn't really matter
because its not your code, its theirs. Because its theirs, it should
be written to their tastes, not yours.

If you take this approach to code that you're paid to write, things
get a *lot* easier. You don't end up getting embroiled in arguments
about style because you simply adopt the style they request. If they
have a coding standard that says specific things are required, chances
are it was in response to something that happened earlier that caused
them problems and they attempted to address it in their coding
standard. It could also be the personal whim of the person who
introduced the coding standard of the organization.

Your brain cycles are more effectively spent on solving problems and
adding value to the company than tilting against style/coding standard
windmills.

 

[Stuart Golodetz]

[Please do not mail me a copy of your followup]

mojmir<[email protected]> spake the secret code

I'd like to know you professional opinion on following coding rule
freshly imposed by my beloved employer: [...]

I'll add a different opinion:

Since your employer is paying you to write code, you should write it
the way they want it written. It is their intellectual property, so
if the code is not to your personal tastes, that doesn't really matter
because its not your code, its theirs. Because its theirs, it should
be written to their tastes, not yours.

Your employer is paying you to do the best you can for their business,
not just to do what you're told without question. You shouldn't question
minor things that don't make any difference to the end result, but you
should always provide those above you with appropriate information that
might affect the business decisions that are their responsibility to
take. In the case of silly coding rules like this, I'd err on the side
of not making too much fuss, but if a rule imposed from above
fundamentally threatened the quality of the product, then it would be
important for the person above me to be told. You're not paid to be a
mindless automaton -- no-one likes someone who complains all the time,
but nor do good employers want to employ a robot who follows all their
orders to the letter, even when they're daft. If they still want it done
their way once you've talked to them, then so be it -- but it's your
responsibility to give them the information they might need to take that
decision.

If you take this approach to code that you're paid to write, things
get a *lot* easier. You don't end up getting embroiled in arguments
about style because you simply adopt the style they request. If they
have a coding standard that says specific things are required, chances
are it was in response to something that happened earlier that caused
them problems and they attempted to address it in their coding
standard. It could also be the personal whim of the person who
introduced the coding standard of the organization.

Your brain cycles are more effectively spent on solving problems and
adding value to the company than tilting against style/coding standard
windmills.

Agreed -- but it is possible to devise coding rules that restrict
programmers to the extent that it prevents them adding value to the
company to the best of their abilities. If and when that's the case
(it's not here), they have to speak up.

Cheers,
Stu

 

[Öö Tiib]

Agreed -- but it is possible to devise coding rules that restrict
programmers to the extent that it prevents them adding value to the
company to the best of their abilities. If and when that's the case
(it's not here), they have to speak up.

I also prefer projects where coding standards are set and agreed by
the team. Casual silly rule however like the one under discussion can
not do much harm to productivity. It will just add little logic
exercise ... how to follow that rule and still keep things robust,
elegant, flexible and readable.

 

[Stuart Golodetz]

I also prefer projects where coding standards are set and agreed by
the team. Casual silly rule however like the one under discussion can
not do much harm to productivity. It will just add little logic
exercise ... how to follow that rule and still keep things robust,
elegant, flexible and readable.

Agreed, on both counts

 

[Dombo]

Op 23-Nov-10 23:26, Matthias Meixner schreef:

Am 23.11.2010 19:24, schrieb Puppet_Sock:

Since error checking is an important part, most functions will have more
than one return statement....

Or throw statement..

 

[Ebenezer]

[Please do not mail me a copy of your followup]

mojmir <[email protected]> spake the secret code

I'd like to know you professional opinion on following coding rule
freshly imposed by my beloved employer: [...]

I'll add a different opinion:

Since your employer is paying you to write code, you should write it
the way they want it written.  It is their intellectual property, so
if the code is not to your personal tastes, that doesn't really matter
because its not your code, its theirs.  Because its theirs, it should
be written to their tastes, not yours.

If you take this approach to code that you're paid to write, things
get a *lot* easier.  You don't end up getting embroiled in arguments
about style because you simply adopt the style they request.  If they
have a coding standard that says specific things are required, chances
are it was in response to something that happened earlier that caused
them problems and they attempted to address it in their coding
standard.  It could also be the personal whim of the person who
introduced the coding standard of the organization.

Your brain cycles are more effectively spent on solving problems and
adding value to the company than tilting against style/coding standard
windmills.

I think the "brain cycles" part there isn't helpful. I might write
it as, "Your time is more effectively spent ...", but I'm not exactly
sure how I know how his time would be more effectively spent.


Brian Wood
Ebenezer Enterprises

 

[James Kanze]

Sounds a little too dogmatic the way you word it.

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]

That's exactly what I said about the rule too. Seems a little too
dogmatic. If it simply said, "prefer single return" rather than "thou
shalt not have multiple returns" (i.e., if it was a guideline,) I would
be fine with it... It's how I normally code anyway.

Independently, if you have coding rules, instead of coding
guidelines, it's probably a problem. (On the other hand, you do
need guidelines, and some of them do have almost the force of
rules; e.g. where you put braces. But those are generally only
the more or less arbitrary ones.)

 

[Stuart Golodetz]

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.

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

 

[Andrea Crotti]

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

Well you ALWAYS return modification anyway, why not just

if (...)
else if(...)
else if(...)

return modification;

For my taste the function does already too many things, maybe with
some
more polymorphism it could look better (but I know I exaggerate
sometimes )

 

[Stuart Golodetz]

Well you ALWAYS return modification anyway, why not just

if (...)
else if(...)
else if(...)

return modification;

Because I don't have the things I want to test available at the right
points, e.g. I want to test ancestor or descendants, but I need to
calculate them first. I don't want to calculate them at the start of the
function, because that might not be necessary. I could write it as:

PFNodeID ancestor;
std::stack<PFNodeID> trail;

....

else if((ancestor = find_ancestor_in_representation(node, trail)) !=
PFNodeID::invalid())
{
...
}

....

etc., but then I have to create all the required objects up-front,
possibly unnecessarily. I'm not sure it's really clearer that way.

For my taste the function does already too many things, maybe with
some
more polymorphism it could look better (but I know I exaggerate
sometimes )

It might well do too many things So perhaps breaking it into multiple
functions might be an improvement, I'm not sure. Still wonder whether
that's really clearer though. Not sure what you're thinking of in terms
of polymorphism here -- could you elaborate?

Cheers,
Stu

 

[Andrea Crotti]

Because I don't have the things I want to test available at the right
points, e.g. I want to test ancestor or descendants, but I need to
calculate them first. I don't want to calculate them at the start of
the function, because that might not be necessary. I could write it
as:

PFNodeID ancestor;
std::stack<PFNodeID> trail;

...

else if((ancestor = find_ancestor_in_representation(node, trail)) !=
PFNodeID::invalid())
{
...
}

...

etc., but then I have to create all the required objects up-front,
possibly unnecessarily. I'm not sure it's really clearer that way.

I don't get your point, if you do
if (...)
else {
std::stack<...>
}

whenever the first condition is false I never compute anyway the values
below.
There is also some not nice duplication like
m_listeners->node_was_deselected(node, commandDepth);

And that line also is very suspicios.
For my understanding something called
"node_was_deselected" should be BOOL, and thus not being discarded like
this.
That thing I guess has some side effects then, not very nice for a
function called like that...

It might well do too many things So perhaps breaking it into
multiple functions might be an improvement, I'm not sure. Still wonder
whether that's really clearer though. Not sure what you're thinking of
in terms of polymorphism here -- could you elaborate?

Well I don't know the problem enough, but for example you can divide
ancestors and descendants in two separate sets maybe..

 

[Stuart Golodetz]

I don't get your point, if you do
if (...)
else {
std::stack<...>
}

whenever the first condition is false I never compute anyway the values
below.

I can certainly do this:

if(in_representation(node))
{
...
}
else
{
std::stack<PFNodeID> trail;
PFNodeID ancestor = find_ancestor_in_representation(node, trail);
if(ancestor != PFNodeID::invalid())
{
...
}
else
{
std::list<PFNodeID> descendants = descendants_in_representation(node);
if(!descendants.empty())
{
...
}
}
}

But that just makes the thing more nested -- and if anything less rather
than more clear. At the moment, there are certain objections I have to
the way I've written it, but it is at least pretty clear what's going on.

There is also some not nice duplication like
m_listeners->node_was_deselected(node, commandDepth);

Agreed -- that's one of the objections I have to it.

And that line also is very suspicios.
For my understanding something called
"node_was_deselected" should be BOOL, and thus not being discarded like
this.
That thing I guess has some side effects then, not very nice for a
function called like that...

I think you may be missing the point because you don't have the context,
actually -- the point is to alert any listeners that a node was
deselected, so that they can react to it. (It's just an implementation
of the Observer pattern, for what it's worth.) I have similar functions
called things like "layer_will_be_deleted" and "layer_was_deleted" --
the point being that listeners sometimes need to be told before things
happen, sometimes afterwards and sometimes both. Hence the "will be" vs.
"was" aspect of the naming scheme.

For what it's worth, it's quite common to see Model-View implementations
that use a function called something like model_changed -- the name
node_was_deselected is more or less along the same sort of lines.

Well I don't know the problem enough, but for example you can divide
ancestors and descendants in two separate sets maybe..

This is from some hierarchical selection code -- either (1) the node is
selected and none of its ancestors or descendants are, or (2) one of the
node's ancestors is selected and none of that ancestor's ancestors or
descendants are (including the original node), or (3) some of the node's
descendants are selected, but neither it nor any of its ancestors are,
or (4) neither the node, nor any of its ancestors or descendants are
selected.

I'm not sure what you mean about dividing the ancestors and descendants
into two separate sets, though.

Cheers,
Stu

 

[Ruslan Mullakhmetov]

There are also employees who take such policies to the letter without
understanding their trade-offs and applicability. No technical argument
would get through to them. They would look for policy violations in your
code and report to your boss, making the work environment even more
unbearable.

there are also employees who do not care of any such polices and even do
not look into code at all.

so, be happy!

 

[James Kanze]

James Kanze <[email protected]> wrote:

[...]

The most common pattern where my code has more than one return is for
initial precondition checking:

some_type foo(some_arg)
{
if(! check_sanity(some_arg) )
{
// either return immediately with an error value
// or throw, it depends
}

// keep processing
return return_value;
}

I find it nicer than the alternative:

some_type foo(some_arg)
{
some_type return_value = some_predetermined_init;
if(check_sanity(some_arg) )
{
// Keep processing

}
return return_value;
}

This is the most frequent example. In my experience, in C++,
most of the time, these sort of errors would call for an
exception (when not an assertion failure). In the other cases,
the return value must have some sentinal value which must be
returned in case of failure. Using the second pattern,
initializing the return value with this sentinal value, has the
advantage that you can have more complicated checks without
loosing readability---you initialize the return value with the
sentinal, and leave it with that value until you've actually
found the real results.

 

[Bob]

Hello Everyone,

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"

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

Best regards,
Mojmir

I'm going to disagree with most posters here and say that I tend to
avoid multiple returns as they do reduce readability. The times when
I will tolerate multiple returns tend to be when the code is very
symmetric, such as returning from each case in a switch statement or
code like this:

if (cond1) {
return something;
}
else if (cond2) {
return anotherthing;
}
else {
return somethingelse;
}

A counterargument to multiple returns is that it renders the function
less amenable to refactoring and therefore is likely to become even
more unreadable over time as additional conditions need to be handled
etc.

The argument that equates throwing exceptions with return statements
is spurious IMO. Throwing an exception is blatantly not the same as
returning normally from a function and the possibility of an exception
being thrown at a particular line of code does not have the same
effect on the function's amenability to being refactored.