attack of silly coding standard?

[Bob]

Isn't that obvious?  If a function only has one return, it's tempting to
perform clean up before the return.  If a function is written to be
exception or SEME safe, resources allocated have to be automatically freed.

No it's not obvious. I take it as a given that C++ code should be
written in an exception safe way.

Your argument seems to be that because one is writing in a SESE style,
for some reason one forgets to use the standard techniques and idioms
of modern C++, thereby rendering the code exception unsafe. As James
said, there are other reasons for favouring SESE. And BTW I am not
dogmatic about this - I use the word 'favouring' advisedly.

By inspection yes, by testing, no.

Inspection is important in writing, understanding and maintaining
code, is it not?

Because exception safe code is also SEME safe code.

What is 'SEME safe' code?

 

[Ian Collins]

No it's not obvious. I take it as a given that C++ code should be
written in an exception safe way.

Your argument seems to be that because one is writing in a SESE style,
for some reason one forgets to use the standard techniques and idioms
of modern C++, thereby rendering the code exception unsafe.

No, I'm not. Maybe I've just been exposed to too much C code littered
with "goto error"! I was responding to your assertion that multiple
returns render the function less amenable to refactoring, which I
disagree with.

As James
said, there are other reasons for favouring SESE. And BTW I am not
dogmatic about this - I use the word 'favouring' advisedly.

Fair enough, James is becoming increasingly dogmatic, which is goading
me into responding.

Inspection is important in writing, understanding and maintaining
code, is it not?

I was probably too strong there, I should have said "By inspection
maybe". Checking preconditions and retuning early can make the code
easier to inspect and verify.

What is 'SEME safe' code?

Code that doesn't require a single point of return to clean up after its
self.

 

[Stefan Ram]

"Thou shalt not have multiple returns from function"

(I have not read the entire thread, so I might repeat
something written before.)

If this coding standard was issued by an employer, an
employee should try to comply to it, even if he would
not use it when programming in his leisure.

If the employee deems this rule wrong/bad, he can try to
change this standard.

If the employee does not listen and insists on the standard,
any problem caused by it is more a problem of the employer
than of the employee, because the employee usually gets paid
by the hour and the employer cannot blame the employee for
bad consequences of the rule when this rule was enforced by
the employer.

I myself usually follow this rule, because it allows blocks
of code to be extracted into methods or from methods more
easily, and it might make complex functions more easy to
read and to modify. For exameple, if one wanted to add a
statement that is always executed directly before the end of
the function, one needs to only add it once: directly in
front of the single exit of the function. I would not apply
this rule, however, when optimizing and the code with
multiple returns is faster.

 

[Keith H Duggar]

I was probably too strong there, I should have said "By inspection
maybe".  Checking preconditions and retuning early can make the code
easier to inspect and verify.

Let's bring some concreteness to the debate. Give me an example
from your /real world/ code of a function having at least several
interesting lines and multiple returns that you think is "better"
for having multiple returns. Then I will tell you if it can be
written better as single return and if so I will show you how
to write it.

And please, don't trot out the same tired old "toy" constructs
from some hypothetical parser etc crap that have been debated time
and time again. Let's see something /real and new/ this time. If
you are such a fan of ME you must have multiple interesting
examples from your actual work.

KHD

 

[Keith H Duggar]

     [...]

RAII has everything to do with this rule as RAII makes old fashioned
SESE redundant.

How?  SESE has nothing to do with managing resources: it's
a rule to simplify analysis of program logic, in order to ensure
program correctness.

SESE is commonly used to ensure any resources allocated in the function
are deallocated in one place; it is an easy mistake to omit freeing a
resource if a function has many returns; however, RAII ensures that
resources are freed irrespective of number of returns or if an exception
is thrown.

SESE is commonly used to ensure cleanup precisely /because it
simplifies program logic/. SESE is about /code simplification/
and that has value far and beyond anyone particular goal such
as cleanup.

RAII makes SESE redundant and SEME common in modern C++ designs.

RAII makes it partially redundant /for cleanup/. But not for the
many other improvements that come from code simplification.

Anyhow, what matters above all else is simple (to a human) code.
Sometimes SEME delivers the simplest most understandable code.
However, most of the time SESE delivers the simplest code. This
becomes ever more true the better factored the code becomes.

KHD

 

[Stefan Ram]

Since your employer is paying you to write code, you should write it
the way they want it written.

Yes. But some employees have problems, when individual
requirements seem to contradict each other.

