attack of silly coding standard?

[Ian Collins]

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.

Or maybe I'm not expressing my self clearly.

I have been working with a lot of filesystem C code with functions that
allocate stuff as they go and when they hit an error and can't progress
"goto cleanup".

Obviously being C the code isn't exception safe and would certainly leak
like a sieve if anyone were to add an early return. If I had to convert
the code to exception safe C++, the first thing I would do is wrap the
allocated resources in an auto_ptr like object to ensure no mess was
left behind if an exception was thrown.

Having done that, I could probably also replace most of the "goto
cleanup" lines with a return of the appropriate error code.

So what I'm saying is code written to be exception safe is generally
early return safe. Yes there are cases where certain post conditions
require SESE, but they are probably uncommon.

I also accept your point regarding machine reasoning of program
correctness, but I don't think many projects have access to requirements
to use such tools. If they did, we wouldn't have software bugs, would we

 

[James Kanze]

On 12/ 7/10 01:55 PM, Keith H Duggar wrote:

[...]

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" );
}
}

I'd accept that in a code review, but that's not the way I'd
write it. I'd write something more along the lines of:

Entries const&
Ldif::getOneEntryByOu( std::string const& ou ) const
{
EntriesByName::const_iterator result = entriesByOu.find( ou );
if ( result == entriesByOu.end() )
throw std::runtime_error( ou+": entries not present" );
return result;
}

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

In the case where ou is equal to "all", you return something
different---you're doing something different---so it seems more
logical to me to handle the two cases separately (in separate
functions, except that in the case where ou is "all", the
separate "function" is so trivial that it can be written in
line).

 

[James Kanze]

On 12/ 7/10 10:42 PM, James Kanze wrote:

[...]

Or maybe I'm not expressing my self clearly.

I have been working with a lot of filesystem C code with functions that
allocate stuff as they go and when they hit an error and can't progress
"goto cleanup".

I don't think any structured program advocate would consider
goto SESE. I think that the rule about a single return only
makes sense in the larger sense of SESE. (And in some special
cases, the use of goto to ensure the execution of clean up code,
in C, might be justified. Although in all of the cases I've
actually seen, the solution involved breaking the function down
into smaller parts, each of which naturally finished at the
end.)

Obviously being C the code isn't exception safe and would certainly leak
like a sieve if anyone were to add an early return. If I had to convert
the code to exception safe C++, the first thing I would do is wrap the
allocated resources in an auto_ptr like object to ensure no mess was
left behind if an exception was thrown.

Certainly. But that has nothing to do with SESE. Using SESE
doesn't mean that you shouldn't use RAII. (Note too that I'm
only advocating SESE for the non-exceptional branch. I do make
use of exceptions, and long before exceptions, I've made
extensive use of assert---which results in a rather brusk return
from the function when it fails, and is certainly not SESE.)

Having done that, I could probably also replace most of the "goto
cleanup" lines with a return of the appropriate error code.

So what I'm saying is code written to be exception safe is generally
early return safe. Yes there are cases where certain post conditions
require SESE, but they are probably uncommon.

The issue isn't whether they require it. The issue is whether
you can more easily verify that the post condition is meant if
the code is written in SESE.

I also accept your point regarding machine reasoning of
program correctness, but I don't think many projects have
access to requirements to use such tools. If they did, we
wouldn't have software bugs, would we

I think the main reason that such tools aren't widely used (at
least where it is important that the program be correct) is that
you still have to tell the tool what the code is supposed to do.
In a formal way, very much like writing a program. And of
course, with the same risk of error as you have when writing a
program. (They are used on some very critical systems. With
the program specifications for the tool being written by a
different team, whose errors hopefully will not overlap with
those of the programming team. But having two complete teams
write what is basically the same program, in different
languages, is fairly expensive.)

 

[James Kanze]

On 06/12/2010 16:33, James Kanze wrote:

On 05/12/2010 14:08, Daniel T. wrote:

[...]

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. RAII makes SESE redundant and SEME common in modern C++
designs.

