Hello all. I work on a small team and I’ve been tasked with taking on the bulk of code review for when my senior goes on leave. Although I’ve read a fair bit of code (man am I lucky I learned to code before AI “got good”) I can’t say I’ve done a lot of actual “review”. Does anyone have any tips? I feel responsible if anything breaks after I review it so of course I want to avoid that.

  • Scratch@sh.itjust.works
    link
    fedilink
    English
    arrow-up
    11
    ·
    6 days ago

    Well for starters, having one person be the reviewer is not a great idea. One reason is that you have a single point of failure, as you are now discovering.
    Another reason is that it creates a codebase that reinforces one persons biases.
    Thirdly it can (potentially) silo knowledge, so if you only ever work on the data layer, you have no insight into any other part of the codebase.

    I would encourage you to try get others in on the reviewing effort. I’d prefer assigning people to review, rather than just hoping someone steps up. (There is a saying; If it is everyone’s job, it is nobody’s job)

    The actual review is multi part, Step 1: understand what was supposed to happen. (Was this change supposed to update a dependency? Update some text? Add a button?).
    Step 2: confirm the code builds. (I ain’t reviewing shit if it doesn’t even compile).
    Step 3: reading the code. You’re looking out for a bunch of different stuff; access control, function size, file size, design patterns, cyclomatic complexity, weird names, weird types, overly broad implementations, complexity where simplicity should be, threading shenanigans, lots of comments (usually bad). The list is nearly endless, but trust your gut, if something feels off that’s called a Code Smell, and it warrants further digging.

    It is very ok to reach out to the author and ask why choices were made, if they don’t have a firm answer, it’s time to get concerned.

    Once the code looks good, I move on to tests. Check that they build and pass. Then start looking at what they actually do.
    If a function takes an input, I like to add 3 kinds of test: Expected good inputs (like valid passwords, or correctly formatted date strings) Expected bad inputs (zeros and empty strings, malformed inputs) and lastly: Fucking chaos (+-int_max, the first chapter of Moby Dick, actual face rolling on the keyboard) a function with one test is not tested at all.

    Finally, at long last, actually run the code. Verify the code does what it set out to do.

    PR Approved.

    • galaxy_nova@lemmy.worldOP
      link
      fedilink
      English
      arrow-up
      1
      ·
      5 days ago

      We’re data engineers so all our code is the data layer for the most part. I do wish we had more process for code review but there’s a few reasons that isn’t super feasible, but we do definitely have knowledge silos regarding different pipelines and such. Probably I will be learning a fair bit of some processes I don’t own. Thanks for your comment!