开源日报 每天推荐一个 GitHub 优质开源项目和一篇精选英文科技或编程文章原文，坚持阅读《开源日报》，保持每日学习的好习惯。
今日推荐开源项目：《反其道而行之 QtScrcpy》
今日推荐英文原文：《How To Give a Great Code Review》

---

今日推荐开源项目：《反其道而行之 QtScrcpy》传送门：GitHub链接
推荐理由：有的时候需要把手机作为电脑的第二屏幕来用，有的时候则是反过来——把手机画面加入电脑屏幕一起操作。这个项目可以将连接的安卓设备在电脑上直接进行操作，免去低头操作手机的额外一步，你甚至可以在这里操作手机来玩吃鸡游戏（虽然比直接操作手机还要麻烦些）。当然了，如果在工作空闲的时候，也可以用来稍微摸一个鱼什么的……

---

今日推荐英文原文：《How To Give a Great Code Review》作者：Steven Griffith
原文链接：https://medium.com/better-programming/how-to-give-a-great-code-review-7e32e5ba0771
推荐理由：审查别人代码时的注意事项

How To Give a Great Code Review

Giving and receiving feedback is hard but we all have the same goal: getting better

Code reviews are tricky. They can be hard to give and can take a lot of energy.

What is it that makes code reviews great? I’m glad you asked. I’m about to give you the steps I take to give a code review. This is for async reviews like you’d do on GitHub. I think in-person reviews are much more fun, but that’s not always the approach you get to take.

Hopefully, you have tools in place to prevent most of the style and preference comments in code reviews. It’s simply a waste of everyone’s time to have to comment about a missing semicolon, preferred white space rules, camel case or snake case, or other things that linters, prettiers, and common tools can take care of. Don’t waste your time on this. If your project doesn’t have any tools, then suggest some.

Also, consider how your comments stack. Don’t make comments on a method that will possibly get changed from an architecture suggestion. This guide should give some tiers to keep that from happening. Just let comments from one tier be addressed before moving to the next until you get a better feel for it. Multiple rounds of code review are okay, but overwhelming someone with a massive set of suggestions is hard to deal with. Each of these steps is meant to be taken in order. I don’t see a reason to continue with the next step if the previous one hasn’t been completed.

It’s important to remember that code reviews aren’t just looking for errors in your teammates’ code. These are opportunities to learn, catch possible oversights, screen bugs, ask questions, share domain knowledge, etc. Everyone on the team should want the same thing: quality software. The more eyes and minds on a set of code, the better. I’ve learned quite a bit just from reviewing code. It should be a thoughtful and rewarding process.

Also, don’t be scared to ask questions or have some conversation in code review comments. I’ve learned coding tricks and good architecture from code review questions I asked and had conversations about. It’s also okay to give an encouraging comment like “good catch!” or “I’m glad to see that get refactored.” This is another part of the programming process, not a rigid process to find errors and look super-smart.

High-Level Overview

You first need to read the pull request thoroughly. You should be able to see from this what has changed, why there was a need for a change, and how to prove that the change works or is happening. There are some cases where the changes will have no visible impact, so there is nothing to test. The request should, hopefully, state that. If not, you’ll need to converse with the author to make sure you understand. That’s a good time to ask them to clarify the description.

From this description, you should have at least enough of an understanding to test this changeset and prove that it works, that the bug has been removed, etc. For changes that have little to no impact on other code, you should have enough of an idea to test some paths to verify that. In some cases, it will simply be “ensure the tests pass.” If this is your specific work domain, you may have an idea of what it might impact, so you’ll be able to prove it.

Run the Code

Now you’ve got a proper understanding of the changeset, you need to pull the branch down. This step shouldn’t be skipped because it’s your job to verify that the change does what it’s supposed to. Your job as a programming team is to make sure QA finds nothing. If you don’t have a QA team, it’s even more important that you test as completely as you can.

Besides, if the code doesn’t work, you may not even need/want to go review the changes. It’s worth taking a look afterward to see if you can identify why it might be broken. You’ve already pulled it down and have your focus on the issue, so it’s worth suggesting where you see an issue if you can. I suggest at least a quick attempt at locating the error(s) before commenting, so you can make the comment more productive.

Get Your Head Around the Scope of the Changes

Now you have the branch pulled down, and you can verify nothing is broken, and the code works as expected. You’ll want to go through the diff and figure out where the meat of the changes are. A lot of the time, there are single lines with little to no impact, and sometimes there are only a few chunks of changes for the most part. If the diff is large, you may have better luck looking at one commit at a time. The idea is to get your head around the actual code changes at a level similar to the diff description. You’re not looking for coding errors specifically, though if something jumps out, you can take a note. Don’t comment on that right now.

