Coding Blocks

Coding Blocks

The world of computer programming is vast in scope. There are literally thousands of topics to cover and no one person could ever reach them all. One of the goals of the Coding Blocks podcast is to introduce a number of these topics to the audience so they can learn during their commute or while cutting the grass. We will cover topics such as best programming practices, design patterns, coding for performance, object oriented coding, database design and implementation, tips, tricks and a whole lot of other things. You'll be exposed to broad areas of information as well as deep dives into the guts of a programming language. While Microsoft.NET is the development platform we're using, most topics discussed are relevant in any number of Object Oriented programming languages. We are all web and database programmers and will be providing useful information on a full spectrum of technologies and are open to any suggestions anyone might have for a topic. So please join us, subscribe, and invite your computer programming friends to come along for the ride.... Show More
May 25, 2020

We learn what to look for in a code review while reviewing Google’s engineering practices documentation as Michael relates patterns to choo-choos, Joe has a “weird voice”, and Allen has a new favorite portion of the show.

Are you reading this via your podcast player? You can find this episode’s full show notes at https://www.codingblocks.net/episode133 where you can also join the conversation.

Sponsors
  • University of California, Irvine Division of Continuing Education – One of the top 50 nationally ranked universities, UCI offers over 80 certificates and specialized programs designed for working professionals. Registration is NOW OPENSign up and reserve your seat today!
  • Datadog.com/codingblocks – Sign up today for a free 14 day trial and get a free Datadog t-shirt after your first dashboard.
  • Survey Says How likely are you to advocate for working from home in the future?

    Take the survey at: https://www.codingblocks.net/episode133.

    News
  • Thank you to everyone that left us a review:
  • iTunes: codewith_himanshu, SpaceDuckets, akirakinski
  • Stitcher: Anonymous (from Croatia), Llanfairpwllgwyngyll (Wikipedia), Murali Suriar
  • Watch Joe solve LeetCode Problems (YouTube)
  • Regarding the OWNERs file … // TODO: Insert Clever Subtitle Here Design
  • This is the MOST IMPORTANT part of the review: the overall design of the changelist (CL).
  • Does the code make sense?
  • Does it belong in the codebase or in a library?
  • Does it meld well with the rest of the system?
  • Is it the right time to add it to the code base?
  • Functionality
  • Does the CL do what it’s supposed to do?
  • Even if it does what it’s supposed to do, is it a good change for the users, both developers and actual end-users?
  • As a reviewer, you should be thinking about all the edge-cases, concurrency issues, and generally just trying to see if any bugs arise just looking at the code.
  • As a reviewer, you can verify the CL if you’d like, or have the developer walk you through the changes (the actual implemented changes rather than just slogging through code).
  • Google specifically calls out parallel programming types of issues that are hard to reason about (even when debugging) especially when it comes to deadlocks and similar types of situations.
  • Complexity
  • This should be checked at every level of the change:
  • Single lines of code,
  • Functions, and
  • Classes
  • Too complex is code that is not easy to understand just looking at the code. Code like this will potentially introduce bugs as developers need to change it in the future.
  • A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates might need to be solved in the future. The future problem should be solved once it arrives and you can see its actual shape and requirements in the physical universe.

    Google’s Engineering Practices documentation Tests
  • Usually tests should be added in the same CL as the change, unless the CL is for an emergency.
  • Emergencies were discussed in episode 132.
  • Make sure the tests are correct and useful.
  • Will the tests fail if the code is broken?
  • Are the assertions simple and useful?
  • Are the tests separated appropriately into different test methods?
  • Naming
  • Were good names chosen?
  • A good name is long enough to be useful and not too long to be hard to read,
  • Comments
  • Were the comments clear and understandable, in English?
  • Were the comments necessary?
  • They should explain WHY code exists and NOT what it’s doing.
  • If the code isn’t clear enough on its own, it should be refactored.
  • Exceptions to the rule can include regular expressions and complex algorithms.
  • Comments are different than documentation of code. Code documentation expresses the purpose, usage and behavior of that code.
  • Style
  • Have a style guide. Google has one for most of the languages they use.
  • Make sure the CL follows the style guide.
  • If something isn’t in the style guide, and as the reviewer you want to comment on the CL to make a point about style, prefix your comment with “Nit”.
  • DO NOT BLOCK PR’s based on personal style preference!
  • Style changes should not be mixed in with “real” changes. Those should be a separate CL.
  • Consistency
  • Google indicates that if existing code conflicts with the style guide, the style guide wins.
  • If the style guide is a recommendation rather than a hard requirement, it’s a judgement call on whether to follow the guide or existing code.
  • If no style guide applies, the CL should remain consistent with existing code.
  • Use TODO statements for cleaning up existing code if outside the scope of the CL.
  • Documentation
  • If the CL changes any significant portion of builds, interactions, tests, etc., then appropriate README’s, reference docs, etc. should be updated.
  • If the CL deprecates portions of the documentation, that should also likely be removed.
  • Every Line
  • Look over every line of non-generated, human written code.
  • You need to at least understand what the code is doing.
  • If you’re having a hard time examining the code in a timely fashion, you may want to ask the developer to walk you through it.
  • If you can’t understand it, it’s very likely future developers won’t either, so getting clarification is good for everyone.
  • If you don’t feel qualified to be the only reviewer, make sure someone else reviews the CL who is qualified, especially when you’re dealing with sensitive subjects such as security, concurrency, accessibility, internationalization, etc.
  • Context
  • Sometimes you need to back up to get a bigger view of what’s changing, rather than just looking at the individual lines that changed.
  • Seeing the whole file versus the few lines that were changed might reveal that 5 lines were added to a 200 line method which likely needs to be revisited.
  • Is the CL improving the health of the system?
  • Is the CL complicating the system?
  • Is the CL making the system more tested or less tested?
  • “Don’t accept CLs that degrade the code health of the system.”
  • Most systems become complex through many small changes.
  • Good Things
  • If you see something good in a CL, let the author know.
  • Many times we focus on mistakes as reviewers, but some positive reinforcement may actually be more valuable.
  • Especially true when mentoring.
  • Resources We Like
  • OWNERS files (chormium.googlesource.com)
  • Modern Code Review: A Case Study at Google (research.google)
  • Google Engineering Practices Documentation (GitHub)
  • What to look for in a code review (GitHub)
  • Comparing Git Workflows (episode 90)
  • Google Style Guides (GitHub)
  • Perl Special Variables Quick Reference (PerlMonks)
  • Email Address Regular Expression That 99.99% Works. (emailregex.com)
  • Tip of the Week
  • List of common misconceptions (Wikipedia)
  • The unofficial extension that integrates Draw.io into VS Code. (marketplace.visualstudio.com)
  • Use Dataproc’s Cluster properties to easily update XML settings. (cloud.google.com)
  • Bonus tip: Include a Dockerfile (or Docker Compose) file with your open source project to help it gain traction.
  • Share
    Mark as Played

    Chat About Coding Blocks

    Popular Podcasts

    13 Days of Halloween
    13 Days of Halloween
    Welcome to Hawthorne Manor. As our newest arrival, the caretaker himself (Keegan-Michael Key) will be providing a daily tour and introduction to another guest. Please remember to wear headphones throughout your stay. And make yourself at home. After all, this is it! A production of iHeartRadio, Blumhouse Television, and Grim & Mild from Aaron Mahnke.
    Halloween in Hell
    Halloween in Hell
    A scripted horror musical podcast, just in time for Halloween. Rock legend Tommy Lee stars as the Devil, featuring original music and performances by Machine Gun Kelly, iann dior, Dana Dentata, phem, and 24kGoldn. On Halloween, Satan lures three performers to his sadistic game show in Hell where they must perform or die. The winner returns to Earth. The losers will play out their showbiz careers in fiery hellfire and damnation.
    Stuff You Should Know
    Stuff You Should Know
    If you've ever wanted to know about champagne, satanism, the Stonewall Uprising, chaos theory, LSD, El Nino, true crime and Rosa Parks then look no further. Josh and Chuck have you covered.
      Music, radio and podcasts, all free. Listen online or download the iHeartRadio App.

      Connect

      © 2020 iHeartMedia, Inc.