Sequential Stupidity



  • Several years ago, this programmer wrote a utility library which took a count field of a structure and used it to iterate over a list. More recently, another programmer wrote an unrelated db method in the class that used the handy list-size variable to hold a db-generated sequence counter (comments mine):

    public class Data {
       private long n;             // number of items in: list
       private List<stuff> list;   // initialized to a list of two items elsewhere
    

    public boolean utilFunction() {
    boolean result = false;
    for (int i=0; i<n; i++) {
    if (list.get(0).compareTo(constantObjectA) == 0) result = true;
    if (list.get(1).compareTo(constantObjectB) == 0) result = true;
    }
    return result;
    }

    public void unrelatedDbFunctionAddedMuchLater() {
    // ...
    n = <get sequence value from db>;
    // ...
    }
    }


    Forgetting the stupid iteration over the list when the indices being tested are hard wired, and forgetting that one could just return-true instead of using the temp result variable, the subsequent code that replaced the list-length with a sequence value replaced a value of 2 with a value that, over time, grew in our production db to 3 billion!

    And of course, this function is called from multiple places. And javac optimization is intentionally turned off.

    And they wondered why the application was taking longer and longer to complete!



  •  You know... recently, I dabbled with Hibernate for the first time, and I'm trying my hardest to do things the proper way. So how do sequences and stored procedures work from Hibernate? Ah... that way.

    Sometimes, though, I wonder why I bother. Professional pride maybe? But with so many inadequate people calling themselves programmers about, how does one stand out?



  • A for loop that is unnecessary, coupled with a DB retrieval that is unnecessary, coupled with a "programmer" whose job should be made redundant. WHY are optimisations disabled? What happens if utilFunction() is called before unrelatedDbFunctionAddedMuchLater()?



  • @The_Assimilator said:

    A for loop that is unnecessary, coupled with a DB retrieval that is unnecessary, coupled with a "programmer" whose job should be made redundant.

    You missed a length field which is unnecessary. This looks like Java, and java.util.List has a size() method.



  • @snoofle said:

    forgetting that one could just return-true instead of using the temp result variable
     

    And violate the "Single Entry, Single Exit" principle? Blasphemy!

    (seriously, people should stop hammering this "principle" in students' heads)



  • @Medinoc said:

    @snoofle said:
    forgetting that one could just return-true instead of using the temp result variable
     

    And violate the "Single Entry, Single Exit" principle? Blasphemy!

    (seriously, people should stop hammering this "principle" in students' heads)

    Aye, I have had several functions where there was a special case that I want to exit from very early in the function, that way I would not have to encapsulate almost the entire function in a giant if statement.  Then if I had multiple points in the process where I can conclude the result and leave early, but still had only one exit point it would cause some very ugly code to be created.



  • The single entry single exit principle is a good idea, it just isn't law.

    Personally I adhere to it in normal circumstances, but reality does get in the way.  If I do have an If statement where one side is basically set an invalid then return a determined result, stick that as the first one and simply exit, nothing wrong with that as long as it is clear.

    I think that really is the point, clear code.  If you have exits everywhere for whatever reason maybe you need to refactor, but if you make the "single exit" priciple a law, you are also doing it wrong.  We are engineers, or we should be, we can make those decision when it is good and bad.



  • @snoofle said:

    Forgetting the stupid iteration over the list when the indices being tested are hard wired...

    I can't. This will haunt me. It makes me want to hurt someone. Big, big hurt. Maim. Cripple. Torment. Kill, kill, kill, KILL, KILL, KILL!!!



  • @The_Assimilator said:

    WHY are optimisations disabled? What happens if utilFunction() is called before unrelatedDbFunctionAddedMuchLater()?
     

    Optimizations are turned off because the person who set up the scripts was originally a C programmer, and in his world, you get a more accurate stack dump with optimizations off. 

    The utilFunction has no bearing on the db call and vice versa. The whole thing could be implemented as:

    return list.get(0).compare(x)==0 || list.get(1).compare(y)==0;

    Eash subsequent call to the db function would simply update the variable (n) with the latest sequence value, which would then control the loop count the next time the utilFunction was called.

    The reason he used n to hold the list size instead of using list.size() was that list.size() would be called each time through the loop, so it's better to calculate it once and use a local variable. With only 2 items ever in the list it's sort of pointless, but it's a good habit to get into so I don't fault him for that.

    The loop was pointless, even without the db function. It was just the stupidity of overwriting an existing variable intended for another purpose with an ever growing value that would just cause things to pointlessly spin re-doing the same calculation countless times for absolutely no reason.

     



  • @KattMan said:

    The single entry single exit principle is a good idea
    Care to elaborate?

    Because I don't think I've ever seen a case where code benefited from it. But I've seen plenty where it didn't (mostly because of nested <font face="courier new,courier">if</font>s).


  • ♿ (Parody)

    @snoofle said:

    The reason he used n to hold the list size instead of using list.size() was that list.size() would be called each time through the loop, so it's better to calculate it once and use a local variable. With only 2 items ever in the list it's sort of pointless, but it's a good habit to get into so I don't fault him for that.

    No, you should still fault him. I don't know how Lists are implemented, but the most likely thing is that it stores the value returned by size() as a member variable, so this dufus is at best duplicating that datum. Given what else you've said about him, I can't believe he even knows what a profiler is, let alone ever fired one up on any of his code.



  • @Zecc said:

    @KattMan said:

    The single entry single exit principle is a good idea
    Care to elaborate?

    Because I don't think I've ever seen a case where code benefited from it. But I've seen plenty where it didn't (mostly because of nested <FONT face="courier new,courier">if</FONT>s).

    I said the principle is a good idea, but also that it should not be a law.  It's just there to keep someone from scattering returns all over thier code for no other reason.  I explained it in my earlier post, also stating that reality gets in the way of making this a law because reality doesn't fit into this neat little box.

    As a principle it is something you teach to guide an idea, as a law it is something you have to follow.

     


  • Trolleybus Mechanic

    @Zecc said:

    Because I don't think I've ever seen a case where code benefited from it. But I've seen plenty where it didn't (mostly because of nested <font face="courier new,courier">if</font>s).
     

    If there's any extra processing, or any clean-up, that needs to be done, a single exit point works because you don't have to coffee-pasta the cleanup code to each Return False statement.

    I've seen this (pseudocode):



    Function DidIFuckUp() as Boolean

    Array RunningErrors;

    SomeResource.Open()

    if (first_fail_condition)
    {
       RunningErrors.Add("You fucked up");
    }

    if (second_fail_condition)
    {
      RunningErrors.Add("Guess who fucked up? You!")
    }

    if (this_fail_condition)
    {
      RunningErrors.Add("Anyone who didn't fuck up, please step forward. Not so fast, $username")
    }

    if (RunningErrors.Count == 0)
    {

       // Do some processing, which may, in turn, cause more errors.
    }

    GlobalErrors.AddRange(RunningErrors);

    SomeResource.Close()

    Return RunningErrors.Count == 0

     End Function

     


     



  • @snoofle said:

    The reason he used n to hold the list size instead of using list.size() was that list.size() would be called each time through the loop, so it's better to calculate it once and use a local variable. With only 2 items ever in the list it's sort of pointless, but it's a good habit to get into so I don't fault him for that.


    If he were actually storing the result of list.size() in a local variable I wouldn't fault him for it.



  • @Lorne Kates said:

    pseudocode

    There's no simple, sane equivalent to that code using multiple returns (3 independent conditions would mean 6 different possible combinations), but I get what you mean and I believe I've done similar to this myself before, in fact.

    Update: never mind, you just return  <font face="courier new,courier">RunningErrors.Count == 0</font>.

    But I'd file this under what I like to call "Don't Put Unconditionals Under Conditions", or maybe even "Don't Repeat Yourself", rather than the single-entry-single-exit principle, but okay.

    So still, I ask: any case where code benefits from the SESE principle alone? This is an honest question, I'm not being argumentative.



  • @KattMan said:

    I said the principle is a good idea, but also that it should not be a law.  It's just there to keep someone from scattering returns all over thier code for no other reason.  I explained it in my earlier post, also stating that reality gets in the way of making this a law because reality doesn't fit into this neat little box.

    As a principle it is something you teach to guide an idea, as a law it is something you have to follow.

    It's not even a good principle though. I dunno, maybe there's some field of programming in which it is...

    Me, I'm building web pages. If a page can't render because of one of the query string params doesn't validate, I want to drop what I'm doing right there and tell the server to return a 500. I don't want to have to write 500 lines of code to "keep track" of what I'm returning, even though I already know it's a 500. I also don't want to enclose the entire code of the page in 26 if() statements. Just so I can have some ridiculous single point of exit.


  • Trolleybus Mechanic

    @Zecc said:

    So still, I ask: any case where code benefits from the SESE principle alone? This is an honest question, I'm not being argumentative.
     

    In it's absolute simpliest form?  No. It would come down 100% to coding style and preference. I'm fine with either:


    rv = false;
    for (i= 1 to 10)
    {
       if i==5
      {

         rv = true;
         i = 10;
       }

       return rv;
    }

     or

    for (i=0 to 10)  if (i==5) return true;
    return false;

     The only ADVANTAGE I can see to the SESE method, is that if you later decide you need a single exit for any of the reasons people need a single exist (like cleanup, multiple paths, etc), then you don't have to worry about an errant "return" statement in the middle of your code.



  • @blakeyrat said:

    @KattMan said:

    I said the principle is a good idea, but also that it should not be a law.  It's just there to keep someone from scattering returns all over thier code for no other reason.  I explained it in my earlier post, also stating that reality gets in the way of making this a law because reality doesn't fit into this neat little box.

    As a principle it is something you teach to guide an idea, as a law it is something you have to follow.

    It's not even a good principle though. I dunno, maybe there's some field of programming in which it is...

    Me, I'm building web pages. If a page can't render because of one of the query string params doesn't validate, I want to drop what I'm doing right there and tell the server to return a 500. I don't want to have to write 500 lines of code to "keep track" of what I'm returning, even though I already know it's a 500. I also don't want to enclose the entire code of the page in 26 if() statements. Just so I can have some ridiculous single point of exit.

    It's a good principle to keep in mind so you don't end up with 500 exits from a single method trying to do everything.

    As I said before A better idea is simply "clear code".  If adhering to the single exit principle makes code unclear and bulky when adding another exit cleans it up then do it, I'km not arguing against that, but when you start getting exits everywhere, it's time to refactor.  I doubt everything can come down to a single exit, but using that as a guidline can simplify your code to a sane level.



  • @Zecc said:

    @Lorne Kates said:

    pseudocode

    There's no simple, sane equivalent to that code using multiple returns (3 independent conditions would mean 6 different possible combinations), but I get what you mean and I believe I've done similar to this myself before, in fact.

    Update: never mind, you just return  <font face="courier new,courier">RunningErrors.Count == 0</font>.

    But I'd file this under what I like to call "Don't Put Unconditionals Under Conditions", or maybe even "Don't Repeat Yourself", rather than the single-entry-single-exit principle, but okay.

    So still, I ask: any case where code benefits from the SESE principle alone? This is an honest question, I'm not being argumentative.

    Single entry should be a no-brainer for you, as modern programming languages enforce it. But would you ever advise someone to use a C longjmp to jump to a function?

    Single exit is also enforced in all modern programming programming languages with one notable exception: Exceptions provide a way for a function to return to a different point as to where it came from, albeit with a very specific purpose and rules. Here's an interesting stack exchange thread discussing it.

    Disregarding pointer tricks and longjmp's you can't break SESE in modern programming languages.



  • @dtech said:

    Single entry should be a no-brainer for you, as modern programming languages enforce it. But would you ever advise someone to use a C longjmp to jump to a function?

    Single exit is also enforced in all modern programming programming languages with one notable exception: Exceptions provide a way for a function to return to a different point as to where it came from, albeit with a very specific purpose and rules. Here's an interesting stack exchange thread discussing it.

    Disregarding pointer tricks and longjmp's you can't break SESE in modern programming languages.

    Thanks.  I was about to post that same link.  The best answer on there is the second one (ranked by votes).   It explains why so many people have trouble with SESE: because it was designed to solve problems that don't exist in modern languages.  Returning early from a function is not a violation of SESE as described by Dijkstra.  Nothing you can do in a well-designed procedural, OO or functional language is a violation of SESE as described by Dijkstra, with the exception of Exceptions.  (No pun intended.)  (And yes, I realize that C longjmp violates SESE.  Note that I specified well-designed languages.)

     



  • @Medinoc said:

    "Single Entry, Single Exit" principle
     

    I hold you accountable for the direction this thread took, you foul instigator.



  • @Lorne Kates said:

    I'm fine with either:


    rv = false;
    for (i= 1 to 10)
    {
       if i==5
      {

         rv = true;
         i = 10;
       }

       return rv;
    }

     or

    for (i=0 to 10)  if (i==5) return true;
    return false;

    I'm not fine with i = 10; instead of break;

    Seriously, you do that? breaking out of the loop won't set i to 10, which could be needed in some infrequent circumstances, but this isn't the case.

    @Lorne Kates said:

    The only ADVANTAGE I can see to the SESE method, is that if you later decide you need a single exit for any of the reasons people need a single exist (like cleanup, multiple paths, etc), then you don't have to worry about an errant "return" statement in the middle of your code.

    This is true and I agree somewhat. I've put some more thought into this and I realize I sometimes subsconciously minimize the number of returns in a function when there's some kind of complex clean up I have to do before returning to the calling code.

    But.. This is the minor exception rather than the rule. Specially when there are modern commodities such as GC'd objects, try..finally blocks, IDisposable, etc.

    And most of the time I'm not really acquiring resources that I can't release immediately after I got the information I need from them. Also, in cases where I do need to do hold on to resources while doing something complex and with a lot of branching, more often than not this complex processing is being abstracted inside other methods anyway.

    Many times there will even be a context / request / transaction / command / whachacallit object, which has been created so I don't have to specifically mention every single resource I need to pass around between all the different functions and subfunctions; and this object will be the one responsible for keeping track of the stuff I've ask for and its disposal when I say I'm done (whatever the outcome).

    Of course, this is my experience. Pretty much all the code I've written or worked with as been client/server, MVC-like stuff, and I don't know what it's like outside of this universe. Your mileage may vary.

    In any case, I was just trying to understand if there was a good reason for following SESE that I might not be aware of.

    @dtech said:

    Here's an interesting stack exchange thread discussing it.
    Thanks for that. Very enlightening.



  • @Lorne Kates said:

    If there's any extra processing, or any clean-up, that needs to be done, a single exit point works because you don't have to coffee-pasta the cleanup code to each Return False statement.

     I often find myself having to use different cleanup depending on how long I got into a function. Then I often end up writing things like

    res1 = AllocateResource1()
    if ( ! res1 ) return E_FAIL
    res2 = AllocateResource2()
    if ( ! res2 )
    {
       Destroy(res1)
       return E_FAIL
    }
    res3 = AllocateResource3()
    if ( ! res3 )
    {
       Destroy(res2)
       Destroy(res1)
       return E_FAIL
    }
    ... do stuff with res1, res2 and res3 ...

    Anyone have better suggestions?


  • :belt_onion:

    @boh said:

     I often find myself having to use different cleanup depending on how long I got into a function. Then I often end up writing things like

    res1 = AllocateResource1()
    if ( ! res1 ) return E_FAIL
    res2 = AllocateResource2()
    if ( ! res2 )
    {
       Destroy(res1)
       return E_FAIL
    }
    res3 = AllocateResource3()
    if ( ! res3 )
    {
       Destroy(res2)
       Destroy(res1)
       return E_FAIL
    }
    ... do stuff with res1, res2 and res3 ...

    Anyone have better suggestions?

    How about

    try {
      res1 = Alloc1();
      if(!res1) 
        throw new ResourceAllocationException();
      res2 = Alloc2();
      if(!res2) 
        throw new ResourceAllocationException();
     
      return E_SUCCESS;
    }
    catch(ResourceAllocationException) {
      if(res1) Destroy(res1);
      if(res2) Destroy(res2);
       return E_FAIL;
    }
     

    And the code can become cleaner if the Alloc1 and Alloc2 functions throw the exception themselves so that every calling function doesn't have to check for null all the time



  • In Hibernate, there are three approaches to call a database store procedure.

    @Severity One said:

     You know... recently, I dabbled with Hibernate for the first time, and I'm trying my hardest to do things the proper way. So how do sequences and stored procedures work from Hibernate? Ah... that way.

    Sometimes, though, I wonder why I bother. Professional pride maybe? But with so many inadequate people calling themselves programmers about, how does one stand out?

    There are not much big different between the three approaches, which method you choose is depend on your personal prefer.



  • @Severity One said:

    But with so many inadequate people calling themselves programmers about, how does one stand out?

    Usually their code appearing on this site indicates they've stood out from the inadequates as exceptionally inept.


  • Trolleybus Mechanic

    @Zecc said:

    I'm not fine with i = 10; instead of break;

    Seriously, you do that? breaking out of the loop won't set i to 10, which could be needed in some infrequent circumstances, but this isn't the case.

     

    Pseudocode.

     The rest of your post makes sense. SESE has some uses, but like everything, only has some uses.

     



  • @blakeyrat said:

    @KattMan said:

    I said the principle is a good idea, but also that it should not be a law.  It's just there to keep someone from scattering returns all over thier code for no other reason.  I explained it in my earlier post, also stating that reality gets in the way of making this a law because reality doesn't fit into this neat little box.

    As a principle it is something you teach to guide an idea, as a law it is something you have to follow.

    It's not even a good principle though. I dunno, maybe there's some field of programming in which it is...

    Me, I'm building web pages. If a page can't render because of one of the query string params doesn't validate, I want to drop what I'm doing right there and tell the server to return a 500. I don't want to have to write 500 lines of code to "keep track" of what I'm returning, even though I already know it's a 500. I also don't want to enclose the entire code of the page in 26 if() statements. Just so I can have some ridiculous single point of exit.

     

    Warning: Post needs format tags and I don't know what they are.

    The ONLY place this principle is valid is in fortran 77 (and possibly later versions) where it is permitted to pass labels to subroutines and for them to return TO (not from) multiple different places. It is also possible to supply multiple different entry points to any particular function. I am not kidding.

    This is a really bad idea.

     

    However, it isn't possible in C, C++ or any structured language and the rule doesn't need to be applied.

    The following (with apologies for the probably wong syntax, because it is difficult to remember it, fortunately) demonstrates how this can be abused (actuall, I don't think there's any way of doing this that isn't an abuse)

         Subroutine fred1(ret1, ret2, ret3)

          icalled = 1

          goto 101

     

          entry fred2(ret1, ret2, ret3)

         icalled = 2

     

    101 continue

    C Do something magic

        if (icalled .eq. 1 .and. something) return ret1

        if (otherthing .eq. 2) return ret2

        if (thirdpossiblitey) return ret3

        return

        end

     

       call fred1(20, 30, 40)

    C You might return here

    20 continue

    C or here

    30 continue

    C or here

    40 continue

    C or even here 

    </pre>


     


Log in to reply