I shall delay your PR with my giant egotistical scrotum by:

- Use less words for your function name

- Use this obscure pattern you have not learnt in your outdated compsci degree

- Each loop should get their own function

- Your function has too many parameters

- You must name every instance of magic numbers / strings

- Here, have another obscure test case to write

  • 5
    Numbers 0 and 3 were probably reasonable demands. 💩

    Why are you in a hurry anyway?
  • 12
    I've done a gazillion code reviews and TBH any of these could be legit.

    Senior devs are way more concerned about code health because it all comes back on their head eventually.

    Although, yeah, some can be egotistical jerks.
  • 3
    The popularity of a pattern does not influence its utility. If it fits the situation you fucking implement it and appreciate having learned something. Your university education is at best a basis to build on, realistically just a random sample of freely accessible online knowledge to justify the price of the degree, definitely not an arbiter of what techniques are and aren't legitimate.
  • 13
    People whining about PR feedback piss me off to no end. Why don't you just push to master if you don't want critique? It's not like the person reviewing your PR is less motivated in the progress of the project than you are. The ripple effects of bad code can be felt decades down the line and absolutely can kill projects, so getting each and every PR in a state where nothing more can be criticized before merging is the shared interest of everyone working on it.
  • 0
    Whatever comment beats LGTM :)
  • 2
    Comment section not going the way you'd hoped, huh buddy?
  • 3
    The first one is probably the only one I don't agree with. There are only advantages in choosing longer variable names rather than shorter ones.

    A long name is more explanatory, avoid names conflicts, makes clear what are the intentions and what are the differences with similar functions.

    Furthermore, thanks to IDE you don't need to type it entirely and you won't mistype it
  • 0
    As said by others, I've seen cases where all of this feedback has been useful given the correct circumstance.

    Both parties (raiser and reviewer) have to be pragmatic to a level that matches the pressure of the situation.
    E.g. is this a high churn area? Why does this PR need to go out yesterday? If this is existing code: what are the side effects of the reviewer's requested changes? etc

    Saying that about pragmatism: improving code quality tends to reduce the number of things teams need to compromise on when developing.
    In theory, developers can reclaim this time to proactively solve code issues (smells) before they become problems.

    This reviewer may be trying to find somewhere to draw a line to make the team less reactive and more proactive.

    If it helps: I lean on trying to first understand the wisdom in the reviewer's feeling about the code, and then doing my best to accommodate that wisdom if I agree with it. A call about it almost _always_ helps.
  • 0

    I completely feel the frustration of having slogged through some work, only to have what can feel like nitpicking given at the end.

    I'd have to see the PR to say decisively, but usually the sort of smells these comments speak of are warnings of insidious killers lurking in the code.
    Again, speak to the reviewer as this may be the case and they're trying to save your 🥓.

    If PRs always feeling negative: we've been trailing also adding praise to bits we loved about a PR so this stage isn't such a negative experience. Has been a lovely way to work for our team and has spawned some inspiring discussions on new conventions and approaches
Add Comment