SESE is commonly used in functions which don't allocate any
resources as well. That the resources are freed can be
considered as just one more post-condition that has to be
maintained. In the code I work on, we do have other
post-conditions (that the return value is correct, for example),
and SESE helps make them more easily verifiable.

It is a bullshit position to say that constructors and destructors
(which RAII utilizes) has nothing to do with *C++* OOP.

Then I guess people like Booch are just spouting bullshit. I'd
suggest that you learn what OO means before trying to explain it
to other people.

 

[James Kanze]

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?

Yesterday. (OK, it wasn't multi-thousand lines. But there
was a loop spanning 150 lines.)

Without going to the extreme of thousand line functions, Ian
posted an example here which really should have been two
functions. An example which probably would have passed review
as well.

So these reasons are not that actual anymore.

It is certain that the importance of SESE goes down when the
functions become smaller and more succinct. But given the
choice of writing something in a way very easy to prove correct,
or in a way slightly less easy (even if still not difficult),
why not chose very easy.

The only reason
left for SESE in C is to ease the cleanup phase of resource
management. Are there other reasons left?

The initial reason: reasoning about program correctness.

[...]

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.

OO generally implies dynamic polymorphism. At least according
to the classical texts (Booch, etc.).

 

[James Kanze]

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

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.

I think the main argument for introducing SESE as a rule is that
it is easily verified. There are clearly cases where mutliple
exits make code completely unreadable, and it's difficult to
formulate rules which distinguish between them, and cases where
multiple exits are acceptable. If there is a good code review
procedure, and a fairly broad consensus concerning when the
multiple exits are acceptable, and when they aren't, the rule
probably isn't necessary. Otherwise, it might be useful, if
only to prevent blatant abuse. (Whether a function has more
than one exit is something that can't be argued---it either has,
or it hasn't.)

In general, I prefer more flexible rules, and count on the
programmers (and reviewers) to enforce "readability" (which
can't be easily defined). And if functions are short enough and
concise enough, I don't think that SESE, while beneficial, is
important enough to warrent a rule. And if they aren't, SESE
isn't a silver bullet which will make them readable; the real
advantage of SESE as a rule may be that it makes the awkwardness
of overly complex functions more visible.

 

[James Kanze]

For the third and final time Mr Troll:

I see that you've run out of arguments again, and have reverted
to name calling.

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.

You're constant repeating it doesn't make it true. Try reading
any of the authors who recommend SESE, starting with Dijkstra.
Typically, they don't mention resource allocation.

It is a bullshit position to say that constructors and destructors
(which RAII utilizes) has nothing to do with *C++* OOP.

It's the position of the authors who defined OO. Try reading
Booch, for example. OO (regardless of the language) implies
runtime polymorphism. Otherwise, it's not OO.

 

[James Kanze]

On 06/12/2010 17:07, peter koch wrote:

[...]

I (and others) totally disagree that SESE makes functions easier to
read; SESE can also make functions harder to read.

I've yet to see a case where it made the function harder to
read, but that was not my argument. My argument was that SESE
makes it easier to reason about program correctness. Something
that can easily be verified by reading the literature concerning
program correctness.

[...]

We are dicussing OOP in the context of C++; constructors and destructors
are part of C++ OOP.

Could you post some references which would indicate that anyone
other than you is of that opinion? Are you familiar, for
example, with the works of Booch (who makes it clear that OO
requires dynamic polymorphism)?

Mr Kanze has a track record of troll like replies including "a
function should contain no more than 10 lines of code" for
example.

In other words, anyone who doesn't agree with your unsupported
options is a troll. Ad hominum when you don't have any real
arguments.

 

[James Kanze]

On 12/ 7/10 06:10 AM, James Kanze wrote:

That's an incredibly pompous and condescending comment James.

Sort of. I just adapted my style to that of the posting I was
responding to. Apply it to the statement immediately above, and
it makes perfect sense.

You are
starting to come across like a religious extremest; your way is the one
true way and everyone else is a cowboy.

I'm trying to say that the rule "return as quickly/early as
possible" is not a good rule. It ignores too many other
important issues. Not everyone else is a cowboy, but there are
some cowboys out there.

Do you really claim the programmer who writes

{
if( !precondition )
return error;

// do stuff
}

cares less about program correctness (or readability) than the
programmer who writes

{
if( precondition )
{
// do stuff
}
else
return error;
}

?

No, and I didn't say I did. I said that someone who
systematically applies the rule, return as quicky/early as
possible, doesn't care about program correctness. Such simple
rules don't cut it.

 

[Keith H Duggar]

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.

Ok cool. We agree on that. Perhaps one point where we differ is
I find "keeping an eye out" for ways to structure as SESE helps
one envision better code factoring and code simplification.

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" );
     }
   }