For example, when the employer has the high-level
requirement of well-maintainable code and then a collection
of low-level requirements as a style guide, which might
forbid some coding patterns that are needed to fulfill the
high-level requirement. So the employee cannot fulfill both
the high and the low level requirements at the same time.

 

[Ian Collins]

Let's bring some concreteness to the debate. Give me an example
from your /real world/ code of a function having at least several
interesting lines and multiple returns that you think is "better"
for having multiple returns.

I'm not really fussed one way or the other. All I am arguing against is
the dogmatic position that single return is better. In my own code I
use what I consider best fits the problem.

Anyway, here's one where I used multiple returns:

const Entries&
Ldif::getEntriesByOu( const std::string& ou ) const
{
if( ou == "all" )
{
return entries;
}

EntriesByName::const_iterator i = entriesByOu.find( ou );

if( i != entriesByOu.end() )
{
return i->second;
}
else
{
throw std::runtime_error( ou+": entries not present" );
}
}

 

[Keith H Duggar]

First random choice of a *real world* function of mine:

plugins::entry_list::iterator plugins::find(const plugin& aEntry)
{
        lib::lock lock(*this);
        for (entry_list::iterator i = iEntries.begin(); i != iEntries.end(); ++i)
                if (i->path() == aEntry.path())
                        return i;
        return iEntries.end();

}

/Leigh

Guess you missed the "don't trot out the same tired old toys" request.
Really, you consider a find implementation new and interesting?

Anyhow, your example is fine as either SEME or SESE. I certainly would
not reject it in a review (though I wonder question a design that does
not allow easy use of STL find). That said I would have coded the loop
the same way some STL implementations do:

plugins::entry_list::iterator plugins::find(const plugin& aEntry)
{
lib::lock lock(*this) ;
entry_list::iterator i = iEntries.begin() ;
entry_list::iterator const e = iEntries.end() ;
while ( i != e && i->path() != aEntry.path() ) ++i ;
return i ;
}

Which by the way is likely to be faster than your version (because of
the cached end iterator and removal of the unlikely event == from the
branch condition replacing it with the more likely event !=).

KHD

 

[Ian Collins]

Guess you missed the "don't trot out the same tired old toys" request.
Really, you consider a find implementation new and interesting?

Just because a function is short, doesn't make it a toy.

Anyhow, your example is fine as either SEME or SESE. I certainly would
not reject it in a review (though I wonder question a design that does
not allow easy use of STL find). That said I would have coded the loop
the same way some STL implementations do:

plugins::entry_list::iterator plugins::find(const plugin& aEntry)
{
lib::lock lock(*this) ;
entry_list::iterator i = iEntries.begin() ;
entry_list::iterator const e = iEntries.end() ;
while ( i != e&& i->path() != aEntry.path() ) ++i ;
return i ;
}

Do you really find that clearer? At least with Leigh's version it is
immediately clear to the reader what gets returned if a match isn't found.

Which by the way is likely to be faster than your version (because of
the cached end iterator and removal of the unlikely event == from the
branch condition replacing it with the more likely event !=).

end() would very likely be optimised to a constant.

 

[Richard]

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

(e-mail address removed)-berlin.de (Stefan Ram) spake the secret code

Yes. But some employees have problems, when individual
requirements seem to contradict each other.

This situation is easily resolved by asking your employer which one is
more important.

 

[Ian Collins]

I think there was an earlier post in this thread that was a fairly good
example of when to use multiple returns as well.

That said, and without specifically calling out the above as poor in any
way, and without fully understanding what this function is supposed to
do conceptually. I likely wouldn't have written it that way. I would
likely have written something like:

// The Assert function is from TC++PL section 24.3.7.2
class MissingEntry : public std::exception { };

const Entries&
Ldif::getEntriesByOu(const std::string& ou) const
{
EntriesByName::const_iterator i = entriesByOu.find(ou);
Assert<MissingEntry>(ou == "all" || i != entriesByOu.end());

return ou == "all" ? entries : i;
}

I grant that this version requires the find to be called every time,
even if ou == "all", and this version uses the conditional operator
which is hated by many, but there you go.

The "all" case is fairly common, hence the early check and return.

One problem I have with the above function as a concept is that it
requires the calling function(s) to keep track of, or otherwise know
what is contained in, entriesByOu (unless it always sends "all" to the
function.) This seems like an inappropriate distribution of
responsibilities to me. But maybe the contents of entriesByOu is
embodied in some program wide invariant, or maybe this function is
private within the class.

