This is the first in a series of guest posts by Andrew Tate, a writer turned software developer at Animalz. He has been using using PullRequest code review to help improve their first product, Revive, and learn more about development.

1. 3 Lessons Learned from My First Week of Code Review
2. Learning Security One Code Review at a Time
3. Solving Problems Collaboratively Through Code Review

Last week, the marketing agency I work for signed up with PullRequest. I was nervous.

I am a new developer, and 2019 has been my first year in this role. As a company, we are expanding out from just providing services to building products that help other marketers. I had some previous programming knowledge, so I was eager to learn a new skill and start building products.

Our first product launched in July, and has thousands of users. I built it from scratch with only cursory input from other people. My code was functional, but without anyone else looking over it consistently, I could never be quite sure I hadn’t made any glaring errors.

We wanted to make my code and our products more robust. I also wanted more opportunities to learn. External code review made sense—seasoned developers looking at the code, helping identify errors to make the code more robust, and giving me insights into how more experienced people think.

But after this first week of reviews, code review hasn’t just given me simple tactical advice about the lines I was writing. It has given me a different mindset. It is already making me think differently about the way I write code and the way I think about product. Here are three ways it has already improved me as a developer.

Code review forced me to explore the sad paths

Lots of possible exceptions that could be raised. Should likely be logged somewhere instead of ignoring.

—Code reviewer Alex K.

The comment above illustrated that, as a new developer, the ‘happy path’ is where I walk.

This is the code that generated the comment:

try:
    request.POST['newsletter-signup']
    c = pycurl.Curl()
    c.setopt(c.URL, 'https://api.convertkit.com/v3/forms/' + os.environ['CONVERTKIT_FORM_ID'] + '/subscribe')
    post_data = {'api_key': os.environ['CONVERTKIT_API_KEY'], 'email': email}
    postfields = urlencode(post_data)
    c.setopt(c.POSTFIELDS, postfields)
    c.perform()
    c.close()
except:
    pass

The code within the try block works fine as long as it follows the happy path. If everything goes to plan—the form returns properly, the API returns a 200 response—everything will work fine. But I hadn’t thought about the sad path, the path the code will go down if everything is less than perfect.

If the call to the ConvertKit API fails because the API key is invalid, there is nothing that deals with a 403 response. What if the server is down? There is nothing to deal with a 504 response or even a timeout. Anything other than the expected response will fail silently, and, as the reviewer comments, I am just ignoring it. The failure will just keep on happening in the background with me oblivious.

This has an impact beyond just bad code. This code signs people up to an email list. If something goes wrong, they won’t be signed up and neither they nor I will know it. A lack of thought in the code leads directly to a bad user experience.

Following the happy path is common for new developers and is exactly the kind of thing senior developers counsel against. Having someone knowledgeable look over the code at this early point allows me to go back and rethink not just the code from this PR, but it also makes me think about all of the sad paths that are waiting for my users. It is strengthening the code and strengthening me as a developer.

Code review made me think about who is reading my code

“LGTM! good idea to change strings to bools like this”

—Code reviewer Alex K.

This comment came because I had changed:

first_time = 'yes'
if first_time == 'yes':
    search_query = prefix + search_url
    first_time = 'no'
else:
    search_query = search_query + next_sites + search_url

to:

first_time = True
if first_time:
    search_query = prefix + search_url
    first_time = False
else:
    search_query = search_query + next_sites + search_url

The first is kinda embarrassing, and I knew it. Which is why I changed it.

The benefits of code review don’t just come from the review. They come from the change in attitude. This is just a minor example, but I knew that the former was ‘bad code.’

Code review is a forcing function. The mere act of having someone else look at your code forces you to think a little bit more about what you are doing. This takes two forms:

1. The narrow form is the one here. Before submitting a pull request, I now want to go through and make sure there aren’t any obvious issues. I know that a bool is better than a string in this scenario, so I should make sure I make that change. I want to make the initial implementation better to head off the comments.
2. The larger form is a follow-on from the exceptions example above. Though it is great that someone forced me to think about the sad paths of my code, feedback on the right exceptions to raise and the right way to structure a try/except/finally block would have been even better. But I didn’t give the reviewer that opportunity because I hadn’t included any exceptions. If I had thought more about what I really needed help with, I could have raised exceptions and gotten feedback on them. I missed that opportunity.

There is an added bonus here that comes with external code review: this was someone unknown to me. I don’t want to make mistakes, but if I am making them, it is easier to have a stranger point them out. I can feel secure in submitting PRs to build a better product and know the conversation around improvements is going to be non-combative and constructive.

Code review taught to think deeper about the final product

“The implementation should take care to have timeouts when fetching the URL content, validate the content size of the URL is of reasonable limits, and make attempts to not store any data retrieved from the URL. These can all lead to security issues like DDOS, memory overflow issues, etc.”

—Code reviewer Shahrukh S.

An incredibly helpful comment from just a small piece of code:

if form.is_valid():
    pass
url = request.POST['url']
spider(url)

A limiting factor for new developers is unknown unknowns. All the stuff that I just don’t know, I don’t know. This is an example of such a situation. I wasn’t thinking about security issues as I was building this out. If the first reviewer is teaching me about sad paths, this reviewer is teaching me about evil paths—how someone can manipulate my code.

This lesson then isn’t just about the code itself. Like the examples above, the next step here is to start building out the framework to mitigate these problems. The lesson is really about how to build a product on the whole. Am I thinking comprehensively about the security of the users? Am I thinking about what might take the entire service offline? The truthful answer is no. But building a product that survives in the real world requires this level of thought.

All developers, not just neophytes, can get caught up in a single function and lose sight of the entire product being built. I am just building small products, but this is a hundredfold problem when you are building something at scale. This code review makes me understand that, no matter what I am coding at any given time, it fits within the whole. And if I am not thinking of that whole, I could be making serious mistakes.

Just a few reviews have given me bite-sized learnings

This wasn’t a lot of code reviewed. I am still trying to grasp the mechanics of code review. What is the right PR size? What is the right amount of back-and-forth? What is the right cadence? Over the next few weeks I’ll dive deeper into some of these issues as I build out our next product. I will show how I am learning from code review in real-time and how the product is getting better because of it.

But I’ve already learned a lot. These small, bite-sized PRs allow for small bite-sized lessons. They are more specific than a tutorial or Stack Overflow answer on the same subject. They relate directly to my concerns and the issues present in my code.

As a single developer at a marketing agency, I’m limited in what I can learn. Having no one else to look at the code isn’t just bad for the product; it is bad for me. Whether it is nitpicking my code for mistakes in indentations or pointing out that I’m opening myself to DDOS attacks, I am learning something new from every review. Having multiple senior developers go through your code is an obvious boon.

The nervousness is still there, but now I look forward to getting the email from the reviewer teaching me more. As long as I am getting better with each PR, and the products I am building are becoming more robust, it is completely worth it.