• SG (unregistered)

    Blair has been here so long, he checks code in without peer review as it’s a purely manual process.

    Well, there's a problem to start with. In my organisation, nobody, no matter how senior, can merge code without a review... the tools simply won't allow it. And it's very rare that that's even an inconvenience, since getting juniors to review changes is a great way to improve their understanding of frameworks and good practices... resulting in less time spent reviewing issues in their code.

  • MiserableOldGit (unregistered)

    I inherited an application once where the "senior developer" had clearly read an article on n-tier architecture and missed the point entirely. A behemoth of an app with all sorts of vaguely related functionality shoving its info through a "Business logic DLL" and a "Data access DLL", neither of which did a damn thing except pass from input to output and give us a load of extra work and versioning headaches.

    Any actual logic (business, presentation, or otherwise) was in the interface or in the stored procedures/views. Any change was an end to end old-style journey from changing the table at one end to setting up validation on a control at the other, complete with having to update these useless layers in the middle.

    When he did start adding additional functionality via a web front-end he ignored his DLLs as he couldn't figure out how to get his chosen Java application server to use windows DLLs in the "middle tier" (it was hard enough dealing with all the unexpected nonsense sticking a Java front end on an MS SQL DB caused back then). So that was a mix of JSPs and stored procedures writing out javascript and HTML on the fly Needless to say inherited all that garbage too.

  • (nodebb)

    For example, he thinks static is a code smell, and thus removes the keyword any time he sees it.

    That is directly WTF territory, depending on exactly where the keyword is and why it is there.

    In a C or C++ function's variables, it's code smell for sure (mostly because of reentrancy / threading issues), but absolutely should not be just deleted.

    In a C compilation unit, it's unavoidable if you want non-public things (and non-public things are good to have around). In C++, they can be replaced, with sufficient care, by anonymous namespaces.

    In C++ classes, there are legitimate uses for them, notably factory methods, and converting those into per-instance member functions is a major no-no.

    But mostly it's WTF territory because it's a change just for the sake of changing it, which is never a good thing.

  • (nodebb)

    I've seen this more times than I care for: a business logic class with methods which are just wrappers around data access calls. This however is even better, a method which wraps around a method which wraps around a data access call - which isn't even needed? Some people to this day think the dress was yellow

  • (nodebb)

    The part that gets me is the public function is declared async, then the only thing it does is wait for another function call. There is a little bit of overhead setting up the thread so this async and await call is just a waste. Why not just call this function under normal conventions and be done with it?

  • Chris (unregistered) in reply to Steve_The_Cynic

    I believe this is C#, which is still a WTF if that's a case as there are definitely appropriate times to use static for classes and methods. I'm guessing this guy just think "static = global variables = bad", and has no real idea what it actually means.

  • (nodebb) in reply to Mr. TA

    I've seen this more times than I care for: a business logic class with methods which are just wrappers around data access calls.

    Because it's the only way to do it that doesn't involve much thinking.

    I too have seen this happen... usually to a project that I started. Instead of people thinking about reusability and trade-offs, they just try to get today's task done.

    Here's an example:

    You have an order class... while writing the ordering interface, you decide that an order is going to serialize with all of its lines and all you need to do to remove an order line is to remove it from the lines collection and save it. The next guy comes along and works on the order history interface. You are a wholesaler, so customers have a lot of orders and those orders have hundreds of lines each. He complains (rightly) that the serialization decision is creating performance problems in his work.

    This is when things go wrong: rather than talk out how to solve this, the second guy create a new class for orders that serializes without lines. Before long, the other parts of the application are randomly mixing the two orders in different parts of the application and each class is accumulating different parts of the business logic for an order. Even worse, there is duplication and their behaviors are diverging.

    Eventually some brainiac swoops in and decides that the best solution is to implement all logic in the data layer. His action seems to have solves today's problem, so it must have been the right thing to do. Then it is decided that in order to prevent this mistake in the future, no code will be written in the business class, it will just be an empty passthrough.

  • GS pant 76 (unregistered)

    Question to TDWTF staff - How do you find those 4 year old articles? Do you keep some database of submitters, did the submitter remind you of this article's existence or is you memory simply that good? I'm genuinely curious.

  • Functional (unregistered)

    TO: Blair RE: "GetLinkAndContentTrackingModelAndUpdate"

    Please rename to "tracking.update".

    A suspicion of intoxication notice has been sent to HR.

  • (author) in reply to GS pant 76

    Pick a search engine, and then site:thedailywtf.com $terms

  • Naomi (unregistered) in reply to Chris

    Wait until they realize constructors are "static".

  • (nodebb)

    The comment feels familiar. We have a coding guideline that requires function docstrings; The problem js just that most docstrings say what the function does ("calculate labels for result data files") but don't document the expected input data, nor the format of the output data - often liberally mixed with reused global variables. Gotta love "success = calculateResults();".

  • (nodebb) in reply to Chris

    I believe this is C#,

    For sure, which is why I specified that I was talking about C and C++ (because I know them well, and C# almost not at all). And of course it's worth remembering that I may also have said this:

    But mostly it's WTF territory because it's a change just for the sake of changing it, which is never a good thing.

  • Wizofaus (unregistered) in reply to Steve_The_Cynic

    Static variables in C# are definitely a code smell, there's no case I've come across recently where there wasn't a better alternative. Unfortunately popular libraries like JSON.NET still use them, meaning your JSON serialization can behave erratically depending on what the static variable holding the "default settings" happens to be set to.

  • Airdrik (unregistered)

    "Now, there were some problems with their job description and salary offer, specifically, they were asking for developers who do too much and get paid too little" Well, he definitely does too much, but pay is probably about right if not too high.

  • MiserableOldGit (unregistered)

    Some people get 13 years of accumulated experience on the job, others just repeat the same year 13 times.

  • Neveranull (unregistered)

    Whether or not any particular use of the keyword “static” in a C# program is a good idea, simply removing it completely changes the design of said program, most likely introducing not just a bug, but whole new unintended functionality.

  • Salty Team Lead (unregistered)

    "There’s only so many times you can reject a peer review before you start questioning yourself."

    It's a relief to know I'm not alone.

  • Jessica (unregistered) in reply to SG

    Unfortunately we use a cheap version of TFS which doesn't allow for true peer review process. We've just migrated to Git though, and proper review processes will be implemented now.

    Static can absolutely be a code smell. But static classes can be incredibly useful. We don't have static variables because that causes problems. But we have a couple of classes... or did...

    And I jogged Remy's memory of my previous submission, without digging it out myself. I assumed they have some sort of record of submissions! I'm glad the backstory was included. That's as much TRWTF as the code!

Leave a comment on “A Private Code Review”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article