The this application, entriesByOu is built when the input is parsed at
startup, so it effectively is a program wide invariant. The containing
class represents all of the entries in a LDAP database in an application
that transforms the data, so it's role is both well known and well defined.

 

[Öö Tiib]

C is a low-level procedural language, yes.  How it deals with
low-level resource management is up to the programmer, however.
And the SESE structured programming idiom has very little to do
with it, having been developed for entirely different reasons.

Yes SESE and other structured programming idioms were developed to get
rid of wild goto spaghetti in multi-thousand-lines functions. When did
you last see such code passing review anyway? So these reasons are not
that actual anymore. The only reason left for SESE in C is to ease the
cleanup phase of resource management. Are there other reasons left?

[...]

And?  What does that have to do with the original question?  Or
OO?

Resources are therefore always bound and handled as objects/parts of
objects. Not something external managed outside of objects manually.
So my impression is that RAII means that C++ style OOP is used for
resource management.

 

[Öö Tiib]

Modules produce objects which have invariants that are maintained by the
code in the module. Don't get confused here, conceptually a "class"
invariant is no different than a "struct" invariant or an invariant that
is guaranteed on an opaque or other user defined type. Such invariants
have been around for a long time. Let's not wrap *everything* under the
OO banner please.

You are right there that you can do OOP in any language. In C (for
example) you can use structs as classes. You can use forward declared
structs as abstract interfaces and information hiding. You can use
function pointers for dynamic dispatch and polymorphism.

However ... quite some things you have to do manually and if you try
to be too clever there then there is danger that it stops being simple
and robust and easy to understand.

 

[James Kanze]

SESE is commonly used to ensure any resources allocated in the function
are deallocated in one place; it is an easy mistake to omit freeing a
resource if a function has many returns; however, RAII ensures that
resources are freed irrespective of number of returns or if an exception
is thrown. RAII makes SESE redundant and SEME common in modern C++ designs.

That may be one use of SESE, but it is not the initial, nor the
principle motivation behind SESE. SESE was designed to make
reasoning about program correctness easier: things like
maintaining loop invariants or guaranteeing post-conditions, for
example.

 

[hanukas]

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

SESE may help making the compiled machine code smaller. Example;

const char* getError(GLenum error)
{
switch (error)
{
case GL_BLA: return "bla";
case GL_FOO: return "foo";
// ... etc ...
}
// ... etc ...
}

YMMV, but often compilers replicate the return statement as many times
as you have case statements there. This will bloat the binary a lot.
There is a good mechanism to overcome that:

const char* v;
switch (error)
{
case GL_BLA: v = "bla"; break;
// ... etc.
default: v = "unknown error"; break;
}
return v;

Also use ## in a nice macro to save some typing when converting error
into string. ;-)

This was just one example that came to mind (probably because this is
a common case). Multiple returns are more often less clear than single
return, but doesn't mean it isn't true from time to time.

Rules like that are a good thing when you have junior staff members.
They will eventually learn what time it is and make good design calls
based on experience; when you have a good reason to have many exits,
by all means do it.

 

[Michael Doubez]

Yes SESE and other structured programming idioms were developed to get
rid of wild goto spaghetti in multi-thousand-lines functions. When did
you last see such code passing review anyway?

If there is a lack of coding review, SESE can be easily checked by a
static anaysis tool.

So these reasons are not
that actual anymore. The only reason left for SESE in C is to ease the
cleanup phase of resource management. Are there other reasons left?

I am not a proponent of SESE rule but there are some case where SEME
can be a hell:

if( case1 )
{
if( casea ) return resulta;
}
else
{
if( caseb ) return resultb;
if( casec ) return resultc;
return resultd;
}
return resultx;

At least with SESE, the logic of the function has (more or less) only
one representation.

 

[James Kanze]

On 12/ 7/10 05:27 AM, James Kanze wrote:

Isn't that obvious? If a function only has one return, it's tempting to
perform clean up before the return. If a function is written to be
exception or SEME safe, resources allocated have to be automatically freed.

Or freed at both return points. Or all of the return points
except one are before any resources are allocated. Or (by far
the most frequence case in my current work) the only "resources"
used are on stack variables.

SESE is sufficient to ensure that all resources are freed, but
it's hardly necessary. It is necessary for most of the more
mechanical means of reasoning about program correction, and it
does make reasoning about it simpler.

By inspection yes, by testing, no.

Ideally, you need both. Testing can never prove that code is
correct, only that it is incorrect. And a lot of things simply
aren't testable. But a human can overlook things as well, so
testing serves as a guardrail.

Because exception safe code is also SEME safe code.

