1. 29

Hi Lobsters,

Would you all be interested in having a regular “code review thread” on the site, a la the weekly “what are you working” on threads, but for giving and getting feedback on each other’s code?

I’ve been lead to propose this from a couple different angles:

  1. Thinking about these threads about improving as an intermediate programmer.

  2. A lot of the reason I enjoy reading this site is because it often feels more like a community and less like a just a page full of articles.

I think having a regular code review thread would be great on both these fronts. Getting constructive feedback from your peers is one of the best ways to improve. There are a lot of smart cookies and people with deep experience on here and I think we could all learn a lot from each other. Having activities like this and the “what are you working on” threads also makes the site feel more like a community, to me at least.

What does everyone think? If enough people are interested, I’ll write up more details of what I’m imagining.

  1.  

  2. 9

    I find that reviewing code is one of the most dreaded aspects of my work, because it takes a lot of effort to keep enough context in my head to provide useful feedback beyond variable naming and very simple local improvements. All too often I overlook issues which I would have avoided if I was writing the code myself - reading somebody else’s diffs is a different level of difficulty.

    If I have so much trouble with a system I created and am immersed in every day, I can’t imagine how I could review somebody else’s code and provide useful non-trivial feedback without knowing the first thing about the software it’s part of.

    If somebody has strategies for reviewing and providing useful comments on somebody else’s code base, I’d love to hear about them.

    1. 7

      I do a lot of code reviews on codebases I’m not at all familiar with. Sometimes I’ll read some extra code for context - but often, if I’m puzzled or have a question about the code, then there’s something amiss. You might call it instinct, but if someone else can’t explain to you what a piece of code does, then quite often you’ll also find bugs there.

      Code reviews are also a chance for you to gain that context, and make sure that less of it needs to be in your head at one time. That’s why small, readable commits can be important - they help the reviewer walk through your thought process. Think of them as a learning lesson - “Who can see this code and get more context from it?” or “Whose code can I read to gain more context on X?”

      1. 2

        Sure, as long as it’s contained in the diff, then it’s OK to review. The difficult part for me is the “side effects” of the change.

        To take a simple example, if the code I’m reviewing changed the database schema and a query, were the other queries updated to reflect the schema change? Does the change have some code in common with another part of the system that should have been extracted? If the format or terminology changed in a report, were the other reports updated to match? Are there existing types or libraries that should have been used instead of adding new ones?

        This sort of thing doesn’t show up in a diff and requires me to have full knowledge of the system. Some of it could be caught by a sufficiently detailed test suite, but not a lot.

      2. 3

        If I have so much trouble with a system I created and am immersed in every day, I can’t imagine how I could review somebody else’s code and provide useful non-trivial feedback without knowing the first thing about the software it’s part of.

        I agree, and I think it’s entirely possible this won’t work out for this reason.

        That said, I’m imagining this being focused on relatively small (a few hundred lines or so), self-contained projects, which might help. I think this would also be different that reviewing code at work in that often the goal there is ambiguous or broad, whereas here the expectation would be that people request feedback on specific things (e.g. “I’m just learning $LANGUAGE and I’d like to make this code more idiomatic” or “This specific aspect of the design is dissatisfying to me and I’d like to improve it”). Ideally this would give the reviewer something specific to focus on at the expense of other everything else, which might help.

        It can be useful to have someone else read your code and just point out where they were confused and had trouble following it. This isn’t code review per se, but its certainly within the scope of what I’m proposing.

        In any case, I would also love to hear others' strategies for giving good reviews!

      3. 3

        I really like the idea!

        I’m curious to know how you would implement it as I have the feeling that lobsters comments wouldn’t be a very good medium to comment code.

        1. 4

          Some kind of gist like system? With the links being in the comments here.

          1. 5

            Yeah, I was envisioning something like this. Anyone who had a piece of code they’d like reviewed on would make a comment with a link to the code (on github, bitbucket, wherever) and some context (why they’re working on it, specifically what aspects of the code they’d like feedback on), and then others could reply indicating that they’d like to review.