What you want to do is find the architecture of the code. Not just of the changes, so you’ll need to look at more than just the red and green. That’s why it’s helpful to have the code pulled down for this step. You want to find out how the code is changed and how it relates to the code around it and, where applicable, the code that calls it.

Make Architecture Comments

You want to do this partly because you’re trying to get a sense for how you might do it. It doesn’t take nearly as long as new code because it’s easy to make guesswork like this from already-written code. You don’t need a 100% roadmap of how you would implement it; the author has already done that. You want an abstract look at a possible architecture choice, which will not only allow you to suggest something if you see it but understand better why the author might have made the choices they made.

If you have a suggestion on a new approach, go ahead and make it. Pseudocode, examples, links, etc. will help greatly in this. If you don’t have any suggestions, you should have an idea of why they might have chosen to implement it that way. If not, you should ask.

Make Comments About Surrounding Code

It’s important to get an idea of the context of the code around what has changed. I’ve seen some code that was in the diff that looked fine, but there was a helper method that did a similar thing that could have been used. The author took my comment and refactored the helper method a bit for an overall better solution. Had I not been looking at other code, we would have both missed that opportunity.

I’ve also seen code that looked fine in the diff. Imagine a total output that was total + tax and some defensive code to have tax be 0 when it wasn’t defined. That seems reasonable, but this was in a summary component. There were a subtotal method and output already, so I looked at the getTotal method and found that tax was already being added, even though none of this was in the code that changed. I found this by investigating why that particular change needed to be made. It turned out there a pre-existing bug that affected this. The solution was refactored to fix that bug and remove the code that caused me to look.

I could mention many cases just like this. If you see a change you’re not sure about, go investigate. If you wonder if changing XYZ to ABC in a test case was necessary, change it back and test it out. Look around at the code. Snoop. Don’t just view the stuff in the green.

Regarding These Past Two Steps

Getting a high-level understanding of the changes as well as poking around in the code to figure out scope and related code is important. Obviously, it’s important so you can give a good review, but it’s also important for knowledge sharing. This step can help you learn about the code and understand what is going on. If it’s code you work in, you’re being kept up to speed, and these steps will be quicker because you’re familiar with it. If it’s code you don’t usually work in, you’re getting knowledge of the system or systems via code review.

Many companies and teams struggle with knowledge sharing. I think giving good code reviews helps. Not only does it share knowledge by actually seeing and understanding code incrementally in changes, but it gives you the specific Rolodex of who to ask which questions. It also helps having seen something that you may remember in other work. That’ll help with overlapping tasks. It could prevent you from doing extra work, or it could just give you someone to consult before a task is implemented.

The last note here is about helpful comments. Just keep in mind it is much more helpful to point something out if you have examples, links, best practices to reference, etc. Just bringing something up without suggestions, pseudocode, or code examples is comparable to saying “I would have done that differently.” That in itself isn’t a great help to the reader. I’ll have more to say about comments later.

Reviewing the Code

Now that the code has a stable architecture and doesn’t need to change from anything around it, we are ready to actually review the code. I go back to the GitHub diff for viewing the changes and additions. I like to make two passes.

First pass, I make comments on general coding things I might see. I check whether anything can change to help the clarity, which I find just by reading the code. If I lose track of what a variable is during the code, maybe the scope of that code is too big, or the variable could have a better name. If I see lots of indentation, I might focus more on that chunk for a moment to see if something might simplify it. I write a lot of JavaScript, so if I see loops, it catches my eye because I favor the higher-level collection methods. If I see a variable get declared and then built in a loop to be returned, I might see if a map or reduce could work.

In this step, it is even more important to include example code. You already have the branch pulled down, so it’s easy to test out refactor stuff. You can be sure you’re always clear in your comments and looking at the same thing if you put in an example. Sometimes I put in more than one. I may have a few different ideas, and either I want the author to be able to decide which they like best, or perhaps my examples will spark an idea or inspire some good conversation that leads to a group consensus on the best improvement. If you leave multiple suggestions, examples, and are open to conversation, it’s extremely hard for your comment to be taken the wrong way. Besides, you’ve shown you are invested in the codebase by spending time and care on examples. Sometimes when you start writing an example, you figure out that you were mistaken, or you learn the answer to your question, so there’s no need for a comment at all.

The final pass is for errors and also stuff tools may have missed. Look for subtle logic errors, branches that may never be hit, and things like extra semicolons or whatever jumps out at you. Sometimes there’s formatting stuff the linter misses. This is a good time to double-check variable names and scopes too. Just read through and make sure everything is smooth, logical, and easy to read. This is also when I look at the tests in more depth.

Tests Are Real Code