Interesting example. First, one notices there is a quite generic
and common "try to find and throw if not found" block screaming
"refactor me". Indeed below is just such a function from my code
library (I've posted other members from this family before here,
search for "getOrMake"):

template < class K, class V, class C, class A, class F >
V const &
getOrFail (
std::map<K,V,C,A> const & m
, K const & k
, F const & f
) {
typename std::map<K,V,C,A>::const_iterator i = m.find(k) ;
return i != m.end() ? i->second : f(m,k) ;
}

So for me the above would have been written as:

namespace { //private error handling
Entries const &
notFound (
EntriesByName const &
, std::string const & ou
) {
throw std::runtime_error( ou+": entries not present" );
}
}

Entries const &
Ldif::getEntriesByOu (
std::string const & ou
) const {
return ou == "all" ? entries
: getOrFail(entriesByOu, ou, notFound) ;
}

The separate error handler function idiom (notFound in this case)
also allows for more sophisticated handling without complexifying
the (persumably) normal found path.

However, if one doesn't have or want such library functions or if
one doesn't like error handler functions, etc then Jame's design
elsethread that refactors a separate getOneEntryByOu is superior.

Either way, this is an example of my point that as code improves
specifically as it becomes better factored, SEME becomes less and
less tempting and SESE more and more obvious.

KHD

 

[Bart van Ingen Schenau]

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.

By that characterization, I postulate that this program is also
written using the OOP paradigm:

<code>
#include <iostream>
#include <iterator>
#include <algorithm>
#include <list>
using namespace std;

int main() {
list<int> data((istream_iterator<int>(cin)),
istream_iterator<int>());

data.sort();
copy(data.begin(), data.end(), ostream_iterator<int>(cout, ","));
}
</code>

The code clearly uses objects and RAII to manage resources.

Bart v Ingen Schenau

 

[Keith H Duggar]

It is not a "toy"; it is a real function in real working code.

Umm ... it's also the same old linear find that has been discussed
many many times. It has no interesting /different/ features such as
the "all" special case and exception in Ian's example. That's what
I'm trying to say. In other words, I was hoping for something fresh
instead of ye olde linear find. But thank you nonetheless sharing
an example from your actual code. I appreciate it.

You think that is more readable than my version?  I don't think so; it
is typical of the fuglyness one would expect in the entire codebase of a
company which enforced a SESE rule.

The loop is /the canonical/ linear find loop as you will find it
in too many places to count (give or take a newline and fences).
As for the declarations of the iterators above the loop, that is
just my own preference and has nothing whatsoever to do with the
SESE vs SEME issues.

Have you never heard the phrase involving "premature optimization"?
There will only ever be a few plugins so this function would never be a
bottleneck (and is only ever called when adding/removing plugins).  It
seems that you are the one with the head stuck in the non-real world clouds.

Have you ever learned the meaning of the phrase "by the way"? Or
the concept of an "aside" or parenthetical comment or remark? Ok
I guess not. Hint I'm not saying optimization is /the reason/ to
write the code this way, it just happens to be an accidental and
additional benefit.

KHD

 

