• P (unregistered)

    What do you mean, this code perfectly obeys your "Do one thing, and do it well" rule. At doing everything at once.

  • my name is missing (unregistered)

    The worst code I ever saw was a single function main event loop in a classic Mac app - 14,000 lines long, indented 2.5 monitors deep. Compared to that, this is child's play.

  • bvs23bkv33 (unregistered)

    350+ lines? Alaric, main protagonist of "A Passion For Testing" story from 2019-07-01, sends his best regards and says that code is perfectly fine.

  • (nodebb)

    "Do one thing, and do it well." Has zero to do with how many lines of code something is. The length of this is not the problem, the fact it is doing more than one thing is. I'll put it this way, if it takes 3 lines of code to do something, do not write four, but if it take 100 lines of code to do something do not write 99. While lines of code is an indicator, it is not the measurement, be careful what you measure for you will get it. I will concede that the longer a function is, the higher the chance that it is doing more than one thing, but it is not what we are looking for. Now back to the topic, this thing... should not be broken up, it should be taken out back, shot and buried so we can start over.

  • Peter Tramo (unregistered)

    Wow. Much to see here. Let's have a look...

    empty "else"

    same action in a "if" and its "else"

    cringeworthy "if (boolean == true)"

    check against undefined for "redu" in some places, in others not

    "if (parseFloat(produkt.AKTIONSPREIS)< parseFloat(produkt.PREIS) && produkt.AKTIONSPREIS!="")" nice case of "shoot first, then ask if the string you just tried to parse was empty"

    "produkt.MENGE=parseInt(produkt.MENGE);" variable redefinition with a different type, good one

    Anyway, understanding which variables are meant to be strings and which ones floats is left as an exercice to the poor maintainer (check "var Preis = produkt.AKTIONSPREIS3" vs. "var Preis = parseFloat(produkt.AKTIONSPREIS3).toFixed(2);"

    BTW Preis redeclared with "var" everywhere because who cares. Who cares also whether it's kept to 2 decimals or not.

    Semicolons added according to the current mood of the code-monkey and random indentation for more fun.

  • Peter Tramo (unregistered)

    I missed some. Whoever wrote this thing didn't give a damn really.

    duplicated "POSITION.AKTIONSPREIS6 = produkt.AKTIONSPREIS6" with only 5 lines between the two occurrences

    "data.GESAMTSPARPREIS = parseFloat(data.GESAMTSPARPREIS).toFixed(2);" ... then ... "data.GESAMTSPARPREIS= toFixed(data.GESAMTSPARPREIS,2);" two shall be the number of decimals, and the number of decimals shall be two

    "data.ERSATZLIEFERUNG = data.ERSATZLIEFERUNG;" haha are you kidding me

  • Peter Tramo (unregistered)

    ...I should have stopped at the first line really.

    "var that=this;"

  • Dave (unregistered)

    At least it's well named. It is indeed a dire ctAddCartEntry.

  • Tony Robinson (google)

    The worst case of this I ever experienced was a method a little longer than this in C# with regions in it. I DIDN'T KNOW THAT REGIONS COULD EVEN BE PUT IN A METHOD BLOCK! It took about 2 months to completely refactor it into about 25 smaller methods.

  • (nodebb) in reply to Peter Tramo

    "var that=this;"

    It's a fairly common Javascript trick for when you want to save the binding context for inner functions that might otherwise have other binding contexts. It's at the heart of much of what is wrong with the language…

  • Andrew (unregistered) in reply to my name is missing

    Though I didn't see the code, some former colleagues once spoke to me of a nightmare of code in which a single case statement was 1000+ lines.

  • (nodebb) in reply to Peter Tramo

    cringeworthy "if (boolean == true)"

    Stabworthy.

  • ooOOooGa (unregistered) in reply to Peter Tramo

    The first thing that jumped out at me was the parameter named 'm'.

    Also the amount of dead code left as comments indicates that this is not under version control. Which often suggests that the only copy of this function is the one currently running in production - which is probably the only environment that is available.

    Have fun refactoring.

  • Stranger Things (unregistered)

    Looks to me like someone trained in procedural languages converted code into an object-oriented language and it compile, but really has failed to grasp the philosophical adjustment that needs to be achieved to write object-oriented code.

    The germanic-language flavor of variables and objects means I hear a nice Dutch resonance in my head as I read the code.

  • (nodebb) in reply to Peter Tramo
    "data.ERSATZLIEFERUNG = data.ERSATZLIEFERUNG;" haha are you kidding me

    Hey, it might have changed in the meantime. Race conditions, ya know.

  • Steve (unregistered) in reply to Peter Tramo 
     "data.ERSATZLIEFERUNG = data.ERSATZLIEFERUNG;" haha are you kidding me

    One of my wife's students kept writing A=A, etc. When queried he explained that he wanted to make sure it 'took'.

  • ItsObvious (unregistered) in reply to Peter Tramo

    Peter Tramo: You left off recursion. From 3 places (that I spotted) directAddCartEntry calls itself!

  • sizer99 (google)

    I like how the German and the ALLCAPS combine to make this routine VERY ANGRY. And you know what, I can't blame it.

  • Brad (unregistered)

    Beware the BOMRABATT !!

  • (nodebb) in reply to dkf

    That's what I was expecting to see when I saw that first line: "Ah, there's going to be some closures getting attached to objects somewhere". But no, that variable is never used.

    The second line is also unfortunate.

    var produkt = new Object;

    So I was trying to follow along and make sense of the code while being yelled at in German and wondering what it takes to read the "kategorie" model, until the commented-out browser events ("Ah, maybe this is where that will be used"). I scrolled a bit to pick up the code, and scrolled, and scrolled, and scrolled... Exterminieren! Exterminieren!

  • El Dorko (unregistered)

    Motherf... I worked in a place once where the head honcho did something like this. He had huuuuuuuge methods, thousands of lines, with flags passed in controlling what the method should do. As in, entirely different things depending on what flags you passed in. I still don't understand why, it was like he though there was a limit on how many methods you could have or something... Debugging that was a nightmare, and "fixing" anything would almost guarantee to fix something else.

  • doubting_poster (unregistered)

    low hanging fruit: TRWTF is sap.

  • Ruther Rendommeleigh (unregistered) in reply to El Dorko

    Humans are surprisingly capable of nesting abstractions. I sometimes find myself adding a new parameter with a default value to an existing method instead of writing a new one with a similar name, specifically for corner cases or exceptions to general rules. E.g. I'd rather change CalculateTotal() to CalculateTotal(Discount d=null) than add CalculateTotalWithDiscount(Discount d) - even if the change doubles its line count, because the basic idea of what the method does hasn't changed (much). Or has it? It's the old scope creep problem. If you never take a step back and reevaluate, I could see someone repeating the process ad nauseam without realizing that the "one thing" it does has changed from "calculate total" to "handle anything remotely shopping cart related" because they're just so used to this code that it has, in their mind, become a "one thing" in its own right.

  • (nodebb)

    puke...

    The "do one thing" rule is something that a self-appointed black belt senior developer can safely ignore, these rules are for beginners. A true artist lives beyond pointless restrictions like these.

    Maybe these "a=a" instructions are supposed to trigger some ingenious black box magical stuff through the setter property of whatever model.getData() returns? Something like a hidden recalculation that can't be triggered any other way? Because why implement a setter as a one-liner if more functionality can be included there conveniently to confuse others?

    I live in Germany, and within the first minutes of my apprenticeship I learned that there's no excuse for (and nothing more unprofessional than) using anything but English names in code. We had a "Visual Basic" t-shirt in the office that never ever got into the laundry. The penalty for abusive code like in this example would be to wear the t-shirt for an entire day or longer, depending on the degree of offense. Really helped enhance the output of the dev team lol. I'm picturing the moron inventor of this garbage pile choking on it right now. muahahaha

  • Feeling Lucky (unregistered)

    A day without SAP is like sunshine without rain.

  • PenguinF` (unregistered)

    BACK_PREIS? Does this mean that if I pay extra I get a bonus savory German pie?

    I tried to ignore the jungle of other prices but I couldn't resist making a list: AKTIONSPREIS AKTIONSPREIS3 AKTIONSPREIS6 AKTIONSPREIS12 BACK_PREIS ENDPREIS GESAMTPREIS GESAMTSPARPREIS PAKETPREIS PREIS SPARPREIS STAFFELPREIS3 STAFFELPREIS6

    Maybe AKTIONSPREIS12 gets commented out every year in January?

  • Talis (unregistered) in reply to PenguinF`

    Or maybe they have to add another AKTIONSPREIS each time they start a new selling campaign. ;-)

  • 🤷 (unregistered)

    Reading about PREIS so much really got me craving for PREISelbeermarmelade (cowberry jam). Let's see: AKTIONSPREIS = Special offer price. Apparently they have a special offer every 3 months. Like, spring sale, summer sale, winter sale...? BACK_PREIS. I have no idea bake price? Backup price? ENDPREIS = the price after taxes have been added. GESAMTPREIS = total price. PAKETPREIS = packet price. Sometimes you get a lower price when you buy something "in packs" or ISPs can give you a special "packet price" if you not only order the internet connection at that place but also your mobile plan. PREIS = that's simple. Price. :). SPARPREIS = savings price. "Only today 12% off!". STAFFELPREIS = scale price.

    Conclusion: They work in a clothing store.

Leave a comment on “Nothing Direct About directAddCartEntry”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article