Dead Code?



  • I was refactoring a bit of old code and wondered why the "i++" part was marked with a yellow curly line. Checking the IDE compiler warning told me, that this was dead code. I scratched my head for a while until I saw what made it say so. Can you spot the reason?

    for (int i = 0; i < t_TestList.size(); i++)
    {
        DataCriteria t_dataCriteria = (DataCriteria) t_TestList.get(i);
        t_dataCriteria.setStatus("Test" + i);
        getDataCriteriaDAO ().save(t_dataCriteria);
        break;
    }

    It turns out that the whole loop is unnecessary because it is being executed only once. I deleted the loop, replaced the references to the loop counter variable with zero, and proceeded with my work.



  • And just like that, you've broken the code. What if t_TestList was empty?



  • If it's empty, no problem. If it contains more than 1 element, though...



  • <p[>If it's empty there is a problem, because the new code will try to get() an element number 0 that doesn't exist. This will be why the IDE didn't mark the test part of the for loop as dead code, only the increment.

    For non-empty lists, the new code does the same job the old code would have.



  • @TheRider said:

    ....I deleted the loop, replaced the references to the loop counter variable with zero, and proceeded with my work.
    And that work included testing the change?



  • @OzPeter said:

    @TheRider said:
    ....I deleted the loop, replaced the references to the loop counter variable with zero, and proceeded with my work.
    And that work included testing the change?

    I think we're supposed to assume he's checked for size 0 and tested his solution. The code gives me a sense of unusually tight deadlines or inexperience.



  • @OzPeter said:

    @TheRider said:
    ....I deleted the loop, replaced the references to the loop counter variable with zero, and proceeded with my work.
    And that work included testing the change?
    Yes. The list will always contain exactly two items at that point. I forgot to post the preceding two lines, which are as follows:

    final int t_resultSize = t_TestList.size();

    assertEquals(2, t_resultSize);



  • @TheRider said:

    Yes. The list will always contain exactly two items at that point. I forgot to post the preceding two lines, which are as follows:

    final int t_resultSize = t_TestList.size();

    assertEquals(2, t_resultSize);
    So what happens if the assertion fails? And if you say it "can't fail", then why have the assertion?

    Of course this also raises the question of do you keep assertions in your production code - and there are arguments for keeping them in and leaving them out.



  • Isn't it the break statement that makes it only execute once? The number of items in the list is then irrelevant.


  • Considered Harmful

    @OzPeter said:

    And if you say it "can't fail", then why have the assertion?

    You always verify your assumptions, or someone will make that stupid old joke involving the spelling of assume.



  • This. I can't count how many times I've asked, "So, why aren't you checking for [error condition] here?" and gotten back, "Because that will never happen"

    Then a week later, or right after they commit the changelist, it happens.

    >_<

    Especially with user input. Always assume the user is an idiot.
    [IMG]http://4.bp.blogspot.com/-E4SN1dY0jSA/TrgaZuB8h7I/AAAAAAAAAkU/PgfntQx00Zw/s1600/idiot-using-computer.png[/IMG]



  • @OzPeter said:

    @TheRider said:
    Yes. The list will always contain exactly two items at that point. I forgot to post the preceding two lines, which are as follows:

    final int t_resultSize = t_TestList.size();

    assertEquals(2, t_resultSize);
    So what happens if the assertion fails? And if you say it "can't fail", then why have the assertion?

    Of course this also raises the question of do you keep assertions in your production code - and there are arguments for keeping them in and leaving them out.

    The code I am talking about is Java code, and it is a JUnit-Test. When you do Test-Driven-Development (TDD), as you should nowadays, you always write Unit-Tests to call your real code, and after that you assert that the outcome is what you wish it should be. In this particular case, some business method has been called beforehand, whose output now needs to be validated. The JUnit-Framework offers those assertXxx-Methods for this purpose. assertEquals(...) verifies that the two parameters are equal. If they are not, it throws an Exception and the code thus doesn't continue. The code that follows may thus safely assume that t_TestList exists (is not null, but the corresponding "assertNotNull(t_TestList) has been executed just before that) and contains exactly two items. A loop that does something with the first item in the list and then exits, therefore, has no purpose and can be written much easier, as I did.

    It looks like next time I post something, I need to give more context to begin with... :-)



  • @TheRider said:

    I forgot to post the preceding two lines, which are as follows

    Well then, this is code in a unit test.  If you don't post all the relevant code, how can we see whether its a WTF or not?

    So somebody wrote a test which iterated through the collection of t_TestList items; then later decided that only the first item needed to be tested.  Rather than taking the time to rewrite the loop, he just added a 'break'; re-ran the test, which passed.  Move on.

     Not really a WTF here, except that there probably should have been a comment to indicate why the break statement was added.  There is no value in making a unit test's code pretty or "production perfect".  It needs to be readable (so that someone coming along later can understand what the test is testing) and functional (actually performs the test it's intended to do).  



  • @TheRider said:

    It looks like next time I post something, I need to give more context to begin with... :-)

    I have heard of this thing TDD .. I was just being a bit obnoxious this morning :D

    And as for "should use TDD", the trouble is that in my realm such concepts are extremely hard if not impossible to implement as my code runs inside a proprietary VM on proprietary and specialized expensive hardware and drives complicated physical equipment.



  • @OzPeter said:

    @TheRider said:
    Yes. The list will always contain exactly two items at that point. I forgot to post the preceding two lines, which are as follows:
    final int t_resultSize = t_TestList.size();
    assertEquals(2, t_resultSize);
    So what happens if the assertion fails? And if you say it "can't fail", then why have the assertion?
    If it fails, then it has done its job in alerting you that there is a bug and preventing you from running on with GIGO data.

    @OzPeter said:

    Of course this also raises the question of do you keep assertions in your production code - and there are arguments for keeping them in and leaving them out.

    That's not how assertions work.  In every sane language (and most insane ones too) they are either default disabled at runtime or compiled out in a non-debug build.




  • @CodeNinja said:

    Especially with user input. Always assume the user is an idiot.
    No .. users are not idiots .. they are very intelligent and devious actors that will pillory you for the slightest mistake/assumption that you make in your code. To paraphrase Sun Tzu .. it is the users who tell you where your code fails.


  • Considered Harmful

    They will do very intelligent and devious things like putting their country in the county field, holding their drinks in the disc tray, and storing their important documents in the Recycle Bin.



  • @DaveK said:

    @OzPeter said:
    Of course this also raises the question of do you keep assertions in your production code - and there are arguments for keeping them in and leaving them out.

    That's not how assertions work.  In every sane language (and most insane ones too) they are either default disabled at runtime or compiled out in a non-debug build.


    The Assert statement may be disabled/compiled out, but the act of asserting conditions on data is not limited to that statement. For example throwing exceptions is also a form of assertion. In addition if you feel a condition is important to be asserted when you are testing code, then why is not important enough to be asserted in production code where a failure is even more critical?



  • @OzPeter said:

    In addition if you feel a condition is important to be asserted when you are testing code, then why is not important enough to be asserted in production code where a failure is even more critical?
     

    I read it the other way around - if a condition is important enough, don't put it in a programming construct that could be disabled at run-time (assert statement).

    Is TRWTF actually why they called it "assert" in the first place? 



  • @OzPeter said:

    @TheRider said:
    Yes. The list will always contain exactly two items at that point. I forgot to post the preceding two lines, which are as follows:

    final int t_resultSize = t_TestList.size();

    assertEquals(2, t_resultSize);
    So what happens if the assertion fails? And if you say it "can't fail", then why have the assertion?

    Of course this also raises the question of do you keep assertions in your production code - and there are arguments for keeping them in and leaving them out.

    What, both at the same time? Wish I knew how to do that.



  • @Fjp said:

    What, both at the same time? Wish I knew how to do that.
     

    You make them "always on top".

    Simples.



  • @OzPeter said:

    @TheRider said:

    It looks like next time I post something, I need to give more context to begin with... :-)

    I have heard of this thing TDD .. I was just being a bit obnoxious this morning :D

    And as for "should use TDD", the trouble is that in my realm such concepts are extremely hard if not impossible to implement as my code runs inside a proprietary VM on proprietary and specialized expensive hardware and drives complicated physical equipment.

     I regularly do embedded/industrial systems, many use custom processors and they all interact with "specialized harware" [ranging from ION beam etch/depo to military communcations/weapons, and space borne systems]...

     That has never prevented a TDD approach from being used.



  • @GoatRider said:

    Isn't it the break statement that makes it only execute once? The number of items in the list is then irrelevant.
    Yes, the number of items is then irrelevant but before that, the break statement may not be reached at all if the list is t_TestList.size() is zero (which, it has since been established, it won't due to a preceding assertion).

    What is the meaning of the t_ prefix in t_TestList btw? "this"?


Log in to reply