• Bob (unregistered)

    The intention of the above snippet appears to be checking whether itemA contains all FRIST mandatory!

  • Dave (unregistered)

    I think it's unfair to criticise the addition of the fifth item. When you see code like this, it doesn't take long to e fairly sure it's just bad code, but much longer to be absolutely certain it is, and that you haven't missed anything.

  • (nodebb)

    Sometimes bad is bad.

  • William F (unregistered)

    TRWTF is those withImportantFooPiece methods are actually mutators instead of returning a new object. That naming style is typically used by immutable classes, not POJOs.

  • (nodebb)

    So the new (nullable boolean) field defaults (via constructor) to false.... Thus a false in an object is actually "nothing", but a "null" is something...

    Aside from other WTF aspects, the code breaking because of this seems right.

  • Jay (unregistered)

    Why? Just why? If you have to do this to work with your "equals" overload method, perhaps you either need a new method for your object or just do a field-by-field comparison. Maybe even use class inheritance to do a comparison at some type of base "Item" type and look for the other fields needed for the ItemB type. Does Java support that?

    As I think has been mentioned, if you have to null out fields after invoking the constructor, maybe you need an alternate constructor.

  • (nodebb)

    I think the most interesting thing is that SimpleItemB is a subclass of ItemB. Seems quite backwards to me since the Simple alowats contains a subset of the properties. I also think this inversion is what led the developer to need to clear out all of the extra fields and led to this particular coding pattern. I've used the SimpleX class concept to reduce traffic. Return a collection of Simples to build a list in the user interface and fetch the full sized object when a record is selected.

    Also, this sounds like it should have been implemented as a factory. Since this is Java and they love Gang of Four naming, I would expect it to be in the ItemBFactory class.

  • Kleyguerth (github) in reply to William F

    With an added WTF on top that itemA also has setXXX methods... I guess their style is: withXXX is chainable, setXXX is not.

  • (nodebb)

    If all other comments are removed, then one can determine whether i posted a comment by comparing the comment section to empty.

  • DROP TABLE USERS (unregistered) in reply to William F

    Actually, that's just how JAXB works. Your setters return void and you can add jaxb-fluent-api plugin to your pom.xml to generate 'withers' (yes, I've actually heard someone call them that, took me a while to understand what they were talking about) that just call the original setters and then return 'this'.

  • Michael (unregistered)

    But the check for whether the required fields are null only checks for fields 1-4.. Did the guy adding in field 5 forget to add it to the list, or was that just a mistake in anonymizing the code?

  • british (unregistered) in reply to Dave

    I agree, but for a different reason - sometimes it's a lot less feasible to rewrite bad code than it is to simply add to it. And when a deadline is approaching fast, you don't have time to waste debugging all the little problems that appear when you rewrite the bad code

  • BeenGoneTooLongFromSustaining (unregistered)

    This story makes me realize how long I've been gone from being responsible for fixing other peoples code. Its been about 18 years. So I'll just describe this as sometimes the road has ruts that are so deep that its easier to traverse them and take a few bumps than it is to flatten and pave a new surface.

  • If it works, it ain't stupid (unregistered)

    A cherry on top is the fact that importantDataPiece5 came about a few years after the original implementation. Someone saw this code, contributed to the monstrosity, and happily kept on trucking.

    Honestly, that's the correct approach. You don't just up and re-write battle-hardened code. That always takes significantly more effort, and often introduces bugs. Sure, in this case the dev introduced a bug while trying to maintain the status quo. But that's only in hindsight. The developer took the right approach with the information they had on hand at the time they had to decide how to proceed.

Leave a comment on “A New Generation”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article