• Prime Mover (unregistered)

    Everybody is a peer. Otherwise your bladder would burst.

  • akozakie (unregistered)

    TRWTF is of course the leader with no peer review. "Leaders" often don't realize that things like that not only are good for code quality, but also strengthen - not weaken - the team and your position in it, allowing you to show - not tell - everyone that while yes, you make the final decisions, you are open to constructive criticism delivered in good form. Let them comment on your code to your face in review (or similar setting), or have them laugh at it behind your back, there's no other choice, because - while it's hard to believe and maybe painful - you're not as infallible as you think.

  • (nodebb)

    The correct way is to define an extension method named something like "In" which has multiple overloads and does equality internally, with inlining attribute: "v".In("a", "b")

  • WTFGuy (unregistered)

    @Mr. TA - Yes. And the In extension method should return an Enum like

    Enum InResult {
    Frist,
    Secnod,
    Thrid,
    fourth,
    Filenot_Found
}

    So the calling code can learn (just in case it cares) which of the arguments matched. When defining the Enum it's important to ensure you have spelling errors, inconsistent casing, inconsistent use of underscores, and most of all, make the default / no-match value be at the the end, not the the beginning to stymie future non-breaking changes.

    Once this Enum is in place you can improve the supervisor's code to read

    if (report.spName.In("thisReport", "thatReport", "thirdReport", "thirdReportButMoreSpecific") == InResult.Frist || (report.spName.In("thisReport", "thatReport", "thirdReport", "thirdReportButMoreSpecific") == InResult.Secnod
{
  LoadUI1();
}
else if (report.spName.In("thisReport", "thatReport", "thirdReportButMoreSpecific", "thirdReport") == InResult.Thrid || (report.spName.In("thisReport", "thatReport", "thirdReport", "thirdReportButMoreSpecific") == InResult.Thrid
{
  LoadUI2();
}
else if (report.spName.In("thirdReport", "thirdReportButMoreSpecific", "thisReport", "thatReport") == InResult.Filenot_Found
{
  LoadUI3();
}
else {
  throw new InExtensionMethodProbenotFoundInargsExcetion() // important to include zero state information for security reasons.
}

    There; that's so much better! ;)

  • (nodebb) in reply to WTFGuy

    You are on the right track but not quite there yet. Why define an enum when you can simply return a string like "frist", etc.? Also, like I said many times before, your code lacks NoSQL, data bricks and big insight lake.

  • Cidolfas (unregistered)

    I actually went through a phase where I made that mistake - being the "senior dev" with everyone else being junior, I didn't get my code reviewed by anyone, and assumed that all other senior devs had the same setup. I realized my mistake when I finally looked at what another senior dev on a mini-team within my team, who also hadn't had his code reviewed, had done. We're still picking up the pieces from that. And since then, everybody, without exception, gets their code reviewed.

  • DQ (unregistered)

    I was going to hire WTFGuy, but Mr. TA makes a good point about not including NoSQL and the rest.

  • (nodebb)

    I would be more concerned about this WTFery if the else block reported "invalid report name". The original code is already forgiving of invalid names (it defaults them to UI3), and the revised code just picks a different incorrect UI in the case that the invalid name happens to be a substring of a valid name. In practice, variables like this don't usually get filled in arbitrarily, so such coincidences are unlikely.

  • One of Us (unregistered)

    report.spName = "thisReport"

    report.ui = LoadUI1

    report.ui()

  • ZZartin (unregistered)

    Lol it's all good until someone names a report "eport, thatR"

  • Angela Anuszewski (google) in reply to Prime Mover

    Tell that to Amazon.

  • James (unregistered) in reply to Prime Mover

    At first I thought this was the worst frist comment ever, but then I remembered that urine is #1

  • jay (unregistered)

    I have fond memories of working on a program where a certain function was called with a string parameter that it compared against "Stock", "Item", and several other values I forget to control processing. ("Stock" meant "addition or adjustment to inventory" and "Item" meant "sale", but that's another story.) We had a bug where a program that called this function wasn't working correctly. I spent hours studying the code and stepping through it trying to figure out the error, before the lightning bolt suddenly hit me that they had called it with parameter value "stock". Not "Stock", but "stock". There was, of course, no checking for invalid values, so it just fell through to the "else" (or out the bottom) in every test. (I won't even get in to discussing the logic flow.)

  • (nodebb)

    "WHY IS thirdReport2 SHOWING UI2?!?!?!?! IT'S SUPPOSED TO SHOW UI1! YOU IDIOTS!" (says the team leader)

  • Alex Vincent (google)

    I have two words for you: automated testing.

  • Nick (unregistered) in reply to akozakie

    Agreed! Everyone’s code should be reviewed.

    The real challenge though is getting Junior devs to do a real code review. I have a strong suspicion (mostly based on the amount of time taken for a review) that the junior devs’ reviews go through a very short checklist:

    1. Did <Senior Dev> write it?
    2. Did it compile?

    Sometimes, I don’t even think they bother checking point 2 if it passes the check on point 1...

  • Erwin (unregistered) in reply to Alex Vincent

    To ensure code quality, all automated tests must be written by the Leader.

  • linepro (unregistered)

    What no rules engine?

  • Officer Johnny Holzkopf (unregistered) in reply to Erwin

    For more reports and greater leadership, code quality has been standardized. We are sorry.

  • (nodebb)

    The best person to review the most senior developer's work is the most junior team member.

  • Some Ed (unregistered) in reply to Nick

    This. OMG this.

    Though, to be perfectly blunt, when I can get them to check point 2 on anybody's code, including their own, it's usually a good day.

    I mean, I think they do it a little, but assertions along the lines of, "I fixed so many compile errors, I hoped that was enough" have been made by several of them, and I feel like it's likely that the only reason I haven't heard it from all of them is the rest understand those claims are just as bad as they sound. The compiler never cares how many compiler errors they fixed, it only cares if there are any compiler errors left.

  • Neveranulll (unregistered)

    My team lead would commit his code changes without review, and without writing any of the unit tests required by everybody else. Later he would assign bugs or enhancements to his team members, and they would realize that in order to test their changes, they’d have to write the entire missing unit test harness. Then he’d complain that their minor changes to his code were taking way too long.

  • You had me.. (unregistered)

    This hits a bit too close to home. Actually I'm wondering if this was submitted by a coworker who recently left.

    How do you deal with "team leads" like this? Micro managers, who always have to have their touch on your work, who demand piles of inept, inane, sometimes incorrect shit before they'll approve your PR? Teams leads who make suggestions exactly like this? Team leads who, in their PR, add a function and changes the quotes in the remainder of the entire file, making you wonder what exactly has changed on those lines and making it hard to understand what his contribution was in the PR?

    Please.. I need help here. I'm not kidding.

  • gnasher729 (unregistered)

    I spent two weeks of my life hunting for a bug that had been introduced six months earlier by another team's lead dev who thought his code didn't have to be reviewed. The effect of the bug was that your computer crashed if it was put to deep sleep for about 45 seconds - 30 seconds was fine, 60 seconds was fine, but an interval somewhere in between crashed it.

    His change: He spotted a statement if (ptr != NULL) { ... } and since clear and understandable code like that is for wimps and not for real programmers, and because he was smarter than anyone else, he changed it to if (!ptr) { ... }. If you can't see the difference, neither could he, apparently.

  • Grunthos the Flatulent (unregistered) in reply to You had me..

    Changes such as a quote on the remainder of the file should be in a different commit, doesn't matter about PR.

  • 516052 (unregistered) in reply to You had me..

    You find another job. If you are good at what you do that's not a big problem. If you aint well there ain't helping you.

  • Gerold 'Geri' Broser (github)

    For the friendship book (not only for team leads):

    be conservative in what you do, be liberal in what you accept from others

    – Jon Postel, RFC 761, January 1980, https://datatracker.ietf.org/doc/html/rfc761#section-2.10

Leave a comment on “A True Leader's Enhancement”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article