It doesn't need refactoring



  • A peer has been off to the side working on something big in a separate branch - for several months. Yesterday, he merged into the main trunk, and I got the, erm, privilege, of reviewing what he did.

    BrandOneProduct1Event.java
    BrandOneProduct1EventDAO.java // one of these loads reference data
    BrandOneProduct1EventsDAO.java // the other one saves new records

    BrandOneProduct2Event.java
    BrandOneProduct2EventDAO.java // one of these loads reference data
    BrandOneProduct2EventsDAO.java // the other one saves new records
    ...
    BrandOneProduct14Event.java
    BrandOneProduct14EventDAO.java // one of these loads reference data
    BrandOneProduct14EventsDAO.java // the other one saves new records
    ...
    BrandSixProduct14Event.java
    BrandSixProduct14EventDAO.java // one of these loads reference data
    BrandSixProduct14EventsDAO.java // the other one saves new records

    Of course, he simply refused to put the load and save code for a given type of object in the same class, so you just need to randomly open one of XxyEventDAO and XyzEventsDAO to figure out which one does what.

    Also, Brands 1-6 are nearly identical, save for a few fields, so how about a base class with subclasses adding only what's needed? That's actually what he did in the DB; a primary table, with ancillary tables adding only the separate fields for each object. But not in code, no. Each class is a stand alone base class with all the common members, accessors and utility methods duplicated - across 84 classes.

    When I told him this was a pointless maintenance nightmare when the inevitable changes came along, he claimed that it made sense to him, and that it doesn't need refactoring.

    I'll let my boss discuss it with him.

     



  • Any class name with an ordinal in it is a WTF.



  • @morbiuswilters said:

    Any class name with an ordinal in it is a WTF.
    The brand/product class names are anonymized, but the person who wrote this is a walking WTF, straight off the boat of cheap H1-B imported labor.



  • @snoofle said:

    ... he claimed that it made sense to him, and that it doesn't need refactoring ...

    Quite right. It doesn't need refactoring; it needs reauthoring, preferably by a different author.

    @snoofle said:

    A peer ...

    Ah, this is obviously some strange usage of the word 'peer' that I wasn't previously aware of.



  • @snoofle said:

    he claimed that it made sense to him, and that it doesn't need refactoring.

    I'll let my boss discuss it with him.

     

    We had a coworker like that. Everything he did made sense to him.. unfortunately it didn't make sense to anyone else which lead to a lot of heated discussions with the software dev manager. The end result of any of those discussions was simply: just do it the way we told you to.


    Any discussions about architecture I'd make sure to have in ear shot of that manager so I could short-cut having to refactor/rewrite and just pull a, "What do you think $boss?" As he would make sure to keep an open ear on such discussions to try an nip the WTFery in the bud.


    This guy was a piece of work though. He once read that functions should be no more than a few lines (or something along those lines), which he bought into wholesale (no clue where he read that). So I never had to worry about 100 line methods. Instead I had to worry about digging through a depth of 9 calls across 4 classes to find what a call did. A call the could have just been a win32 call at the top level (single frigging function call). Each depth would add another argument until you had all the arguments from various objects at the final call.


    Not an original name, but I like to call him "the complicator". If something could be done in 3 steps: A->B->C, instead of writing it that way, or going directly A->C he would do something along the lines of: 01->A1->2->D->F->B1->B2->B3->E->A3->C


    We called it lily-padding the solution (i.e. trying to cross a body of water by jumping on lily pads, with 0 planning before hand). He'd eventually get there, but he often did a lot of backtracking and unnecessary work in the effort. Any other dev would have used the bridge that was already constructed.


    This sounds like a simple day or two refactor. I don't see why he's opposed other than being stubborn. I could see a, "Yeah, yeah, once I did the data layer I was a lot clearer about it. Didn't refactor due to time." But outright saying it doesn't need a refactor? Come on, his own code proves it.


  • ♿ (Parody)

    @dubbreak said:

    This sounds like a simple day or two refactor. I don't see why he's opposed other than being stubborn. I could see a, "Yeah, yeah, once I did the data layer I was a lot clearer about it. Didn't refactor due to time." But outright saying it doesn't need a refactor? Come on, his own code proves it.

    Most times I've seen this, the answer has turned out to be that copy and paste is less taxing on the poor soul than thinking about the problem in a reasonable fashion.


  • Discourse touched me in a no-no place

    @dubbreak said:

    This guy was a piece of work though. He once read that functions should be no more than a few lines (or something along those lines), which he bought into wholesale (no clue where he read that).
    Because there are idiotic coding standards enforcing such limits and idiotic programmers suggesting such limits littered all over the place.



  • @PJH said:

    @dubbreak said:
    This guy was a piece of work though. He once read that functions should be no more than a few lines (or something along those lines), which he bought into wholesale (no clue where he read that).
    Because there are idiotic coding standards enforcing such limits and idiotic programmers suggesting such limits littered all over the place.

    I would be more impressed if you had found a link to TDWTF.


  • Discourse touched me in a no-no place

    @morbiuswilters said:

    I would be more impressed if you had found a link to TDWTF.
    The nearest my google-fu could get to it is this one asking about limits on function length in Adobe Flash back in 2008.


  • ♿ (Parody)

    @PJH said:

    @morbiuswilters said:
    I would be more impressed if you had found a link to TDWTF.
    The nearest my google-fu could get to it is this one asking about limits on function length in Adobe Flash back in 2008.

    Here's something sort of relevant from discussion about the calculator contest:
    @DAL1978 said:

    My best/worst function has to be my division. Note that I am storing and processing numbers as strings with the order of the digits backwards. I have managed to come up with a valid use of true/false/maybe and abuse logic by gotoing from the if to the else block.
    My second best function is the subtraction which I refactored from 2 medium length functions into 144 functions of around 5 lines each.



  •  It's true that if a function gets very long (many unix-type projects put that level at the standard text editor's screen size), you should consider refactoring it into multiple /logical/ steps. Of course, there are the types that simple draw a line half way through the function, and make a _2 that is passed by reference every variable in play. Has anyone seen any such orangutan try to split a function in the middle of a loop?



  • @PJH said:

    @dubbreak said:
    This guy was a piece of work though. He once read that functions should be no more than a few lines (or something along those lines), which he bought into wholesale (no clue where he read that).
    Because there are idiotic coding standards enforcing such limits and idiotic programmers suggesting such limits littered all over the place.

    If this guy did functions that were 20 lines long I would have been happier. Somehow he got the idea that 1 line was the length to shoot for. And when he got on that stint the longest I saw was around 5 lines.


    This points towards the overall problem of people that can't use sense and must follow rules. Does extracting the portion of that long function as a separate function/procedure/method make sense? Does it make it more readable? Is it a good abstraction? Is it providing legitimate re-usability? I mean, think about what you are doing for FSM's sake.



    Why was the suggestion of 20 lines made? To be more readable? Ok, does splitting up that function make it more or less readable? THINK! As a dev that's what you're paid to do. If you want to just program then be prepared to be told what to do by someone else. Don't like that? Then use your brain.


    That guy was let go. The mess he left wasn't fully understood until he left. Somehow, absurd um-maintainable coding practices aside, he wrote stable code that was amazingly bug free (considering his practices). Of course I'd take maintaining the code from the bug prone guy that only coded for the ideal conditions as he commented and had a reasonable clue on how to structure code. Refactoring his code was livable as opposed to a living nightmare. This guy LOVED inheritance. He tried to incorporate it wherever possible, because.. well that's what you do in OOP right? I have never seen such weird abuses of polymorphism before. I really should have kept some to submit, but none could be capture in a few lines anyhow. The call 9 layers deep with one or two lines per function was a good one. Should have submitted that.


  • Discourse touched me in a no-no place

    @dubbreak said:

    Why was the suggestion of 20 lines made?
    Probably a throwback to when terminals were only 80x25 and the idea was to be able to see the whole function on the screen at the same time in most cases.



  • @PJH said:

    @dubbreak said:
    Why was the suggestion of 20 lines made?
    Probably a throwback to when terminals were only 80x25 and the idea was to be able to see the whole function on the screen at the same time in most cases.

    Because, God knows, it's not possible to understand a function if you can't see the entire thing all at once. And, God knows, nobody has a screen height over 25 lines.



  • @PJH said:

    @dubbreak said:
    This guy was a piece of work though. He once read that functions should be no more than a few lines (or something along those lines), which he bought into wholesale (no clue where he read that).
    Because there are idiotic coding standards enforcing such limits and idiotic programmers suggesting such limits littered all over the place.

    At the risk of sounding repetative: individuals like structure and guidelines, and in the absence of any they'll either enjoy the freedom to become a loose cannon or adopt a set of practises from elsewhere, believing it to be more beneficial than without.

    (I know in this case the issue revolved around not adhering to organisational policy yet had no problems following external guidance. WTF?)

    Those links are quite interesting - many use "should", "ideally" - implying that they are suggestions and aspirations rather than cold rules. Does anyone here have such rules/guidance in their organisation's coding policies? (I've not heard of it meself)

    @morbiuswilters said:

    @PJH said:
    [quote user="dubbreak"]Why was the suggestion of 20 lines made?
    Probably a throwback to when terminals were only 80x25 and the idea was to be able to see the whole function on the screen at the same time in most cases.

    Because, God knows, it's not possible to understand a function if you
    can't see the entire thing all at once. And, God knows, nobody has a
    screen height over 25 lines
    .[/quote]

    Not back in the day of dumb terminals, they didn't. You young 'uns don't know how lucky you are with your colour and sound and mouse thisand drag that, this was all fields, etc.

    Perl has some rule about the number of bugs trippling when the code goes over one page/screen, doesn't it?



  • @Cassidy said:

    Those links are quite interesting - many use "should", "ideally" - implying that they are suggestions and aspirations rather than cold rules. Does anyone here have such rules/guidance in their organisation's coding policies? (I've not heard of it meself)

     I'ld rather have suggestions in a coding guide than idiotic "must" rules.



  •  20 lines or less for a method or function? That's a bloody joke.

    It's as bad as the rule (although not always strictly enforced) that code in the Linux kernel shouldn't go beyond 80 characters per line because of old tty terminals. I'm not sure, but I think people may have gone beyond those now to use larger monitors with support for non-monospaced fonts, but maybe that's just me being crazy and assuming too many advances in technology have been adopted on a wider scale?



  • Personally, I could care less abotu number of lines in a function, that's as stupid as paying a programmer per line of code.

    I much preferr and in some places made ti a strong recommendation when I coudl have the influence that a finction should have it's focus on one task.  If you could do that task in 5 lines or 500 I didn't care, as long as it was a single task.

    Now how do you define tasks, well we did hope our programmers had some common sense and didn't call "processing this entire file of individual transactions while charging the credit cards" or "adding this variable to a string" a single task.  Code reviews kept that at least in the realm of sanity.


  • ♿ (Parody)

    @KattMan said:

    Personally, I could care less abotu number of lines in a function, that's as stupid as paying a programmer per line of code.

    I much preferr and in some places made ti a strong recommendation when I coudl have the influence that a finction should have it's focus on one task.  If you could do that task in 5 lines or 500 I didn't care, as long as it was a single task.

    Now how do you define tasks, well we did hope our programmers had some common sense and didn't call "processing this entire file of individual transactions while charging the credit cards" or "adding this variable to a string" a single task.  Code reviews kept that at least in the realm of sanity.

    But how much less? What if we start with a simple bound that we can compare? More or less than spelling typos?



  • @Weps said:

    I'ld rather have suggestions in a coding guide than idiotic "must" rules.

    That was kinda my point - I read them as COULD DO and SHOULD DO, rather than MUST DO.

    (or maybe I misread them. It was early morning and I didn't take it all in)



  • Cup1ProductDrinkEvent  // One of these contains wine, one contains poision.

    Cup1ProductDrinksEvent  // The battle of wits has begun.




  • @snoofle said:

    BrandOneProduct1Event.java
    BrandOneProduct1EventDAO.java // one of these loads reference data
    BrandOneProduct1EventsDAO.java // the other one saves new records

    snip

    Of course, he simply refused to put the load and save code for a given type of object in the same class, so you just need to randomly open one of XxyEventDAO and XyzEventsDAO to figure out which one does what.

    Was it consistent (eg, Loading in Event and Saving in Events) or random? Please tell me it was random.



  • @pkmnfrk said:

    @snoofle said:
    BrandOneProduct1Event.java
    BrandOneProduct1EventDAO.java // one of these loads reference data
    BrandOneProduct1EventsDAO.java // the other one saves new records

    snip

    Of course, he simply refused to put the load and save code for a given type of object in the same class, so you just need to randomly open one of XxyEventDAO and XyzEventsDAO to figure out which one does what.

    Was it consistent (eg, Loading in Event and Saving in Events) or random? Please tell me it was random.


    I bet it was consistent but he changed his mind part way through (possibly more than once). Don't worry, it makes sense to him.


  • @pkmnfrk said:

    Was it consistent (eg, Loading in Event and Saving in Events) or random? Please tell me it was random.
    It was mostly consistent (three exceptions, and one of them neither loaded nor saved data; it just had seven unrelated functions that the guy just put in there, probably because it was the file he had open when he realized he needed those functions... but that's so typical around here that I don't even bother posting stuff like that)



  • @RichP said:

    Cup1ProductDrinkEvent  // One of these contains wine, one contains poision.

    Cup1ProductDrinksEvent  // The battle of wits has begun.


    2cups1RichP


  • @morbiuswilters said:

    @PJH said:
    @dubbreak said:
    Why was the suggestion of 20 lines made?
    Probably a throwback to when terminals were only 80x25 and the idea was to be able to see the whole function on the screen at the same time in most cases.

    Because, God knows, it's not possible to understand a function if you can't see the entire thing all at once. And, God knows, nobody has a screen height over 25 lines.

    Um, I used to have a co-worker (who also happened to be my boss) who was like that. Whether his dyslexia had anything to do with it, I don't know, but he needed to have all of a function on the screen before him. (And this was at the time that 17" screens were the standard for professionals.)

    His solution? Don't use any formatting (after all, C is a context-free language) and have all code fit on one screen.



  • @CarnivorousHippie said:

    @snoofle said:

    A peer ...

    Ah, this is obviously some strange usage of the word 'peer' that I wasn't previously aware of.

    Who says the guy isn't nobility? Everybody knows that nobility are useless at anything (except hunting poor defenceless animals and nurturing inbred mental retardation), and this includes programming.

     


  • Discourse touched me in a no-no place

    @PJH said:

    @dubbreak said:
    This guy was a piece of work though. He once read that functions should be no more than a few lines (or something along those lines), which he bought into wholesale (no clue where he read that).
    Because there are idiotic coding standards enforcing such limits and idiotic programmers suggesting such limits littered all over the place.

     I hate you for making me read that first link, which advocates using exceptions to fake goto.


  • Discourse touched me in a no-no place

    @FrostCat said:

     I hate you for making me read that first link, which advocates using exceptions to fake goto.
    You bothered reading that far? The bit I was referring to was the last paragraph of the first section



    Anyway, you're welcome.



  • @Severity One said:

    after all, C is a context-free language

    No, it's not.



  • @Sutherlands said:

    @RichP said:

    Cup1ProductDrinkEvent  // One of these contains wine, one contains poision.

    Cup1ProductDrinksEvent  // The battle of wits has begun.

    2cups1RichP
     

    As you wish

     



  • REgarding lines of code in a functions...my general rule is if you can not describe the function in sufficient detail for a person to comprehend the functionallity in one sentance, it might be too big.



  • @TheCPUWizard said:

    REgarding lines of code in a functions...my general rule is if you can not describe the function in sufficient detail for a person to comprehend the functionallity in one sentance, it *might* be too big.
     

    If your function's parse tree does not fork...you might be a redneck coder!



  • @Severity One said:

    @CarnivorousHippie said:

    @snoofle said:

    A peer ...

    Ah, this is obviously some strange usage of the word 'peer' that I wasn't previously aware of.

    Who says the guy isn't nobility? Everybody knows that nobility are useless at anything (except hunting poor defenceless animals and nurturing inbred mental retardation), and this includes programming.

     

    Hey! that hurts...

    btw it seems that this site is chrome retarded



  • @serguey123 said:

    btw it seems that this site is chrome retarded

    Works fine for me, except for the fact that the editor uses plain mode instead of RTF mode. But since CS hasn't been updated since before Chrome was released, that's understandable.



  • @morbiuswilters said:

    @serguey123 said:
    btw it seems that this site is chrome retarded

    Works fine for me, except for the fact that the editor uses plain mode instead of RTF mode. But since CS hasn't been updated since before Chrome was released, that's understandable.

    Besides the editor the font seems screwy


Log in to reply