That's the first time I've heard the expression "SEME safe
code". We speak of exception safety because exceptions
introduce invisible edges in the flow graph; we also only use
exceptions in cases where we've abandonned trying to achieve
a significant part of the normal post-conditions. Neither is
necessarily true with SEME.

I think I understand what you're trying to say, but I think your
confounding two separate issues.

 

[James Kanze]

[...]

Fair enough, James is becoming increasingly dogmatic, which is goading
me into responding.

Where am I becoming dogmatic? I actually posted an example of
a case where I do use multiple returns. And there are certainly
more important issues than multiple returns when it comes to
understanding code---I'd rather deal with multiple returns in
a ten line function with only only level of control nesting than
a 100 line function with 5 levels of control nesting, even if it
only has a single return. (Interestingly, my experience
suggests that the opposite is more common. People who write
short, easy to understand functions often---but not always---use
SESE. People who aren't bothered with loops spanning a hundred
lines, with five or more levels of nesting, generally aren't
bothered by throwing a return in somewhere in the middle of the
mess either.)

About the only statement I've made that could be considered
"dogmatic" is that SESE makes reasoning about program
correctness easier. This is a simple fact: it may be that it
doesn't have to be that way (and some more reason work has
tended to attenuate it somewhat), but all of the standard
literature on the subject describes how to reason about program
correctness based on a few standard flow control patterns, all
of which are SESE. If the function is simple enough, however,
the difference shouldn't be too significant for a human reader
(and there may be other reasons arguing against SESE); if it
isn't, the function is too complicated anyway, and needs to be
refactored. If various automated proofs are being used,
however, the difference may be significant; the proof engine may
not be able to handle anything but SESE.

I was probably too strong there, I should have said "By inspection
maybe". Checking preconditions and retuning early can make the code
easier to inspect and verify.

Which may, in some cases, be a valid reason for violating SESE.

Strictly speaking, any time you throw an exception, or have an
assertion failure, you violate SESE. Practically speaking, in
such cases, you're concerned with a much weaker set of
post-conditions---in the case of an exception, they still have
to be validated, but the usual validate procedure is simply to
ensure that an exception cannot occur when the "exceptional"
post-conditions are not met (e.g. an object has an inconsistent
internal state). This is not always trivial, but it does
require a different sort of reasoning than that used when
dealing with post-conditions along the lines of "the return
value will be accurate to one half LSB", for example. (The
latter, of course, is a good example of something that cannot be
tested in reasonable time. And which is also often very, very
difficult to prove by analysis as well.)

 

[James Kanze]

On 07/12/2010 00:55, Keith H Duggar wrote:

[...]

First random choice of a *real world* function of mine:

plugins::entry_list::iterator plugins::find(const plugin& aEntry)
{
lib::lock lock(*this);
for (entry_list::iterator i = iEntries.begin(); i != iEntries.end(); ++i)
if (i->path() == aEntry.path())
return i;
return iEntries.end();
}

That's nothing but a linear search. What's wrong with the
canonical form:

plugins::entry_list::iterator plugins::find(const plugin& aEntry)
{
lib::lock lock(*this);
entry_list::iterator i = iEntries.begin();
while (i != iEntries.end() && i->path() != aEntry.path())
++ i;
return i;
}

It's simpler, and since it is in the canonical form for a linear
search, anyone looking at it recognizes it as a linear search
immediately, and since it is easily recognized as a standard
algorithm in a standard form, its correctness is also
immediately recognized. And if for some reason, you didn't want
to trust it, it's easy to determine the loop invariant (since it
is just the condition of the while), to prove that the loop must
terminate (assuming the pre-condition that starting from
iEntries.begin() and advancing will eventually reach
iEntries.end()), and that results will be correct.

 

[James Kanze]

That's more or less my position as well. I might also question
why the added complexity? Why not use the canonical form?

Do you really find that clearer?

If he'd put the statement controled by the while on a separate
line, it would be significantly clearer. If for no other reason
that it makes the loop invariant explicit.

At least with Leigh's version it is immediately clear to the
reader what gets returned if a match isn't found.

At least with Leigh's version, you have to do a little bit of
additional analysis to prove progress in the loop, and to prove
that the loop actually terminates. For such simple cases, the
difference is marginal (unless, as I mentionned elsewhere,
you've automated some of the verification).

end() would very likely be optimised to a constant.

Not according to the measurements I've made. But IMHO, that's
an irrelevant point: I'd write "while ( i != iEntries.end()
...." myself. (Until the profiler said I had to change it, of
course.)