Code review

Why do code review?

Code review is simply getting someone to check your code before you it gets included in the Pioneer code proper to make sure that the code is of a good quality, is consistent with the rest of the codebase and takes Pioneer in the direction that the community wants it to go.

When does it happen?

Code review can happy at any time, whenever you like. For small items of work it will get at least one review when it is submitted for inclusion (via a pull request). For a large development you may wish to find to someone to review your code periodically, to make sure you're on the right track. If you're starting to feel a bit unsure about what you're doing, that's probably a good time to ask someone!

You don't have to do this of course, but be aware that the reviewers don't particularly being asked to review hundreds or thousands of lines of code changes that appear out of nowhere. You face a good chance of having your code rejected outright or ignored completely, and that sucks. So if you're going to do something massive, at least check in at the start to make sure someone knows about it.

Who can review code?

Anyone that wants to! Anyone can use the review tools on Github to read and comment on code while its under development or test pull requests.

In the case of a pull request, someone with commit access needs to do the final sign-off (typically, but not necessarily, at the same time as merging). If you don't have commit access you can still help make life easier for that person by leaving comments and working with the original submitter.

I have commit access, does my code need to be reviewed?

No, you have commit access for a reason - you've proven that you can be trusted to be a good steward for the project. You should feel free to change whatever you want or need to, but as you do you should be considering all the points normally covered by a review. Self-review, if you like.

That said, no person is an island. Be sure to ask for help if you need it, even if its just a feeling that something isn't right.

What gets reviewed?

Every single change is a candidate for review, no matter how small. Most review will happen in a pull request, but you can be reviewing code by reading the commit history or even by reading code as you're working on it.

Here are some good things to look out for:

• Bugs, logical errors, mathematical errors, typos (any simple, isolated code error). These are easy. If you find a bug during testing or by looking at the code, either fix it or tell the submitter to fix it. It's easy because everyone agrees that bugs need to be fixed.

• Uncertainty about some code logic or behaviour ("Is this maths right?" "Did the submitter intend this weird behaviour?"). These are easy: Ask the submitter (obviously). Explanatory comments are usually the "fix" here.

• Code style errors (braces, whitespace, naming conventions, etc). Same as bugs: fix it, or tell the submitter fix it. Not everyone agrees about the importance of code style, or perhaps even what style we should be using (particularly since the existing codebase is not totally consistent), but we have a code style guide and we want everything to conform to it. If there is some aspect of style not covered by the code style guide on the wiki, amend the guide and tell the submitter to fix their style. Yes, style is subjective, and it's not the submitter's fault that they didn't use the "correct" style, since it may not have been clear what was the "correct" style when they made the PR, but its not worth arguing about - the time and energy to discuss it costs more than just fixing the code.

• Messy git history. Fix it or ignore it. The trouble with telling the submitter to fix it is that many of our contributors don't have enough confidence wth git to do it right and the ones that do tend not to submit messy histories in the first place. So its not worth arguing about. Just do the best you can to keep your own history tidy, but if its not working out don't stress too much about. If you really need help, talk to someone that can drive Git well; they can help you out.

• Game design uncertainty ("Do we want this feature?", "Do we want it to behave this way?"). For the most part you should choose what you think is best. You should be paying attention to what the other developers and the players are asking for and be aware of what other developments are happening, and from there be able to make a reasonable decision. If you don't know, ask someone who does. But remember that you should not reject an idea simply because you prefer it a different way. Of course your opinion matters, but try to balance everyone's needs as much as possible. (This is hard. Please ask for help).

• Code structure problems ("We want the feature, but not implemented like this"). Structural problems can take a significant effort to fix and they make future code maintenance harder. They're the ones that often end up needing to be ripped out or rewritten later. The difficulty is that its often a ridiculous amount of effort to do something the "right" way, especially if it relates to a very old piece of code. The rule of thumb then is to permit mess if its necessary, but do the best you can to keep it contained. Remember that you're now adding to the cleanup work that someone else will have to do in the future.