[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.

Whilst the for loop / while loop transformation is well known (I often
do it myself) it does not mean that one should eschew for loops.

Certainly. I use for loops myself a lot. In this case, I don't
see any real difference between my while and:

for ( ; i != iEntries.end()&& i->path() != aEntry.path(); ++ i) {
}

Which ever seems more natural or more appropriate is purely a
matter of taste.

You
attach too much value to the /placement/ of pre/post-conditions and too
much value on having a single loop invariant simply because you have
this irrational fear of if statements and return statements.

I don't have any irrational fear of if or return. I just prefer
to use them when appropriate. I do have a fear of code that I'm
not certain is correct. For a short bit of code like this, of
course, all of the posted forms are fairly readable: the form I
posted (with the while) has the advantage of being the canonical
form: the way linear search was first presented and analysed
(and proved) in literature. I would expect most programmers to
immediately recognize it as such.

My version
is just as readable as your version; I would argue my version is
slightly simpler to understand as the returns make it explicit as to the
two possible results of the function: an iterator from the sequence
range or end(); this is less obvious in your version.

The issue of readability is arguable. I can understand your
point of view. The issue of verifiability is less arguable,
since the classical tools (logical or automatic) don't handle
multiple exits as well. And in this particular case, the fact
that it is a classical algorithm with a canonical form argues
strongly for using that canonical form, even for reasons of
readability.

And as far as your version being the canonical well recognized form of
linear search here is Microsoft's std::find implementation:

Microsoft does not define the canonical forms. The standard
literature where the algorithm was present *and* proved does.
And in small functions like these, it's not that important. (I
don't know the full context in which the two functions you cite
were written, so I don't know what considerations entered into
the choice of how they were formatted. I do know that---as I've
stated elsewhere---if the function is small enough, it's not an
important issue.)

 

[James Kanze]

I know what OO means; I know what it /traditionally/ means: inheritance,
encapsulation, polymorphism; I also know what it means in the real C++
world: establishing, maintaining and tearing down a class invariant
through the use of constructors (and RAII), correct public interface and
destructors.

Yes. I did some searching on the network, and there does seem
to be a community where OO simply means using class instead of
struct. This is not the traditional use, however, and not the
use I'm used to hearing in the professional world. (Maybe
because a candidate who can't make the distinction between
paradigms like OO, compile time generics, and simple
encapsulation doesn't get hired.)

In the end, it's a question of vocabulary: judging from what
I've seen (in an admittedly very quick scan of the network),
people who understand the traditional meaning use OO with that
meaning.

One could argue that maintaining an class invariant is
covered by "encapsulation" and that would be a fair point but you very
rarely make fair points; you quite often make trollish points.

Is this your only troll of the day or are there more to come? Pathetic.

And there you go back into your ad hominum attacks. When you
try, you can do better. (FWIW: "encapsulation" is a very old
idea, and not unique to OO. And the notion of "invariant" as
well.)

 

[Keith H Duggar]

Have you ever learned the meaning of the phrase "by the way"? Or
the concept of an "aside" or parenthetical comment or remark? Ok
I guess not. Hint I'm not saying optimization is /the reason/ to
write the code this way, it just happens to be an accidental and
additional benefit.


Bullshit; there is more than one way of skinning a lemming, see the
Microsoft examples I posted else-thread.

Ironically (given your heritage) you are having great difficulty
today understanding English. You seem ignorant of the meaning of
the word "canonical" just as you were of the phrase "by the way".

Only in your warped and isolated Microsoft fanboy world does
Microsoft == canonical (for algorithms C++ or otherwise). The
very point of the word (in a computer science or mathematics
context) is to distinguish one among many alternative forms.

KHD

 

[Ian Collins]

I'm trying to say that the rule "return as quickly/early as
possible" is not a good rule. It ignores too many other
important issues. Not everyone else is a cowboy, but there are
some cowboys out there.







No, and I didn't say I did. I said that someone who
systematically applies the rule, return as quicky/early as
possible, doesn't care about program correctness. Such simple
rules don't cut it.

What if I changed it to "check preconditions and return early"?.

