“You can’t curl alone.” - Tim Nelson
“All sports are games of inches.” - Real curling quote
Curling, the most popular sport in America, is a game in which two teams of four take turns sliding stones on a sheet of ice toward a target area which is segmented into four concentric circles. Although an individual’s gameplay only makes up one-fourth of the team’s total gameplay, that one-fourth can make or break the game. Players can knock each others’ stones off the ice, causing shock and awe in this magnificent match, but, more importantly, completely change how the game plays out. Every player on a team has something to gain from one another, and professional curlers consistently note that their biggest improvements come from listening and learning from their teammates. Thus, after every match, it is curler tradition to stand in a circle, lean in close to one another, and provide constructive, professional feedback about gameplay to all members of the team. This is called the curl review. Software engineers have stolen this practice from the curlers, renaming it code review. You will get to participate in your very own code review today.
Code review is a standard industry practice in which fellow team members check new code for mistakes before it is integrated into the team’s code base. It serves several functions:
- It ensures that all software engineers on a team have a general understanding of what code their teammates are writing
- It facilitates discussion within the team about design patterns, team style, and overall best practices
- It catches mistakes and redundancies that were not identified via testing
Now that you’ve completed your first project for this class, you will participate in code review with your soon-to-be tIMDb partner. In addition to practicing reviewing and understanding another person’s code, we expect you to treat the review process as an exercise in collaborating professionally with colleagues. To that end, your grade on this assignment will be a combination of a) thorough engagement with your partner’s code and b) professional feedback and responses to feedback. Exactly what this means is laid out in the Guidelines section.
- Understand the code. Before you start reviewing, briefly read through your partner’s code. Make sure you understand what each component is doing and how those components work together.
- Check for objective code problems. These primarily include bugs and test failures. An objective problem is one that might be considered “blocking” in an industry setting – that is, until it is fixed, the code shouldn’t be integrated into the final codebase. When you notice one of these problems, highlight the specific line of code and the problem caused. If it’s a small change (like a missing paren, you tell them they’re missing a paren. If it’s a more complicated problem, it is sufficient to clearly explain what caused it.
- Check for test coverage. In general, all of the code in a project should be tested. If you see any gaps in test coverage, note them in the appropriate location (ie, where you think those tests should be in the code). You do not need to provide new test cases yourself. This can be easily spotted using the Jacoco report.
- Check for subjective code decisions. These primarily include architectural choices and code clarity. A subjective problem is one that might be considered “non-blocking” in an industry setting – that is, the code works perfectly well as is, but the reviewer has personal design preferences that they’d like to bring up to the author. When you notice a design choice like this, highlight the relevant block of code (if possible). State your position and weigh its pros and cons. Remember, you are not telling your partner what they should have done, you are providing them with an alternate perspective that you believe would improve code readability or maintainability. Your comment should be well-reasoned: after reading it, your partner should understand why you made your recommendation.
You will use Github’s pull request (PR) feature to write and hand in your code review.
You should have received a link to the repo containing your partner’s code. You should be able to access
the repo at
https://github.com/cs032-2020/stars-<your partner's GitHub username>.
You should then click on the pull requests tab and open the pull request that was merged
review branch, not the
If you weren't matched with a partner, haven’t received an invite to a repo,
or the repo you were assigned does not contain a pull request, please email the HTAs!
We want to ensure that this process goes as smoothly as possible for you all!
Once you’ve opened the pull request, you should see four tabs: “Conversation,” “Commits,” “Checks,” and “Files Changed.” The first of these, Conversation, is a space in which reviewers (you) and authors (your partner) can leave general comments about the PR. Click on this tab and scroll down to the bottom. You should see a textbox. Use it for comments that are too broad to fit into a single file or block of code.
Under the Files Changed tabs, you will be able to see all of your partner’s code sorted into files. Typically, pull requests in industry will contain small changes to existing code rather than a large and entirely new project. In those cases, you’d see the “diffs”: additions and subtractions to existing code that the PR author wants to make. For our purposes, however, you will just see all of your partner’s code.
Use the Jump to… menu to navigate across files. To leave a comment on a code block, move your cursor to the beginning of the line until you see a blue plus button appear. You can either click on the plus to leave a comment on that line or click and drag to leave a comment on multiple lines. This is the best way to comment specifically on parts of your partner's code.
For this code review, we would like you to focus on 3 components of Stars: the KDTree, the REPL, and the CSVParser. Feel free to venture outside of these three areas as well. For each component, you should include the following:
- Comments for any objective code problems you find. These comments must be on the line or lines in which you have identified the problem. We do not expect you to identify all bugs, but we do expect that you at least note significant ones.
- Comments on test coverage for both system tests and unit tests. They should assess test coverage and identify any areas of the code that were not tested. Leave these comments on the README for system tests and on the last line of the relevant file for unit tests. As before, if you think that the tests provide adequate coverage, say so.
- Comments on subjective code decisions. Identify areas where you believe your partner’s code is successful and where it could be improved. Since these choices are subjective, you can also write comments that evaluate the pros and cons of these choices as well as what you took away from looking at their code if it is different from yours. Although it is not standard industry practice to do so, we’d like you to get experience analyzing the design of others’ code.
There should be at least six comments total on your partner’s code. There should be at least one comment about objective code problems, at least one comment about test coverage, and at least three comments on subjective code decisions. Depending on the nature of the comment, you can either leave them on the lines in the code that you are referring to, the last line in a relevant file or section, or on the Conversations tab. Note the use of the word relevant here: there should be a connection between what you are commenting on and where your comment is.
Ethics Readings : Accessibility
Each Code Review assignment will have a corresponding reading and reflection. The topic of this week's reading is Accessibility. You have recently been thinking about final project ideas, and as Tim has discussed in lecture, you cannnot assume that the average user of your product will be the average CS32 student. Rather, you must see how you can broaden the scope of your product to reach as many users as possible. The readings and tools below will give you some insight in how to do this.
- What is accessibility?
- HTML: A good basis for accessibility
- Accessibility checklist (use when developing)
- WAVE Web Accessibility Evaluation Tool (test websites for accessibility)
- AChecker (test websites for accessibility)
- WebAIM Color Contrast Checker (tests colors for appropriate contrast)
Read the three readings and pick two websites you’d like to investigate and test them using both WAVE and AChecker. Then answer the two questions in the Google Form below. We don't care about the length of your resonse; rather, we want to see that you genuinely put in your best effort.
- What did you learn from the readings? What are some inclusive software engineering practices that were new to you, and how will you incorporate them in this course (and in your career!) moving forward?
- How did the websites you picked do on accessibility? Were you surprised by the results of your test?
To hand in, first submit yours code review comments on Github. Your partner should now be able to see your comments, and so will we. You can formally submit using this Google Form. Please link the pull request you left comments on as well as fill in your reading responses. The code review and readings Google Form is due by 11:59 PM on Sunday, Febuary 23rd. You may NOT use late days on code reviews and readings. Every day late will result in a 20% deduction.