Pull Requests
As mentioned before, we have a feat->dev->main type workflow of our branches, hence you will need to make pull requests for code to get into production.
When making a pull request, please title it with your branch prefix, then a small introduction of the purpose. Then the body should be a descriptive brief on what the branch implements/changes about the code base. Their changes will also need to be reviewed by at least one (maybe two) members of the team before being merged.
For example, the branch /feat/user-score-graph might end up with a pull request looking like;
feat: graph visualisation of user's score progression
Implements a graph to visualise the history and progression of a users scores...
Users can navigate tourl.com/my/score/pageto see this graph...
Here we borrow some wise words from Michael Gribben.
The purpose of code reviews is, as far as I can tell, to: Collectively agree that the proposed solution is actually the most appropriate solution. Side note: by collectively agreeing, it also shares responsibility between authors and reviewers in case something goes wrong. It helps build a blameless culture since no ONE person causes the product to crash. Ensure only ‘good’ code is committed. Provide feedback on what is ‘good’ code. Communicate code changes to others.
So a reviewer must be both a mentor and a skeptic. I’ll start with what it means to be a mentor.
Firstly, be cautious that you are reviewing YOUR code; it’s THEIR code. Humans tend to be protective about things they do so be nice. Instead of saying: Instead of “Don’t do this”, say “Have you considered X if you do this?” Instead of “It would be bigger to do X”, say “An alternative is X. What do you think?”
In short, ensure code reviews exhibit psychological safety.
On a similar note, beware the narcissism of small differences. It’s super tempting and super counter productive to get bogged in nitpicks, almost always because of style. Nitpicking because of stylistic preferences really should not happen because: There should be automated linting to enforce consistent style. There should be an agreed upon style guide. So you should nitpick style by linking which parts of the style guide are being violated; there’s no beef, it’s the style guide making us do this, not my personal. And actually link them so they learn and don’t feel micromanaged.
Think of the codebase like a street full of red houses. If the author comes along and wants to build a blue house, you should ask them to paint it red instead. However, if the street is empty let them paint the house blue! It’s a complete waste of time forcing others to conform to your personal style preferences.
You’re a mentor, so try to teach something! If there is a library function that does something they’re doing but better, tell them about it! Are they confused about something? As the reviewer, you likely know the history of that part of the codebase so clarify things!
Try to reduce back and forth. If you have something to say, explain it. If you ask a question, explain why you are asking that question; it will save a lot of time. Don’t be like:
Why are you doing that? Because X. But it can be done like Y instead. I considered Y but found it didn’t work. Why didn’t Y work? Because Z. Makes sense
It would be much faster for the conversation to go like:
Why are you doing that? Have you considered Y? I considered Y but found it didn’t work because Z. Makes sense
When communicating asynchronously, back and forth can stretch over DAYS!!!
As the skeptic, only approve code when you ABSOLUTELY know what’s going on. If you aren’t the right person to review it, ask the right person to review it! If you don’t understand something, ask clarifying questions! Remember, you want to ensure only ‘good’ code is committed so if you’re not sure, don’t let it be committed!
As the skeptic, ensure the problem is actually a problem. It doesn’t always happen but sometimes there are solutions without a problem. Sometimes people have narrowed their scope writing excellent code for a verbose solution when a much simpler solution exists. If you don’t understand the problem and the solution, ask clarifying questions! And TRY TO SOLVE THE PROBLEM YOURSELF! Your feedback should be informed whether or not you hit upon the same solution.
As the skeptic, test assumptions! Is there an assumption in the input that might not be true? If in doubt, point it out!
As the skeptic, investigate if the code is silently fucking itself up. Find the expensive operations (e.g. connecting to a database, API calls) and understand the performance implications; are they acceptable? What are the hot/cold paths and have they been optimized appropriately? Do they have a good idea about the expected load of each API endpoint? Just doing back of the envelope calculations is sufficient since people tend not to do them. You don’t always need to go into such depth, but if you have doubts you should!
As the skeptic, investigate if the code change silently fucks something else up. For example, adding a high latency endpoint will also raise the latency of other endpoints on the same server since requests are being served by the same thread pool. In that case, do some back of the envelope calculations to see if the higher latency is acceptable or if the endpoint should be on a separate server. Again, you don’t always need to go into such depth, but if you have doubts you should!
On the flip side, you should write code conscious of how it will be reviewed. Importantly, keep your code reviews to an appropriate size. At the risk of stating the obvious, a 10 line change will be reviewed faster than a 1000 line change. So if you want fast feedback, split changes so they can be reviewed fast! A general tip in software engineering is that data is more important than code, not least because it’s EXTREMELY hard to change data but very easy to change code. You can split reviews into database schema changes and API changes, which tend to be very small but very important, and leave distracting implementation details behind in another review.
As a caveat, however, I personally believe that all code committed should be compilable and tested. Don’t ask someone to review code that can’t be compiled (i.e don’t split implementation into reviews that don’t compile). The reviewer might want to checkout the branch and run it themselves. Don’t ask someone to review code that isn’t tested (i.e. don’t split reviews into code vs tests). The tests prove to the reviewer that your code actually works.
I have heard contradicting advice for the above so take it as you will.
The main struggle in communication is that people possess different contexts. You know a lot more about the code you wrote than the reviewer. To avoid confusion and cut down on needless back and forth, write a description that provides the reviewer with the same context as you. Descriptions should go from high level to low level so the reviewer can orient themselves. For example: What’s the problem? What’s the solution? What are the pros/cons What alternatives were considered? What are their pros/cons?
Remember reviewers tend to review a lot of code so might lose track of your changes so when writing a set of related code changes, it’s best to link them in order and provide a description of the overall set at the beginning of each description (which they can just ignore if they know what’s going on).
The easiest way to do the above is to review your own code! Empathy is essential; see how this is the flipside of ‘try to solve the problem yourself’! Point out anything strange in advance and provide an explanation of why you did so. It will help the reviewer understand what’s going on and shorten the back and forth.
The above advice is all very general so to get into some Java specifics, I recommend Effective Java by Joshua Bloch. My extremely brief summary is:
Avoid using new Something() because you lock yourself into using a particular object. Use factory methods and dependency injection instead. Dependency injection is also crucial to testing things in Java.
The builder pattern is great. If Java was a good language, objects have builders by default IMO.
You can do really nifty things by using private constructors like singletons.
USE IMMUTABLE OBJECTS!!! Everything is WAY easier! Concurrency becomes trivial. You can save HEAPS of memory (hehe) by using singletons.
Related to the above, use final whenever possible.
Use static whenever possible since it saves memory.
Favor composition over inheritance. Inheritance is fundamentally flawed (when subclassing concrete classes, you either have to violate Liskov’s Substitution Principle or break the mathematical notion of equality; there is no middle ground).
Only use inheritance if the class’ sole purpose is inheritance (e.g. a visitor class of some data structure). Abstract classes are preferred over concrete classes because of the above.
Interfaces are MUCH better than abstract classes since you don’t lock yourself into rigid inheritance hierarchies.
In fact, you can have both an interface and an abstract skeletal implementation of it.
Enums are cool so use them. You can use switch statements without fear.
Use lists over arrays since lists are invariant while arrays are covariant.
Generics are cool.
For each loop are better than for loops.
Prefer primitive types to boxed primitives since boxing/unboxing is relatively expensive.
Java’s functional features like Streams and Optionals are pretty neat, although they are kind of a hack job and in some ways broken. You can’t put a null in an Optional that sounds like a good thing but it also means it isn’t a monad since null for some weird ass type :( Nulls really were a mistake. Side note: Use @nullable to document when null is a valid value for a type.
Try to return collections from streams to remove bloat.
Checked exceptions sound like a good idea but lead to super bloated code.
I haven’t read the concurrency and serialization sections yet but Java serialization is bad.
It’s not in the book but you can also use var to implicitly type which leads to much less bloated code and more versatile if you later change types.