One very successful team I managed adopted that rule (the team did, I
didn't tell them to do it). We had to care about program correctness,
while the software wasn't directly safety critical, failure could bring
down a telephone network.

 

[James Kanze]

What if I changed it to "check preconditions and return early"?.

That's a completely different issue. I've already expressed the
opinion that this is one more or less acceptable use of
premature returns. (Although if it's really a precondition, I'd
expect assert, with an abort instead of a return if it fails.)
There are others.

"Return as quickly/early as possible", on the other hand,
implies that the author would return from the middle of a nested
if or while, deep in the program logic. Something that makes
analysis of the various preconditions, invariants, etc. almost
impossible.

One very successful team I managed adopted that rule (the team did, I
didn't tell them to do it). We had to care about program correctness,
while the software wasn't directly safety critical, failure could bring
down a telephone network.

Was that before or after exceptions became generally available?
And did it really concern preconditions---in the telephone
systems I've worked on, preconditions were normally verified
using assert, so that in case of a software error, the system
stopped as quickly as possible. (Such systems normally have a
hot backup, and if there's a problem in your system, you want it
to take over as quickly as possible.)

 

[Ian Collins]

That's a completely different issue. I've already expressed the
opinion that this is one more or less acceptable use of
premature returns. (Although if it's really a precondition, I'd
expect assert, with an abort instead of a return if it fails.)
There are others.

"Return as quickly/early as possible", on the other hand,
implies that the author would return from the middle of a nested
if or while, deep in the program logic. Something that makes
analysis of the various preconditions, invariants, etc. almost
impossible.

I think we agree on something at last!

Was that before or after exceptions became generally available?

After, but the compiler we used made such a dog's breakfast of
optimising code with exceptions we had to disable them.

And did it really concern preconditions---in the telephone
systems I've worked on, preconditions were normally verified
using assert, so that in case of a software error, the system
stopped as quickly as possible.

I guess you could call them soft preconditions, like "is it OK for me to
run now?".

 

[Öö Tiib]

Yesterday.  (OK, it wasn't multi-thousand lines.  But there
was a loop spanning 150 lines.)

Without going to the extreme of thousand line functions, Ian
posted an example here which really should have been two
functions.  An example which probably would have passed review
as well.

Reviewer just tells when he does not understand how or why something
works or raises some false alarm. That is best indicator that code is
not simple enough: other person has difficulties to parse it.

It is certain that the importance of SESE goes down when the
functions become smaller and more succinct.  But given the
choice of writing something in a way very easy to prove correct,
or in a way slightly less easy (even if still not difficult),
why not chose very easy.


The initial reason: reasoning about program correctness.

One usual case is when processing dirty (taken from external input)
data. The data can be usually broken in multiple ways. So it is if A
can be extracted, extract A, if A is good and B can be extracted
extract B, etc. High mountain of ifs with SESE. If the data is usually
expected to be valid then refusing and throwing can be better. If
valid parts have to be kept anyway then early return can be better. It
is perhaps understandable for reviewer but not for static analysis
tool.

[...]

With C++ most of us expect RAII (OOP) idiom to be used to let
destructors to deal with releasing resources instead of
explicit resource management of C. That is most robust and
simple way.
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.

OO generally implies dynamic polymorphism.  At least according
to the classical texts (Booch, etc.).

Yes, but I don't see an issue here ... RAII does not contradict with
dynamic polymorphism AFAIK. On the contrary, it uses even that concept
of OOP. For example instead of Windows HANDLE (that is void* related
to some OS resource) you can use shared_ptr<void> with dynamically
attached deleter for that particular handle.

 

[Öö Tiib]

By that characterization, I postulate that this program is also
written using the OOP paradigm:

<code>
#include <iostream>
#include <iterator>
#include <algorithm>
#include <list>
using namespace std;

int main() {
  list<int> data((istream_iterator<int>(cin)),
istream_iterator<int>());

  data.sort();
  copy(data.begin(), data.end(), ostream_iterator<int>(cout, ","));}

</code>

The code clearly uses objects and RAII to manage resources.

Perhaps misunderstanding. I claimed that RAII uses OOP to manage
resources not that OOP must use RAII nor that RAII is OOP. RAII is
idiom that fits with OOP. However if you have C++ then you may use
that RAII anyway even if what you do is processing ints from stdin
into ints going to stdout.