Here’s an example code review based the roughly the “average” code that we’ve received thus far for week 1. I’ve left out some specific feedback that hints at design decisions that are better than others for week 1.
I am reviewing specifically with code style and performance in mind.
I am relying on the functional testing suite to confirm that you hit
all of the various corner cases (having a case where ll is NULL,
out of bounds indicies, etc).
In order to ease review burden, I reuse responses across code reviews,
adding them as needed. If the review feedback feels like it was copied
and pasted together, that’s why.
Code reviews OFTEN end up with substantial feedback, especially for new
engineers and computer scientists. Do not take the feedback personally
or as some sort of signal that you did a “bad job”, or any other such
non-sense. Some of my first professionally written perl code (yuck!)
had around 100 comments on things to improve prior to my own checkin,
some functional, some style, some preference of things to change
by the person that would eventually own the code, and I’m sure
other categories as well.
If there is something in this review that you do not understand or
otherwise needs clarification, please send email to <email address redacted to avoid spam>!
The code successfully compiles on my machine and passes all functional
tests.
Your code does not have a license at the top of one or more files.
Generally speaking (as an engineer, not a lawyer), code that lacks a
license can be used for any purpose, and can be published without
limitation. This is almost certainly at odds with the strategy
any business would use for intellectual property; even intellectual
property released as open source tends to include a license to limit
any sort of liability by use of their code.
The code provided to you in the repository is an exception, solely
so I can provide this sort of feedback.
It is unclear whether the LICNESE file within the repository,
which contains information about the “MIT License”, covers code that
lacks an explicit license at the top of it, so please include one
in your next submission. Simply copy and paste the contents of
the LICENSE file (and update the copyright to contain your name,
as it is YOUR intellectual property) next time.
Questions about intellectual property generally should be asked
to a lawyer or other legal professional who is knowledgeable
about the laws in your jurisdiction.
There are very few comments in the code.
Commenting is a personal choice often coming down to style, but
at a minimum I like to see comments for the following cases:
- Brief explanation of what each function does.
- Brief explanation for any logic that is non-obvious,
stating what it does and why it is necessary. - Any non-obvious design decisions.
Chris Feilbach's Blog 