Basic question: Is this code a "WTF"?



  • Hello again  :)

    1)

    <font face="Courier New"><font face="Arial"><font face="Courier New">            //Check if Unit price
                if(!UnitPriceIsValid())
                    return;

                //Check if Stock amount is valid
                if(!StockIsValid())
                    return;

                //Check if the item being added belongs in this shop
                if(!ItemLocationIsValid())
                    return;

    <font face="Arial">After looking at the above code, I realised that I could simply compress all those conditions into one if statement
                                   <font face="Courier New">if(</font></font></font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New">!UnitPriceIsValid() || </font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New">!StockIsValid() || </font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New">!ItemLocationIsValid())
                 return;

    <font face="Arial">I dont know what to do.
    The second peice of code is obviously more readible and "neater".
    On the other hand</font></font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New"><font face="Arial">, I want to keep the original code because it "preserves my thought process" while I was writing the code. But it also kinda makes me look dumb, I mean, another programmer could simply look at it and say "geez, what a dumbass, couldnt he have made that one statement". Or worse, my code could end up here! ;)

    Any insights? </font></font></font></font>

    <font face="Arial">Oh, and, while I am at it, I have another couple questions
    2) You see, sometimes I will learn something... and I will write my code based on that. Later on, I may find out a better way (or merely another way) to implement that function. This puts me in a dilemma.
    For instance, if I find a better way to do X... I will comment out my old code ([b]but will not remove it[/b]) and implement X using the better method.
    If I find merely another way to do X... I will then implement half my code using my old method, and the remaining half using the alternate method.

    3) Is it normal to write comments that are paragraph long? Ofcourse, in most cases my comments are single lines, but sometimes I find the need to elaborately explain what I doing (for later reference). Is this normal at the professional and enterprise level?

    For example: http://img501.imageshack.us/img501/6875/commentot8.png
    You see, I am not an experianced professional programmer (senior in college). So very often I find myself referring back to old code... so thats why I write detailed explanations in my comments so I can refresh my memmory.
    </font>



  • Keep the first version. When debugging, it's easier to see why the method returned early.
    Maybe you will want to do a bit more someday, e.g.

                //Check if Unit price
                if(!UnitPriceIsValid()) {
                    Logger.Log("cannot process item {0} of order {1}, price is invalid", ItemNumber, OrderNumber);
                    return;
                }

                //Check if Stock amount is valid
                if(!StockIsValid())
    {

                    Logger.Log("cannot process item {0} of order {1}, stock is invalid", ItemNumber
    , OrderNumber);
                    return;

                }

                //Check if the item being added belongs in this shop
                if(!ItemLocationIsValid())
    {

                    Logger.Log("cannot process item {0} of order {1}, location is invalid", ItemNumber
    , OrderNumber);
                    return;

                }


    or you might want to replace return with throwing an exception

                //Check if Unit price
                if(!UnitPriceIsValid()) {
                    throw new InvalidDataException("invalid unit price");
                }



                //Check if Stock amount is valid
                if(!StockIsValid())
    {
                    throw new InvalidDataException("invalid stock");
                }

                //Check if the item being added belongs in this shop
                if(!ItemLocationIsValid())
    {
                    throw new InvalidDataException("invalid location");
                }



  • Thanks
    @ammoQ said:

    Keep the first version.


    What you mean? Whats wrong with the other 2 questions?



  • @GizmoC said:

    Thanks
    @ammoQ said:
    Keep the first version.


    What you mean? Whats wrong with the other 2 questions?


    2) there is no easy answer for your questions. If the new way of doing something is much better than the old way, do it. Keeping the old code (as comment) is good for some time, since it might help you to find errors in the new version. After some months, remove those comments.
    On the other hand, if the new way is not much better than the old one, only different, better keep the project consistent.
    Bottomline: Either refactor the whole project to the new way it you think this is necessary, or keep using the old way.

    3) there is nothing wrong with long comments. If there is something non-obvious in your code, e.g. your program is doing something that looks strange but is necessary for some very special reasons, it's a good idea to elaborate about the reasons why you made it that way.



  • 1)Neither version is anywhere near being a wtf.  I'd keep the first one, like ammoQ.  It's easier for me to read, but it's more of a personal choice than anything else.

    2)Think of it also as having the code around to serve a purpose.  In your case, you're putting a lot of effort into learning how to code, and would like to keep detailed track of how your project is evolving.  Seems to me that this method fits your goals as well as any other.

    When you're programming for the man, you can probably rely on source control to keep old versions of your code for you, but for now, since your goal is in part to learn and observe, I would encourage keeping your old code handy.

    3)If you're coding for yourself, do whatever you want.  When you get into programming with other people, I think the way you comment your code is completely up to the group to decide as a preference.  Some people will argue passionately that the code should be 99% self-documenting, and others will tell you that every single step has to be commented.  Just go with the flow, as each has its trade-offs.

     

    All in all, everything in coding is a trade-off, and since the values of the trade-offs are still mostly subjective, the only thing you have to watch out for is some wank who thinks he knows the One True Path.



  • @GizmoC said:

    ...<FONT face="Courier New"><FONT face=Arial><FONT face="Courier New"><FONT face=Arial>another programmer could simply look at it and say "geez, what a dumbass, couldnt he have made that one statement"... </FONT></FONT></FONT></FONT>

    <FONT face=Tahoma>And don't be afraid of this kind of people as well... You probably know better than them...



    </FONT>



  • The best reason to have isolated the functions ( as you have in your first version ) is the order in which you have isolated them. At some point in the future the order may become important.  By isolating them you have given yourself some flexibility. Things always seem to change from the initial plan... It could turn out that Price is dependent upon Stock and Location at some point in the future. It could turn out that Stock is dependent upon Location, or otherwise. 

    Although it looks more elegant to test all conditions in a single if()... it is also more restricted.

    By isolating the functions you have achieved a more robust solution.  Where at some point, one function could fail but the other two might still prove true.  You could handle that with isolated functions. 

    Throwing all the functions into a single if() is an optimization that you would perform once the development process is done, and nothing is bound to change. However, again it's only for elegance. 



  • @olddog said:

    The best reason to have isolated the functions ( as you have in your first version ) is the order in which you have isolated them. At some point in the future the order may become important.  By isolating them you have given yourself some flexibility. Things always seem to change from the initial plan... It could turn out that Price is dependent upon Stock and Location at some point in the future. It could turn out that Stock is dependent upon Location, or otherwise. 

    Although it looks more elegant to test all conditions in a single if()... it is also more restricted.

    By isolating the functions you have achieved a more robust solution.  Where at some point, one function could fail but the other two might still prove true.  You could handle that with isolated functions. 

    Throwing all the functions into a single if() is an optimization that you would perform once the development process is done, and nothing is bound to change. However, again it's only for elegance. 



    I beg to disagree..

    if (a) return;
    if (b) return;
    if (c) return;

    vs.

    if (a || b || c) return;

    I'm going with the later.

    If the order changes. so that c has to validate before a:

    if (c || a || b) return;

    And C++ does short circuit, so if c fails, a and b will not be evaluated.

    I might go further and suggest that given:

    if (!a || !b || !c) return
    // Do lots of other stuff.

    would be better written as

    if (a && b && c) {
      // do lots of other stuff. Maybe in another function call.
    }


    As for the other two questions. Generally, I make a backup copy of a working version (or use RCS), and try out the new/better way.  If it doesn't work, I revert. If it does, then why keep the old/worse way?

    EDIT:

    And comments... Code should be self documented how it works. But it should be well commented on assumptions you make. Especially if your providing a class or function that is used by other people.  I would typically document how the method should be called, what happens when the arguments are given in all special/generic cases, possible side effects, how it handles error conditions, and what its used for.  Then again, I write a lot of Java code, and thats the typical javadoc style :-)


  • @danielpitts said:


    And C++ does short circuit, so if c fails, a and b will not be evaluated.

    C# as well. Not to mention I'm pretty sure "if (a) return;" does short circuit..



  • It's mostly a matter of style. If the three "if"s are sanity checks (which seems to be a reasonable assumption considering the names) I'd like to know which check failed, so I would put it in three if's (and add logging, return an error code or do something else to report why).
    If a, b and c are conditions that may perfectly legal evalutate to false (like in the example given by danielpitts), the one-liner is ok.



  • @danielpitts said:

    And C++ does short circuit

    As far as I know, pretty much every language in common but VB use does short-circuit (and that includes PHP and Javascript as well).

    And VB.Net has special short-circuiting boolean operators (sigh...)



  • @masklinn said:

    @danielpitts said:
    And C++ does short circuit

    As far as I know, pretty much every language in common but VB use does short-circuit (and that includes PHP and Javascript as well).

    And VB.Net has special short-circuiting boolean operators (sigh...)

    That brings me to another observation about coding like this.  For the original post, we can safely assume that short-circuiting behavior doesn't matter.  When you really get down into the nitty-gritty of a language, however, a well thought out one provides most of its various methods of expressing something for a reason.

    With the examples provided earlier in the thread, we can see that which option one picks could end up making a difference in what one's code expresses, depending on the particulars of a language.  The upshot, then is that when a more experienced programmer writes the short form if(a || b || c) in a language that doesn't short-circuit as opposed to the long form if(a), if(b), if(c), he might have intentionally chosen that form to explicitly specify that the order of evaluation doesn't matter.



  • @Oscar L said:

    @masklinn said:
    @danielpitts said:
    And C++ does short circuit

    As far as I know, pretty much every language in common but VB use does short-circuit (and that includes PHP and Javascript as well).

    And VB.Net has special short-circuiting boolean operators (sigh...)

    That brings me to another observation about coding like this.  For the original post, we can safely assume that short-circuiting behavior doesn't matter.  When you really get down into the nitty-gritty of a language, however, a well thought out one provides most of its various methods of expressing something for a reason.

    With the examples provided earlier in the thread, we can see that which option one picks could end up making a difference in what one's code expresses, depending on the particulars of a language.  The upshot, then is that when a more experienced programmer writes the short form if(a || b || c) in a language that doesn't short-circuit as opposed to the long form if(a), if(b), if(c), he might have intentionally chosen that form to explicitly specify that the order of evaluation doesn't matter.

    This would mean that his functions have side effects beyond simply returning (and potentially computing) a boolean value.

    I often hear that kind of programming styles referred to as "suicidal programming", and I truly hope this is not what we have here.



  • I think the best possible solution for this is to use data-driven programming:



    <font face="Courier New">$tests = array(</font>

    <font face="Courier New"><font face="Arial"><font face="Courier New">    "UnitPriceIsValid",</font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New">//Check if Unit price</font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New"> </font></font></font>

    <font face="Courier New"><font face="Arial"><font face="Courier New">    "StockIsValid",</font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New">//Check if Stock amount is valid</font></font></font>

    <font face="Courier New"><font face="Arial"><font face="Courier New">    "</font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New">ItemLocationIsValid"</font></font></font><font face="Courier New"><font face="Arial"><font face="Courier New">//Check if the item being added belongs in this shop</font></font></font>

    <font face="Courier New"><font face="Arial"><font face="Courier New">);



    foreach($tests as $test) if (!$test()) return;

    </font></font></font>



  • Just my $0.02 worth.

    1) I vote keep the first version. Even if you rely on the order of execution and short circuiting, the first option is a bit easier to read and figure out, plus I personally find it easier to reorder whole lines of code than boolean expressions within a single if statement. At least it takes less keystrokes in every editor I use.

    Also, if you come across a situation where the logic changes to:

    <FONT size=2><FONT face="Courier New">            //Check if Unit price
                if(!UnitPriceIsValid())
                    return;

                //Check if Stock amount is valid
                if(!StockIsValid()) {
                    if (somethingWentNuts())
                        throw new Tantrum("WTF!?!");
                    return;
                }

    </FONT></FONT><FONT size=2><FONT face="Courier New">            //Check if the item being added belongs in this shop
                if(!ItemLocationIsValid())
                    return;</FONT></FONT>

    <FONT size=2>
    </FONT>

    <FONT color=#000000 size=3>... again, it's a lot easier to modify and read/understand the result. All that said, there are times when you know the logic is not going to change all that much, in which case brevity is your friend.</FONT>

    <FONT color=#000000>2) Use some kind of version management!!!! Even if it's only zipping the source and naming it myApp_src_20060817.tar.bz2. Keep an archive and delete what you don't use.</FONT>

    <FONT color=#000000>I used to keep bits of code commented out here and there, but I found it just got in the way. Throw it away, you will feel a lot lighter in the centre of your being. Free yourself of unneeded worldly possesions and all that trippy crap.</FONT>

    <FONT color=#000000>3) Some comments are good short, some are good long. A rule of thumb is try to read the comments and code as if you were reading them for the first time. People who don't comment or under comment at the enterprise level are likely to be job security nuts. Such behaviour is based on fear. Fear leads to the dark side.</FONT>

    <FONT color=#000000>Typing an explaination to something tricky is way quicker than going through most of the mental process of solving the problem again. It's even worse if the next person to read the code is not you. Also, if your mental process is just plain nuts, the addition of verbose comments makes it a lot funnier if your code is posted to TDWTF.</FONT>



  • I prefer systems more like:

    <font face="Courier New">/** return a list of errors, or empty list if valid */</font>
    <font face="Courier New">public List validate() {
        List errors = new ArrayList();
        if (!UnitPriceIsValid()) {
            errors.add("Unit price not valid");       
        }
        if (!StockIsValid()) {
            errors.add("Stock is not valid");
        }
        if(!ItemLocationIsValid()) {
            errors.add("Item location is not valid");
        }
        return errors;
    }</font>

    Only return early if one error prevents and/or removes the necessity of checking another.  Use some kind of error object if you want to provide more info than just the strings.



  • As with most others, I would keep with the long version for three reasons:

    1) Debugging.  When you step through the shortened version, can you tell which function call came out false?  I don't know of any debuggers that step by any less than a line.  At least doing it line by line, you can see where it stopped.  This has bit me a few times.

    2) Clutter.  If you need to add several more if statements, or each case ends up having a bit more logic to it (i.e. a|b&c|^d) you won't end up with a single line that is really long and hard to read.  Also, by spreading them out it's easy to comment each one separately, instead of putting in one large block of comments that refer to all of the items on the line (becomes hard to follow).

    3) Flexibility.  Someday down the road you just know that you'll have to do a little extra something for one of those cases.  Maybe we won't consider some of those fatal errors.  Maybe we'll try an alternative if one fails.  Who knows.  The second method works only for simple cases and gives no room to maneuver.

    I think that's more than enough reason to stick with the previous style.  I would only ever use the condensed version in special cases, where there are few options and you are quite certain it will never change.



  • The fact that you're even asking questions like these puts you ahead of most developers.

    So many coders get this idea in their head that they know the best way to solve a problem and that their way is always best in every remotely similar case. The truth is that two cases might seem nearly identical at first, but when you actually think about it you realize that they have very different optimal solutions.



  • @xrT said:

    @GizmoC said:
    ...<FONT face="Courier New"><FONT face=Arial><FONT face="Courier New"><FONT face=Arial>another programmer could simply look at it and say "geez, what a dumbass, couldnt he have made that one statement"... </FONT></FONT></FONT></FONT>

    <FONT face=Tahoma>And don't be afraid of this kind of people as well... You probably know better than them...

    </FONT>

    No doubt.  Tell them you could have put the whole file on one line if you wanted.



  • 1)  As you're noticing, there is no concensus of opinion regarding the 'right' way to do this.  Needless to say, then no matter what you choose it is not a WTF.  As to the choice of coding style, let me recommend this rule of thumb:

    If your variables are inspectable in a debugger, then compressing them into one statement is cleaner and easier to read.  If you are testing against the return of a function call (as you are in your case), which is not as easy to inspect in a debugger, then separating them is cleaner so that you can determine which value returned more easily.

    2) Here's my recommendation: 

    • Write a unit test for the code to excersize the behaviour before you rewrite it. 
    • Do the rewrite, and make sure all the unit tests still pass. 
    • Dump the old code.

    3) Is it normal?  No.  Do I wish it was normal?  Yes, most definitely.


Log in to reply