Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add first draft of new blog post - Code reviews: getting started #21

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@thibaudcolas
Copy link

thibaudcolas commented May 3, 2018

Related: thibaudcolas/cookbook#1

This is a first draft of a new article for Codemate's blog. It will be published on the current blog/website, but I thought it could be nice to review it via GitHub.

Finally, this last resource briefly mentions another reason why I think code reviews should be part of everybody’s toolset: you can learn A LOT from them. They are a great place for the team to share their knowledge and learn:

* How other people would approach and solve a given problem.
* General stylistic or architectural guidelines to follow for the specific project. Those are defined elsewhere (eg. in a styleguide or architecture principles document) but propagate well via code reviews.

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor
  • s/styleguide/style guide
  • s/eg./e.g.

1. [Code review guidelines](https://github.com/CodemateLtd/cookbook/blob/master/CONTRIBUTING.md). We usually put those in a `CONTRIBUTING.md` file at the root of the project.
2. [A pull request template](https://github.com/CodemateLtd/cookbook/blob/master/.github/PULL_REQUEST_TEMPLATE.md). This is where you state that the team does code reviews, that they will be enforced, and that pull requests should be authored accordingly. Ours is simple but gets the job done.
3. [Useful resources about how to do this well](https://github.com/CodemateLtd/cookbook/blob/master/CONTRIBUTING.md). Code reviews are a solved problem, there are great resources out there to take inspiration from.

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor

Code reviews are a solved problem, there are great resources out there to take inspiration from.

Feels clumsy, maybe try ...are a solved problem, and there are great resources...

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 15, 2018

Author

Thank you, will use your wording as-is.

* [Code Reviews: Just Do It](https://blog.codinghorror.com/code-reviews-just-do-it/) by Jeff Atwood, my absolute favorite.
* The Flutter project has good [code review guidelines](https://flutter.io/design-principles/). I particularly like how they talk about the benefits of code reviews. This is a large-scale open-source project so their particular workflow might not fit perfectly for proprietary work, but still worth having a look.
* Karumi has really nice pull request guidelines and templates as part of their [project quality assurance guidelines](https://github.com/Karumi/project-quality-assurance). I would really like all the teams I’m part of to have such high-quality guidelines.
* Finally, Michael Lynch’s two-part article on doing code reviews [like a human](https://mtlynch.io/human-code-reviews-1/) is well worth a read. I think it is probably not necessary to get started, but if your team needs to improve how they communicate as well then this contains good advice.

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor
  • Maybe highlight doing code reviews like a human instead of like a human? I was going to comment that you missed that awesome article but found this when doing a Ctrl+F with the url for that article. :)
  • The last sentence is hard to parse, maybe add a comma: if your team needs to improve how they communicate as well, then this contains good advice

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 15, 2018

Author

👍 that last sentence sucks, I'll try to see if I can get rid of it.


### Convince the boss

In practice, the first step is to “convince the boss”. Show them those great [metrics on the effectiveness of code reviews](https://blog.codinghorror.com/code-reviews-just-do-it/). Talk about how reviews improve [quality _in terms of business value_](https://www.codemate.com/quality-in-terms-of-business-value). Their main scepticism might be in the cost of having more people working on an otherwise identical amount of changes – this is a good time to talk about the economics of fixing software defects:

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor

s/scepticism/skepticism

This comment has been minimized.

@thibaudcolas

thibaudcolas May 3, 2018

Author

Is american english the norm in Finland?

This comment has been minimized.

@roughike

roughike May 4, 2018

Contributor

Ah, my bad! I thought it felt weird and maybe not a word at all; silly me.

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 15, 2018

Author

👌 it's probably not a great choice of word for what I'm trying to say, I'll replace it with "concern" (people might have concerns even if they aren't skeptical).


## Do it!

Still sceptical? The best way to know whether code reviews will work for your team is to try them. All of the major hosted Git platforms have pull request features with code reviews built-in, you can try them out and see for yourself how many defects you will catch before they even get merged. At Codemate, a good example of this in practice is [my review of Iiro’s inKino project](https://github.com/roughike/inKino/issues/52). I didn’t catch any bugs, but we certainly all learned a lot 😄 – for me, [Flutter](https://www.codemate.com//considering-flutter), and for him a web developer’s perspective on UI code architecture.

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor

s/sceptical/skeptical

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 15, 2018

Author

Fuck I use that word a lot.


<iframe width="560" height="315" src="https://www.youtube-nocookie.com/embed/56mETnrByBM" frameborder="0" allow="autoplay; encrypted-media" allowfullscreen></iframe>

If you like numbers, another great article on the topic is [Code Reviews: Just Do It](https://blog.codinghorror.com/code-reviews-just-do-it/) by Jeff Atwood. You should definitely read it. I especially like its excerpts from [Code Complete](https://www.amazon.com/exec/obidos/ASIN/0735619670):

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor

I feel like "you should definitely read it" doesn't really tell the reader why they should read it or why is it a valuable read.

Suggestions:

  • tell the reader why the article is such a good read
  • AND/OR drop the "You should definitely read it" statement altogether

This comment has been minimized.

@thibaudcolas

thibaudcolas May 3, 2018

Author

The reason is "If you like numbers", implying that the article contains statistics. If that's not clear enough I should probably reword, yup.

This comment has been minimized.

@roughike

roughike May 4, 2018

Contributor

In that case, maybe:

Another great article on the topic is Code Reviews: Just Do It by Jeff Atwood. If you like numbers, you should definitely read it.

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 15, 2018

Author

👌 edited


Developers tend to think of productivity in terms of “how fast can I write the code for this new feature”. We also tend to say “I write code” when describing our jobs. In practice though, professional software development is a matter of understanding the code that is already there: we simply spend much more time reading than writing. Productivity thus depends more on the overall code’s quality and maintainability, rather than the skills of individual contributors making changes.

Joel Spolsky puts it neatly:

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor

Who is Joel Spolsky? Why does his opinion matter?

Maybe worth introducing him to the reader.

Joel Spolsky, founder of Trello and Stack Overflow, puts it neatly:

or maybe:

Joel Spolsky, a widely respected software engineer with decades of experience, puts it neatly:

This comment has been minimized.

@thibaudcolas

thibaudcolas May 3, 2018

Author

I don't think this is worth the extra sentence – either you know who he is already (because he's famous) or you don't and you can click the link which is much more informative than I can ever be.


Joel Spolsky puts it neatly:

> [It’s harder to read code than to write it](https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/).

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor

This transition from Joel Spolsky to John Papa feels kinda quick. Before transitioning to the next guy, it would be beneficial to tell some anecdotes here why Joel Spolsky is right, or what was the article about.

I wrote some quick ideas down below. Please feel free to cherry pick pieces, or ditch all of it and use them as inspiration for something completely new!

In the article above, Joel explains why he feels that rewriting big applications from scratch is never a good idea. He also goes into detail how to combat smelly codebases and what are alternatives to big rewrites.

At the time of writing, Netscape had just shipped a public beta of their brand new major release. Since they decided to do a full rewrite which took them 3 years, their competitors were able to gain leverage and Netscape's market share declined. Joel argues that such big rewrites should never be done, and we should focus on code quality and gradual improvements instead. Although the article is now over 18 years old, it still holds true to this day.

From our anecdotal experiences, if we pay attention to code quality right from the beginning, we'll thank ourselves later. If two persons or more understand and find a piece of code well written now, chances are that it will be more pleasant to maintain later.

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 15, 2018

Author

I quite like it as-is, but I can see why if you don't know those people that well it feels weird to name-drop people in that quick of a succession. And it's not fair to expect the audience of this post to be familiar with those names.

I'll reword to rely less on the names, but I won't add new content summarising either of those resources as I think it breaks the flow of this article.

Show resolved Hide resolved src/pages/code-reviews-getting-started/index.md Outdated
John Papa recently gave a great talk about what code readability means in practice:

<iframe width="560" height="315" src="https://www.youtube-nocookie.com/embed/56mETnrByBM" frameborder="0" allow="autoplay; encrypted-media" allowfullscreen></iframe>

This comment has been minimized.

@roughike

roughike May 3, 2018

Contributor

I would like to have a short recap about the main points of the video here.

Some key points to consider, don't have to use all of them:

  • speaker is John Papa, developer advocate working for Microsoft
  • you read code more than you write it, according to Robert C. Martin the read/write ratio is "well over 10:1"
  • what makes it worse, like Joel Spolsky said earlier, reading code is harder than writing
  • favor fast and effective communication over clever and fast code
  • clean, readable code helps to avoid rewrites
  • use consistent and meaningful names in code
  • self-documenting code is better than commenting everything
  • write dirty code that works; then clean it

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 15, 2018

Author

I think the video would be a good watch in its own right, and I don't want to break the flow of the article with too many recaps, so I'll leave this as-is.

@roughike
Copy link
Contributor

roughike left a comment

Generally looks really cool!

Writing can be a subjective topic, so I'll just say that here are some of my opinionated observations that came to mind when reading the article.

I also feel like the article introduction block needs some love, but then again I don't have any improvement suggestions so I didn't comment anything there ¯\(ツ)

@iiska
Copy link

iiska left a comment

Overall I like this article and that one point about mentioning tooling and formatting is something you could decide if it fits in the scope or should be an another story. 😃

2. [A pull request template](https://github.com/CodemateLtd/cookbook/blob/master/.github/PULL_REQUEST_TEMPLATE.md). This is where you state that the team does code reviews, that they will be enforced, and that pull requests should be authored accordingly. Ours is simple but gets the job done.
3. [Useful resources about how to do this well](https://github.com/CodemateLtd/cookbook/blob/master/CONTRIBUTING.md). Code reviews are a solved problem, there are great resources out there to take inspiration from.

Take all of these, sprinkle some LGTM ([Looks Good To Me](http://knowyourmeme.com/memes/lgtm)) images on top when approving changes, and you’re good to go. Code reviews can feel tense at times, those silly images set a more relaxed tone, and can ease the relationship between reviewers and reviewees. Plus, a nice picture is a great reward for good work!

This comment has been minimized.

@iiska

iiska Jul 31, 2018

This is important even though looking for some nice pics in the middle of the busy things might feel tedious.

Another thing to preserve humane and friendly atmosphere I feel should be mentioned somewhere could go along the lines:

Be kind and let tooling do the nitpicking. Choose common formatting and enforce it via tooling (Prettier, linters etc). Let humans focus on logic, use cases, readability and learning.

Common formatting makes diffs easier to read and you don't really want to discuss tabs vs spaces on each review.

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 15, 2018

Author

👌 good point. I'll try to cover this around this "LGTM" part.

thibaudcolas added some commits Aug 15, 2018

@thibaudcolas

This comment has been minimized.

Copy link
Author

thibaudcolas commented Aug 16, 2018

Who wants to do another round of reviews, and give this a thumbs up or thumbs down?

Edit: seems like the Netlify integration has stopped working. Here is the rendered post: https://github.com/thibaudcolas/codemate-blog/blob/feature/blog-post-code-reviews-getting-started/src/pages/code-reviews-getting-started/index.md

@roughike

This comment has been minimized.

Copy link
Contributor

roughike commented Aug 17, 2018

I read it through couple times just now with fresh eyes, and...

g1463594554310385012

@thibaudcolas

This comment has been minimized.

Copy link
Author

thibaudcolas commented Sep 4, 2018

Updated to match the latest changes from the published version 🎉 https://www.codemate.com/code-reviews-getting-started/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.