Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Code Review And Why Are We Wrong About It And H...

asendecka
February 10, 2019

Code Review And Why Are We Wrong About It And How To Use It To Our Advantage

In this talk I take a look at code review as a tool in tech and how we can get the most out of this.

asendecka

February 10, 2019
Tweet

More Decks by asendecka

Other Decks in Programming

Transcript

  1. What is a code review? Goals of code review. What

    we can catch in code review? How wrong we are about code reviews?
  2. What is a code review? Goals of code review. What

    we can catch in code review? How wrong we are about code reviews? What we can do about this?
  3. What is a code review? Goals of code review. What

    we can catch in code review? How wrong we are about code reviews? What we can do about this? Ways to get the most out of code review.
  4. Modern Code Review a process that takes as input original

    source code and outputs accepted source code. “ ” Moritz Beller, Elmar Juergens
  5. Moritz Beller, Elmar Juergens Modern Code Review the original source

    code is a work that stemmed solely from the author, “
  6. Moritz Beller, Elmar Juergens Modern Code Review the original source

    code is a work that stemmed solely from the author, whereas in the accepted source code the author incorporated the reviewers’ suggestions “
  7. Moritz Beller, Elmar Juergens Modern Code Review the original source

    code is a work that stemmed solely from the author, whereas in the accepted source code the author incorporated the reviewers’ suggestions so that everybody is satisfied with the result. “ ”
  8. Moritz Beller, Elmar Juergens Modern Code Review the original source

    code is a work that stemmed solely from the author, whereas in the accepted source code the author incorporated the reviewers’ suggestions so that everybody is satisfied with the result. “ ”
  9. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions.
  10. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer.
  11. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness.
  12. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process.
  13. +def get_service_yaml(repo, service_name):
 + response = requests.get(makeServiceYamlUrl(repo, service_name), headers={'Accept': 'application/

    vnd.github.v3.raw', 'Authorization': 'token {}'.format(settings.get('github_token'))})
 + return response 21 22 23
  14. +def get_service_yaml(repo, service_name):
 + response = requests.get(makeServiceYamlUrl(repo, service_name), headers={'Accept': 'application/

    vnd.github.v3.raw', 'Authorization': 'token {}'.format(settings.get('github_token'))})
 + return response 21 22 23 + response = requests.get(makeServiceYamlUrl(repo, service_name), headers={'Accept': 'application/ vnd.github.v3.raw', 'Authorization': 'token {}'.format(settings.get('github_token'))})
  15. +def get_service_yaml(repo, service_name):
 + response = requests.get(makeServiceYamlUrl(repo, service_name), headers={'Accept': 'application/

    vnd.github.v3.raw', 'Authorization': 'token {}'.format(settings.get('github_token'))})
 + return response 21 22 23 makeServiceYamlUrl
  16. Code “Hygiene” Does the code pass linting and has consistent

    code formatting? Is it clear what given function/class/method does?
  17. Code “Hygiene” Does the code pass linting and has consistent

    code formatting? Is it clear what given function/class/method does? Do names reflect what code does?
  18. Makes a request to API. Extracts and formats results. Builds

    attachments out of results. Sets the value of the attachments on the object…
  19. Code “Hygiene” Does the code pass linting and has consistent

    code formatting? Is it clear what given function/class/method does? Do names reflect what code does?
  20. Code “Hygiene” Does the code pass linting and has consistent

    code formatting? Is it clear what given function/class/method does? Do names reflect what code does? ⛳ Do functions have one responsibility?
  21. Code “Hygiene” Does the code pass linting and has consistent

    code formatting? Is it clear what given function/class/method does? Do names reflect what code does? ⛳ Do functions have one responsibility? ✂ Could we split functions into smaller steps?
  22. Code “Hygiene” Does the code pass linting and has consistent

    code formatting? Is it clear what given function/class/method does? Do names reflect what code does? ⛳ Do functions have one responsibility? ✂ Could we split functions into smaller steps? Does code have unexpected side effects?
  23. Code “Hygiene” Does the code pass linting and has consistent

    code formatting? Is it clear what given function/class/method does? Do names reflect what code does? ⛳ Do functions have one responsibility? ✂ Could we split functions into smaller steps? Does code have unexpected side effects?
  24. +def can_be_displayed(publish_start_date): + today = datetime.date.today() + if today >

    publish_start_date: + return True + return False 51 52 53 54 55
  25. +def can_be_displayed(publish_start_date): + today = datetime.date.today() + if today >

    publish_start_date: + return True + return False 51 52 53 54 55 + if today > publish_start_date:
  26. 10th of February +def can_be_displayed(publish_start_date): + today = datetime.date.today() +

    if today > publish_start_date: + return True + return False 51 52 53 54 55 + if today > publish_start_date:
  27. 10th of February publish_start_date = date(2019, 2, 10) +def can_be_displayed(publish_start_date):

    + today = datetime.date.today() + if today > publish_start_date: + return True + return False 51 52 53 54 55 + if today > publish_start_date:
  28. 10th of February publish_start_date = date(2019, 2, 10) today >

    publish_start_date +def can_be_displayed(publish_start_date): + today = datetime.date.today() + if today > publish_start_date: + return True + return False 51 52 53 54 55 + if today > publish_start_date:
  29. 10th of February publish_start_date = date(2019, 2, 10) today >

    publish_start_date False +def can_be_displayed(publish_start_date): + today = datetime.date.today() + if today > publish_start_date: + return True + return False 51 52 53 54 55 + if today > publish_start_date:
  30. +def can_be_displayed(publish_start_date): + today = datetime.date.today() + if today >

    publish_start_date: + return True + return False 51 52 53 54 55 + if today >= publish_start_date: True 10th of February publish_start_date = date(2019, 2, 10) today > publish_start_date
  31. Edge Cases & Broken Logic Edge cases for ifs, fors,

    whiles and dates. Check Python gotchas.
  32. HTTP 414 URI Too Long https://our-internal-api.com/endpoint? fields=url,title,published_at,author, published_at,author,published_at,author, published_at,author,published_at,author, published_at,author,published_at,author,

    published_at,author,published_at,author, published_at,author,published_at,author, published_at,author,published_at,author, published_at,author,published_at,author, published_at,author,published_at,author, published_at,author,published_at,author, published_at,author,published_at,author, published_at,author,published_at,author…
  33. 17 18 19 20 21 22 23 24 25 26

    +DEFAULT_FIELDS = [‘url','title'] + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url
  34. 17 18 19 20 21 22 23 24 25 26

    +DEFAULT_FIELDS = [‘url','title'] + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url + url = url_concat(path, params)
  35. 17 18 19 20 21 22 23 24 25 26

    + query_fields = DEFAULT_FIELDS +DEFAULT_FIELDS = [‘url','title'] + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url
  36. 17 18 19 20 21 22 23 24 25 26

    + query_fields.extend(fields) +DEFAULT_FIELDS = [‘url','title'] + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url
  37. 17 18 19 20 21 22 23 24 25 26

    + query_fields = DEFAULT_FIELDS +DEFAULT_FIELDS = [‘url','title'] + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url
  38. id(DEFAULT_FIELDS) 4339573384 17 18 19 20 21 22 23 24

    25 26 + query_fields = DEFAULT_FIELDS +DEFAULT_FIELDS = [‘url','title'] + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url id(query_fields) 4339573384
  39. 17 18 19 20 21 22 23 24 25 26

    +DEFAULT_FIELDS = [‘url','title'] + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url +DEFAULT_FIELDS = [‘url','title'] Make it immutable!
  40. 17 18 19 20 21 22 23 24 25 26

    +DEFAULT_FIELDS = (‘url’,’title') + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url +DEFAULT_FIELDS = (‘url’,’title') Make it immutable!
  41. 17 18 19 20 21 22 23 24 25 26

    +DEFAULT_FIELDS = (‘url’,’title') + +def get_url(path, fields=()): + query_fields = DEFAULT_FIELDS + query_fields.extend(fields) + params = { + 'fields': ','.join(query_fields) + } + url = url_concat(path, params) + return url AttributeError: 'tuple' object has no attribute 'extend' + query_fields.extend(fields)
  42. Edge Cases & Broken Logic Edge cases for ifs, fors,

    whiles and dates. Check Python gotchas. Are exceptions handled correctly?
  43. 250 251 252 253 254 255 256 +try: + data

    = fetch_video_data(video_id) +except Exception as err: + if err.code == 404: + handle_not_found() + else: + handle_http_error(err)
  44. +except Exception as err: 250 251 252 253 254 255

    256 +try: + data = fetch_video_data(video_id) +except Exception as err: + if err.code == 404: + handle_not_found() + else: + handle_http_error(err)
  45. 250 251 252 253 254 255 256 +try: + data

    = fetch_video_data(video_id) +except Exception as err: + if err.code == 404: + handle_not_found() + else: + handle_http_error(err) + if err.code == 404: + handle_not_found() + else: + handle_http_error(err)
  46. 250 251 252 253 254 255 256 +try: + data

    = fetch_video_data(video_id) +except HTTPError as err: + if err.code == 404: + handle_not_found() + else: + handle_http_error(err) +except HTTPError as err:
  47. Edge Cases & Broken Logic Edge cases for ifs, fors,

    whiles and dates. Check Python gotchas. Are exceptions handled correctly?
  48. Edge Cases & Broken Logic Edge cases for ifs, fors,

    whiles and dates. Check Python gotchas. Are exceptions handled correctly? ➰ Are there unnecessary while loops?
  49. +day = datetime.date.today() +while not is_working_day(day):
 + day = day

    + datetime.timedelta(day=1) 43 44 45 +day = datetime.date.today()
  50. +day = datetime.date.today() +while not is_working_day(day):
 + day = day

    + datetime.timedelta(day=1) 43 44 45 +while not is_working_day(day):
  51. +day = datetime.date.today() +while not is_working_day(day):
 + day = day

    + datetime.timedelta(day=1) 43 44 45 + day = day + datetime.timedelta(day=1)
  52. +day = datetime.date.today() +while not is_working_day(day):
 + day = day

    + datetime.timedelta(day=1) 43 44 45 +while not is_working_day(day):
  53. 43 44 45 46 47 +day = datetime.date.today() +for offset

    in range(31): + day = day + datetime.timedelta(day=offset) + if is_working_day(day): + break +for offset in range(31):
  54. Edge Cases & Broken Logic Edge cases for ifs, fors,

    whiles and dates. Check Python gotchas. Are exceptions handled correctly? ➰ Are there unnecessary while loops?
  55. “Future-Self” Will Thank Me ♻ Is every line of code

    used? Check usage of third party libraries.
  56. Third Party Libraries Check library documentation. Check if library choice

    is consistent with the rest of the codebase. What’s the licence of the library?
  57. Third Party Libraries Check library documentation. Check if library choice

    is consistent with the rest of the codebase. What’s the licence of the library? Is it actively maintained?
  58. “Future-Self” Will Thank Me ♻ Is every line of code

    used? Check usage of third party libraries.
  59. “Future-Self” Will Thank Me ♻ Is every line of code

    used? Check usage of third party libraries. E Is solution performant?
  60. “Future-Self” Will Thank Me ♻ Is every line of code

    used? Check usage of third party libraries. E Is solution performant? Does optimisation hurt code readability?
  61. “Future-Self” Will Thank Me ♻ Is every line of code

    used? Check usage of third party libraries. E Is solution performant? Does optimisation hurt code readability? Look for security vulnerabilities.
  62. “Future-Self” Will Thank Me ♻ Is every line of code

    used? Check usage of third party libraries. E Is solution performant? Does optimisation hurt code readability? Look for security vulnerabilities. What is missing (tests, docs, metrics, logs, etc.)?
  63. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process.
  64. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Finding defects.
  65. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Finding defects. edge cases
  66. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Finding defects. edge cases programmatic errors
  67. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Finding defects. edge cases programmatic errors simple logic issues
  68. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Code improvements. edge cases programmatic errors simple logic issues
  69. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Code improvements. smaller functions edge cases programmatic errors simple logic issues
  70. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Code improvements. smaller functions naming edge cases programmatic errors simple logic issues
  71. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Finding alternative solutions. smaller functions naming edge cases programmatic errors simple logic issues
  72. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Finding alternative solutions. smaller functions naming edge cases programmatic errors simple logic issues
  73. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Knowledge transfer. smaller functions naming edge cases programmatic errors simple logic issues
  74. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Knowledge transfer. reviewers see the code smaller functions naming edge cases programmatic errors simple logic issues
  75. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Team awareness. reviewers see the code smaller functions naming edge cases programmatic errors simple logic issues
  76. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Team awareness. reviewers see the code smaller functions naming edge cases programmatic errors simple logic issues
  77. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Improving dev process. reviewers see the code smaller functions naming edge cases programmatic errors simple logic issues mandatory linting
  78. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. Improving dev process. reviewers see the code smaller functions naming edge cases programmatic errors simple logic issues mandatory linting
  79. Alberto Bacchelli, Christian Bird Goals of Code Review Finding defects.

    Code improvements. Finding alternative solutions. Knowledge transfer. Team awareness. Improving dev process. smaller functions naming edge cases programmatic errors simple logic issues
  80. 75% code maintainability M. Mäntylä and C. Lassenius. What Types

    of Defects Are Really Discovered in Code Reviews?
  81. 25% improve functionality M. Mäntylä and C. Lassenius. What Types

    of Defects Are Really Discovered in Code Reviews?
  82. 25% improve functionality M. Mäntylä and C. Lassenius. What Types

    of Defects Are Really Discovered in Code Reviews?
  83. A. Bacchelli, C. Bird, Modern Code Reviews in Open-Source Projects:

    Which Problems Do They Fix? There is a mismatch between the expectations and the actual outcomes of code reviews. “
  84. A. Bacchelli, C. Bird, Modern Code Reviews in Open-Source Projects:

    Which Problems Do They Fix? There is a mismatch between the expectations and the actual outcomes of code reviews. [...] review does not result in identifying defects as often as project members would like “
  85. A. Bacchelli, C. Bird, Modern Code Reviews in Open-Source Projects:

    Which Problems Do They Fix? There is a mismatch between the expectations and the actual outcomes of code reviews. [...] review does not result in identifying defects as often as project members would like “ and even more rarely detects deep, subtle, or “macro” level issues.
  86. A. Bacchelli, C. Bird, Modern Code Reviews in Open-Source Projects:

    Which Problems Do They Fix? Relying on code review in this way for quality assurance may be fraught. There is a mismatch between the expectations and the actual outcomes of code reviews. [...] review does not result in identifying defects as often as project members would like “ ” and even more rarely detects deep, subtle, or “macro” level issues.
  87. A. Bacchelli, C. Bird, Modern Code Reviews in Open-Source Projects:

    Which Problems Do They Fix? Modern code reviews provide benefits beyond finding defects. “
  88. A. Bacchelli, C. Bird, Modern Code Reviews in Open-Source Projects:

    Which Problems Do They Fix? Modern code reviews provide benefits beyond finding defects. “ Code review can be used to improve code style, find alternative solutions, increase learning, share code ownership, etc..
  89. A. Bacchelli, C. Bird, Modern Code Reviews in Open-Source Projects:

    Which Problems Do They Fix? Modern code reviews provide benefits beyond finding defects. “ ” Code review can be used to improve code style, find alternative solutions, increase learning, share code ownership, etc.. This should guide code review policies.
  90. ✍ Write an architecture proposal. Organize a critique meeting for

    bigger projects. Discuss (Slack, JIRA cards, GitHub issues) smaller tasks.
  91. If you involve people at the beginning, they are more

    likely to provide help as during implementation process. Architecture Proposals
  92. Move responsibility from a single individual to a team. If

    you involve people at the beginning, they are more likely to provide help as during implementation process. Architecture Proposals
  93. ✍ Write an architecture proposal. Organize a critique meeting for

    bigger projects. Discuss (Slack, JIRA cards, GitHub issues) smaller tasks.
  94. Improving code maintainability. Understanding and sharing best practices. Knowledge sharing/education.

    Building blameless culture. What code review really can help us with?
  95. How to promote blameless culture with code reviews? Let the

    author fix all the issues in their pull request.
  96. How to promote blameless culture with code reviews? Let the

    author fix all the issues in their pull request. ⏰ Get involved earlier in a process. Not when code is ready.
  97. Let the author fix all the issues in their pull

    request. ⏰ Get involved earlier in a process. Not when code is ready. Take time to explain suggested improvements. How to promote blameless culture with code reviews?
  98. Let the author fix all the issues in their pull

    request. ⏰ Get involved earlier in a process. Not when code is ready. Take time to explain suggested improvements. Require documentation. How to promote blameless culture with code reviews?
  99. README.md # _name of the service_ ## Point of contact

    and Slack channel ## Usage ## Running the service ## System ## Runbook ## Monitoring ## Documentation https://tinyurl.com/better-docs
  100. Let the author fix all the issues in their pull

    request. ⏰ Get involved earlier in a process. Not when code is ready. Take time to explain suggested improvements. Require documentation. Mind your language! How to promote blameless culture with code reviews?
  101. You don’t point something, because you are worried that author

    will feel horrible about it. Two dangers
  102. You don’t point something, because you are worried that author

    will feel horrible about it. You deliver your feedback in unhelpful and blunt way that make an author angry. Two dangers
  103. You don’t point something, because you are worried that author

    will feel horrible about it. You deliver your feedback in unhelpful and blunt way that make an author angry. Two dangers
  104. You don’t point something, because you are worried that author

    will feel horrible about it. You deliver your feedback in unhelpful and blunt way that make an author angry. Two dangers
  105. Improving code maintainability. Understanding and sharing best practices. Knowledge sharing/education.

    Building blameless culture. a Preventing knowledge silos. What code review really can help us with?
  106. Improving code maintainability. Understanding and sharing best practices. Knowledge sharing/education.

    Building blameless culture. a Preventing knowledge silos. Encouraging shared ownership & responsibility. What code review really can help us with?
  107. There is a surprising disparity between our expectations about code

    reviews and what they can actually achieve.