It’s hard for me to want to review tests. It’s one of the things I just have to do as part of my job. If tests are hard to maintain or read, they won’t be maintained or read. Look for things you can do to refactor tests and setups. You actually have to read the tests to make sure the code is readable and that they make sense. The more complicated the tests are, the harder they are to review.

Things to Keep in Mind

Scope

Sometimes changes may be outside a given task’s scope. It should be up to the author if these changes should be done then or later. You can certainly suggest changes that need to be made, and you should. Just be aware that those changes may be put off for another time.

Boy Scout Rule

Incremental refactoring is necessary. Some people follow the Boy Scout Rule when coding, which means that they will always leave the code cleaner than when they found it. That means they may refactor something with the goal of improving the codebase a little. You must resist the urge to see another refactor as a result and insist they do that. This can take you down a road that seriously inflates the scope of the review. It’s fine to ask about these things and suggest a ticket be created for future work. But it’s not okay to insist the new refactor be made as a result of the incremental refactor unless the author is open to that.

Specifically call out what is optional

Don’t leave it for the reader to interpret what does and doesn’t need change. If something is optional or just a comment, call that out.

Don’t be vague

Be specific with comments. Leave suggestions, examples, and resources. If something is a best practice, it’s helpful to link some resources for the person to learn this. If something has performance impacts, you should be able to find a source for that.

Don’t swoop and poop

Swoop and poop is something I learned from a design team. It’s when someone comes in, shoots a bunch of negativity at your design, and leaves. Don’t make comments unless you have suggestions or solutions, or you want to talk about it.

Always give examples

There is nothing more helpful for the thought process than examples. It guarantees that you have a shared understanding, it can help teach someone, it’ll reduce the questions asked on a comment, it will ensure you’re not swoop and pooping, and it’ll be a more positive review.

Try the suggested changes for simple stuff

Sometimes an inline change can be easily made by the suggested changes feature on GitHub. The author will see this and have an option to just accept changes, and it’ll go right in as a commit. Try it out sometime.

Don’t hold up a review for no reason

If you’re blocking a review, make sure there’s a good reason. If you prefer something but don’t have any provable reasons, that is just a preference. If you feel very strongly about something, you need to make sure that it’s worth everyone’s time and the feature or fix being held up over it.

If there’s pushback, the author wins

Sometimes we get caught up on what is right or what is the best way. Sometimes the author doesn’t agree with us. If there’s a discussion where you disagree, and you don’t have empirical evidence or some demonstrable reason why you’re right, then it should be the author’s preference. Links or examples that are purely preference do not count. It’s okay to give a little.

Don’t go round and round reviews

If you’ve done a couple of rounds of code review, and the code is getting better, then leave it at that. Don’t keep dropping in new comments over new stuff you’ve seen or trying to make the code 100% perfect.

Be nice

For a lot of people, code reviews induce anxiety. You’re talking about something someone spent a good amount of energy on. It’s always easy to be critical of something already written. Be sure you consider the other person when reviewing. It’s much more beneficial to have a positive experience every time than to dread code reviews.

Proofread

Be careful before you hit the submit button on a comment. Read it again before you submit. Look for anything that can be taken wrong. Look for how often you say “I” and “you” instead of “we.” Look for any comments that could include a solution or more suggestions. Read this to make sure it’s something you’d want someone to comment on your review.

More Info

I was exposed to a great two-part article regarding code reviews. It was later in my career after I had already been through a hard uphill learning curve. I had learned a lot of those same steps the hard way.

I started out getting reviews from pretty much the worst person you could have give you a review. I’m not kidding at all to say you could declare a true statement and the guy would argue with you for the sake of arguing. We said “The sky is most definitely blue” one day to prove this. It made me very anxious about code reviews. I dreaded them, and I argued during them.

After he was dismissed, I was ready to argue with the next people. To my surprise, they were very positive and helpful. I started being excited to see what I would learn in code reviews. It was a great experience.

This transitioned to me being a gatekeeper at a new company. We’d bought another company, and I was supposed to review any integration code to make sure it matched our guidelines. I spent every review saying things like “Prefer not to use single-character variable names” and “a try-catch block inside a for loop that is five indentions deep seems very complicated.” I was met with “will not fix” and “I spent so long getting this to work that I don’t want to mess with it.” I wasn’t sure what to do.

This experience has led me to try and learn from code reviews. It reminds me to consider how the other end might feel. Both because I was on the hard end and because I gave reviews to people who just hated the thought of it.

There’s lots of information out there about how to do this, which goes to show you it’s harder than it seems. I try to make sure all the reviews I’m involved in are fun and rewarding. This should be your goal as well.

---

下载开源日报APP：https://opensourcedaily.org/2579/
加入我们：https://opensourcedaily.org/about/join/
关注我们：https://opensourcedaily.org/about/love/