40
Root
1y

The nightmare continues.

Currently dealing with a code review from a “principal” dev (one step above senior), who is unironically called a “legendary dev” by some coworkers. It’s painfully obvious he didn’t read the code, and just started complaining and nitpicking.

It’s full of requests to do things that make absolutely no sense, and would make the code an unmaintainable mess.
• Ex: moving the logic and data collection from the module’s many callers into the module instead of just passing in the data.
• Ex: hiding api endpoint declarations by placing them in the module itself, and using magic instance variables to pass data to it. Basically: using global functions and variables instead of explicit declarations and calls.
• Ex: moving the logic to determine which api endpoint to use, for all callers, into the view.

More comments about methods being “too complex” (barely holds water) right next to comments saying “why are these separate? merge them together!”

Incredulously asking how many times I’m checking permissions and how ridiculous it all is. (The answer? Twice.)

Conflating my “permissions” param and method names with a supposedly forthcoming permissions system overhaul, and saying I shouldn’t use permissions because my code will all have to get rewritten. Even if that were true, and it’s likely not, the ticket still needs to use the current permissions. I can’t just ignore them because they might be rewritten someday.

Requests to revert some code cleanup because the reviewer thought the previous heavily-nested and uncommented versions (with code duplication) were easier to read. Unsurprisingly, he wrote them.

On the same ticket, my boss wants me to remove all styling and clientside validation, debouncing, and error messages from a form. Says “success” and “connection failed” messages are good enough. The form in question sends SMS and email using arbitrary user input for addresses. He also says it shouldn’t be denounced on the server, and doesn’t want me to bother checking permissions. Hello, spam!

Related: the legendary dev reviewer says he can’t think of a reason why we would want to disable the feature for consumers, so I should remove the consumer feature flag.

You can’t make this stuff up.

Comments
  • 23
    And here I thought the “wtf”s were supposed to come from the other side of the table.

    I accepted this job because I thought I would learn something. So far it’s been almost nothing but facepalming on my end.
  • 15
    @Root

    I hope you find a better job soon

    At least you learned how a job and devs should not be
  • 2
    I had a colleague intern that reviewed my PR in such a way I always felt I had a target on my back. Even the full time devs had some compassion in their reviews but a fellow intern got competitive.
  • 3
    Message from your principal dev:
    "I don’t understand your code. I don’t even wanna try to understand it. I’m going to rewrite it so I can understand it. I’ll remove comments as well since I understand my code. Oh, and let’s not sacrifice simplicity on the altar of robustness and security (unless of course that complexity comes from me). Have a good day"
  • 2
    Maybe it’s the same guy, and, when they say, “legendary dev” they mean “dev who is legendary for ripping of scrotum”.
  • 1
    There is always perspective bias and context issues with a reviewer. I'm now the most senior Dev but we are equal here. Many times I too have to defend some decisions and sometimes I can make these parts pointed out even better (while the proposed solution is already fine and better than the suggestion) or a separate ticket can be made to handle scope creep.

    So that is normal. What is not normal is what you just described. Prematurely removing permissions is grounds for a serious conversation. Perhaps some points can be improved but. I would just relay all issues in the comments with naming the general principles (like DRY) and tag your boss for review. Make sure to make screenshots before you do so the fucker doesn't alter shit.
  • 1
    It's fine to have a bit of a reputation and that can help with stalemates and trust in the organisation.

    This legendary Dev crap is bordering on nepotism. It should not be used to steamroll over everything nor should his reputation be tied to everything. That makes it so he can't admit any failure because the god king can't make a mistake.
Add Comment