• MT (unregistered)

    "If I put in fewer test, then there will be fewer failed tests."
    The logic checks out

  • Naomi (unregistered)

    The right type for an optional int is OptionalInt, by the way. :P

  • ray10k (unregistered)

    In the 3rd code block, there's a typo. .isnull() is called on the result of assertthat(), not on the result of .getmax()

  • dogfoot (unregistered) in reply to ray10k

    No typo there, that's the AssertJ convention.

  • Code Refactorer (unregistered)

    The WTF are 1.) using 0 as "no value". What if the max temperature of the sensor is really 0? I once fixed a navi software where "no destination" was coded as Geo-coordinates x=0, y=0. But this is an existing destination, located in the middle of a sea! So it is a first good step that they have corrected the max temperature in the JSON by omitting the value. 2.) not correcting the Data class. The getMax() should have returned an OptionalInt now. Then they could test with OptionalInt.isPresent() or OptionalInt.isEmpty(). (But not return an Integer object instead of an int, that is worse!) 3.) not testing the change. The change is in reading the JSON max property and then, if not there, storing a 0 into their Data class for it. They should have tested that and not changed the tests for the outcome, the final data objects and their lists, which are exactly the same before and after their change.

  • p (unregistered)

    If they changed the JSON serializer, shouldn't that be the unit under test? So they should be leaving these old tests alone and writing a new test that invokes the serializer to check that max value = 0 gets removed, I would think.

  • Brian (unregistered)
    But this was a code change, and a code change needs to have a test change to prove the code change was correct.

    I'd say that's the biggest WTF of the story. If you make a change that's already covered by an existing test, and that test doesn't suddenly start failing, well, that's kinda the whole point of automated testing.

  • Cidolfas (unregistered) in reply to Brian

    Agreed! The whole point of having a test suite is making sure that the output remains the same if you refactor the internals.

  • Naomi (unregistered)

    Oh, and Remy, your JSON document is malformed - there should be a comma after the close brace on line four.

    {
  dataNoMax: [
    {name: "sensor1", value: 20, max: 0} 
  ],
  dataWithMax: [
    {name: "sensor2", value: 25, max: 50 }
  ]
}
  • //Assert.isGood(comment) (unregistered)

    Using 0 for 'undefined' is a big WTF. If it's some zero bounded value then -1 can work, otherwise you want to use INT_MIN or something if you can't use a nullable type.

    Personally I'd bind the POJO tightly to the data object (i.e. allow max = 0) and have the logic to deal with '0 means undefined' elsewhere, even if that's just a property getter on the POJO. I like the fields of data classes to match the serialisation.

    The getMax() should have returned an OptionalInt now. Then they could test with OptionalInt.isPresent() or OptionalInt.isEmpty(). (But not return an Integer object instead of an int, that is worse!)

    What's the difference between using OptionalInt and having to call .isPresent and using Integer and having to check for == null? They're completely equivalent (assuming the compiler/JITter is intelligent enough to inline that .isPresent).

  • Functional (unregistered)

    Better names. Integral for the one with max, proportion for the one without. Integral can cast to proportion, proportion cannot cast to integral. Now the question is what to do with a no history hysteresis filter.

  • (nodebb) in reply to MT

    'I said to my people, 'Slow the testing down, please!'

  • (nodebb) in reply to //Assert.isGood(comment)

    What's the difference between using OptionalInt and having to call .isPresent and using Integer and having to check for == null?

    Code analysis, both human and machine. Joel Spolsky summed it up well when he said "make wrong code look wrong".

  • Scott (unregistered)

    JFC, reminds me of the place I just left. Hurr durr, null checking is too hard, so convert the nullable db values into ints/decimals/whatever, where minvalue means "the db held null."

    Because it's tons easier to check "x != int.MinValue" than "!s.HasValue". Sigh.

    Also, as we know, fewer tests mean fewer cases. Double sigh.

  • (nodebb)

    works as tested

  • MB (unregistered) in reply to Naomi

    That JSON is still malformed because the dictionary keys must be quoted strings.

  • sizer99 (google)

    | A charitable description of Java is that it’s a strict language

    Java's strictness (and verbosity) is a good thing. The language is designed so dozens of poorly skilled codemonkeys working with even worse outsourced stackexchange copypasters can work on large projects while stomping on each others toes as little as possible. It's the new COBOL.

    Of course the incompetent but cunning will find a way, but think what would happen if you gave them the power and dynamic typing of Python, or the random typing of JavaScript.

    Not that I want to use Java either, but it's great for other people.

  • Naomi (unregistered) in reply to MB

    So it is!

  • Chris (unregistered)

    On one hand, Wayne Gretzky says that you miss all the shots you don't take. But on the other hand, TDWTF says you pass all the tests you don't run. Who am I supposed to listen to?

  • Robert (unregistered)

    What I don't understand is, why use 0 to mean "no max"? I would have used something like 1000000000, or maybe even 2147483647. Once, when I had a need for a "placeholder" value in my own code, I used 1e100 (one googol), because I needed something that would compare as greater than any real, non-bogus value.

Leave a comment on “A Dropped Pass”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article