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

Code review

Code review

Code review is a useful and rewarding software development activity that should never be overlooked. There's more than one way to do it: an approach for every kind of project. This presentation compares approaches, and suggests procedures, tools and what to look for in code reviews.

Peter Hilton

November 26, 2023
Tweet

More Decks by Peter Hilton

Other Decks in Technology

Transcript

  1. @PeterHilton
    http://hilton.org.uk/
    Code style matters

    View full-size slide

  2. https://stackoverflow.com/a/3395345 • John Skeet
    String prefix = "";
    for (String serverId : serverIds) {
    sb.append(prefix);
    prefix = ",";
    sb.append(serverId);
    }
    String prefix = "";
    for (String serverId : serverIds) {
    sb.append(prefix);
    prefix = ",";
    sb.append(serverId);
    }
    https://stackoverflow.com/a/3395345 • John Skeet

    View full-size slide

  3. https://stackoverflow.com/a/3395345 • John Skeet
    String prefix = "";
    for (String serverId : serverIds) {
    sb.append(prefix);
    prefix = ",";
    sb.append(serverId);
    }
    String prefix = "";
    for (String serverId : serverIds) {
    sb.append(prefix);
    prefix = ",";
    sb.append(serverId);
    }
    https://stackoverflow.com/a/3395345 • John Skeet

    View full-size slide

  4. Indentation
    Indentation style is almost the only thing that doesn’t
    matter, because it doesn’t change how you read the code.
    As long as you are consistent, indentation style doesn’t
    affect how maintainable the code is
    (but you might need a wider monitor)
    !4
    @PeterHilton •

    View full-size slide

  5. What matters
    (and why)

    View full-size slide

  6. David A. Schmidt

    View full-size slide

  7. Nikolay Tarashchenko - Unsplash

    View full-size slide

  8. Robert Bye - Unsplash

    View full-size slide

  9. Why code style matters
    !9
    @PeterHilton •

    View full-size slide

  10. Why code style matters
    1. Shared code ownership with consistent good coding style
    improves team productivity, code maintainability, and
    developer happiness, but …
    !9
    @PeterHilton •

    View full-size slide

  11. Why code style matters
    1. Shared code ownership with consistent good coding style
    improves team productivity, code maintainability, and
    developer happiness, but …
    2. We want different things, and disagree on even the
    simplest things, such as how to indent code blocks.

    (no, we’re not still discussing tabs vs spaces!)
    !9
    @PeterHilton •

    View full-size slide

  12. Why code style matters
    1. Maintainability
    2. Developer experience
    3. Productivity
    !10
    @PeterHilton •

    View full-size slide

  13. Formatting code
    (getting it out of the way)

    View full-size slide

  14. 7 public
    8 DataPair[] getHotelInformation(String lang, String hotelId, String informationId)
    9 {
    10
    11 String key = "_HOINF_"+lng+"_"+hotelId+"_"+informationId;
    12 DataPair[] tbl = (DataPair[])csh.getObject(key);
    13 if(tbl!=null) return tbl;
    14
    15 Connection cn = null;
    16 OracleCallableStatement cs = null;
    17 try {
    18 String qry = " begin HotelServices.getHotelInfo(?, ?, ?, ?, ?); end; ";
    19 logger . debug("---"+qry+" "+hotelId+" "+informationId);
    20 cn = DriverManager.getConnection("jdbc:weblogic:pool:oraclePool",null);
    21 cs = (OracleCallableStatement)cn.prepareCall(qry);
    22 cs . registerOutParameter(1,java.sql.Types.INTEGER);
    24 cs . setString(3,hotelId);
    27 cs . execute();
    28 int sta = cs.getInt(1);
    29 if(sta!=0) throw new Exception("status not zero sta="+sta);
    30 ResultSet rs = cs.getResultSet(2);
    31 tbl = getDataPairArray(rs);
    32 logger . debug("sta="+sta+" key="+key+" cn="+cn);
    33 csh . put(key,tbl);
    34 }
    35 catch(Exception e)
    36 {
    37 logger . debug("!!! "+e.toString()+" "+key);
    38 }
    39 finally
    40 {
    41 try {
    42 if(cs!=null) cs . close();
    44 }
    45 catch(Exception x)
    46 {
    47 logger . debug("!!! "+x.toString()+" "+key);
    49 }
    50 }
    51 return tbl;
    52 }

    View full-size slide

  15. 7 public
    8 DataPair[] getHotelInformation(String lang, String hotelId, String informationId)
    9 {
    10
    11 String key = "_HOINF_"+lng+"_"+hotelId+"_"+informationId;
    12 DataPair[] tbl = (DataPair[])csh.getObject(key);
    13 if(tbl!=null) return tbl;
    14
    15 Connection cn = null;
    16 OracleCallableStatement cs = null;
    17 try {
    18 String qry = " begin HotelServices.getHotelInfo(?, ?, ?, ?, ?); end; ";
    19 logger . debug("---"+qry+" "+hotelId+" "+informationId);
    20 cn = DriverManager.getConnection("jdbc:weblogic:pool:oraclePool",null);
    21 cs = (OracleCallableStatement)cn.prepareCall(qry);
    22 cs . registerOutParameter(1,java.sql.Types.INTEGER);
    24 cs . setString(3,hotelId);
    27 cs . execute();
    28 int sta = cs.getInt(1);
    29 if(sta!=0) throw new Exception("status not zero sta="+sta);
    30 ResultSet rs = cs.getResultSet(2);
    31 tbl = getDataPairArray(rs);
    32 logger . debug("sta="+sta+" key="+key+" cn="+cn);
    33 csh . put(key,tbl);
    34 }
    35 catch(Exception e)
    36 {
    37 logger . debug("!!! "+e.toString()+" "+key);
    38 }
    39 finally
    40 {
    41 try {
    42 if(cs!=null) cs . close();
    44 }
    45 catch(Exception x)
    46 {
    47 logger . debug("!!! "+x.toString()+" "+key);
    49 }
    50 }
    51 return tbl;
    52 }
    Eastern Polish Christmas tree notation

    View full-size slide

  16. Austin Nguyen / CC BY-ND

    View full-size slide

  17. Formatting code
    Formatting is the first and most basic kind of consistency.
    Making similar things look similar improves readability.
    The compiler doesn’t care about formatting;
    the compiler cares structure - the abstract syntax tree.
    Use formatting to make this code structure more visible
    (in the absence of layout and typography for code).
    !14
    @PeterHilton •

    View full-size slide

  18. ‘Formatting issues are the most contentious but the
    least consequential.
    ‘People can adapt to different formatting styles but it’s
    better if they don’t have to, and less time is devoted
    to the topic if everyone adheres to the same style.
    ‘The problem is how to approach this Utopia without a
    long prescriptive style guide.’
    Effective Go - https://golang.org/doc/effective_go.html#formatting

    View full-size slide

  19. There’s a Java version: ‘Note:
    There is no configurability as to the formatter’s
    algorithm for formatting.
    This is a deliberate design decision to unify our code
    formatting on a single format.’


    https://github.com/google/google-java-format

    View full-size slide

  20. ‘Any color you like’

    View full-size slide

  21. Automate formatting
    Automatic formatting makes diffs cleaner, which saves time.
    The best tools hardly have any options, which saves you
    from wasting time discussing how configure them:
    1. Prettier (JavaScript)
    2. gofmt (Go)
    3. google-java-format (Java)
    4. Black (Python)
    5. scalafmt (Scala) !18
    @PeterHilton •

    View full-size slide

  22. Formatting code
    1. Life’s too short to format code by hand.
    2. Adopt one strict team style.
    3. Only use formatting styles that you can completely
    automate.
    4. Format code automatically, 

    e.g. on save, compile, post-commit.
    5. Try an opinionated formatter (and skip steps 2 and 3)
    !19
    @PeterHilton •

    View full-size slide

  23. Practical matters

    (how to code in style)

    View full-size slide

  24. M A N N I N G
    Peter Hilton
    Erik Bakker
    Francisco Canedo
    FOREWORD BY James Ward
    Covers Play 2
    Play for Scala

    (Manning)

    Peter Hilton

    Erik Bakker

    Francisco Canedo
    http://bit.ly/playscala2p

    View full-size slide

  25. Controller action method
    Now that we have model code that provides data and a template that renders this data
    as HTML, we need to add the code that will coordinate the two. This is the role of a
    controller, and the code looks like listing 2.9.
    package controllers
    import play.api.mvc.{Action, Controller}
    import models.Product
    object Products extends Controller {
    def list = Action { implicit request =>
    val products = Product.findAll
    Ok(views.html.products.list(products))
    }
    }
    Listing 2.9 The products controller—app/controllers/Products.scala
    Controller
    action
    Get a product
    list from model
    Render view
    template

    View full-size slide

  26. More than one kind of idiomatic code style
    There are two kinds of Scala code, corresponding to 

    two kinds of Scala programmers:
    1. those who’d rather be writing Java Kotlin
    2. those who’d rather be writing Haskell
    Scala isn’t the only programming language with this issue
    !23
    @PeterHilton •

    View full-size slide

  27. More than one kind of idiomatic code style
    There are two kinds of Java code, corresponding to 

    two kinds of Java programmers:
    1. those who’d rather be doing object-oriented programming
    2. those who’d rather be writing functional programming
    Java has the same issue
    !24
    @PeterHilton •

    View full-size slide

  28. Naming
    !25
    @PeterHilton •
    1. Start with the language’s idiomatic style.
    2. Consider explicit naming guidelines.
    3. Use code review to apply guidelines.
    4. Accept that you cannot (currently) 

    automate any aspect of naming.
    How to name things: the hardest problem in programming
    https://hilton.org.uk/presentations/naming

    View full-size slide

  29. Peter’s favourite naming guidelines
    1. Only use dictionary words
    2. Use 1-4 words
    3. Be precise and meaningful
    4. Use a larger vocabulary
    5. Use domain terms
    6. Omit type information*
    7. Use collective nouns
    acc → account
    acc → savingsAccount
    data → accountBalance
    receiver → recipient
    user → accountHolder
    dateOpened → opened
    accEntries → statement
    !26
    @PeterHilton •

    View full-size slide

  30. func ReadFull(r Reader, buf []byte) (n int, err error)
    {
    for len(buf) > 0 && err == nil {
    var nr int
    nr, err = r.Read(buf)
    n += nr
    buf = buf[nr:]
    }
    return
    }
    ‘This document gives tips for writing clear, idiomatic Go code.’
    https://golang.org/doc/effective_go.html

    View full-size slide

  31. https://stackoverflow.com/a/3395345 • John Skeet
    String pre = "";
    for (String id : servers) {
    sb.append(pre);
    pre = ",";
    sb.append(id);
    }
    String prefix = "";
    for (String serverId : serverIds) {
    result.append(prefix);
    prefix = ",";
    result.append(serverId);
    }
    https://stackoverflow.com/a/3395345 • John Skeet
    more concise
    more explicit

    View full-size slide

  32. https://stackoverflow.com/a/3395345 • John Skeet
    for (String word : incoming) {
    word = word.toLowerCase();
    if (!"".equals(word.trim())) {
    Integer cnt = wordFrequency.get(word);
    if (cnt == null) {
    wordFrequency.put(word, 1);
    } else {
    int icnt = cnt + 1;
    wordFrequency.put(word, icnt);
    }
    }
    }
    wordFrequency = incoming.stream()
    .map(String::toLowerCase)

    .filter(word -> !word.trim().isEmpty())
    .collect(Collectors.toMap
    (word -> word, word -> 1, (a, b) -> a + b, TreeMap::new));
    https://stackoverflow.com/q/52827369 • azro
    imperative
    functional
    https://stackoverflow.com/a/52827440 • Shubhendu Pramanik

    View full-size slide

  33. Programming paradigm
    Imperative:
    1. procedural
    2. object-oriented
    Declarative:
    3. functional
    4. logic
    5. mathematical
    !30
    @PeterHilton •
    Most modern languages support multiple paradigms.
    This is a mixed blessing, for code style.

    View full-size slide

  34. Paradigm
    Pick one way to use/avoid FP, OOP, async, etc, and stick to it.
    This is easy, provided that you never:
    1. learn anything new about your language
    2. learn anything about functional programming
    3. learn anything new about programming
    4. move to a newer language version
    5. work with other people who have different backgrounds
    !31
    @PeterHilton •

    View full-size slide

  35. case class Kitten(
    photo: URL,
    age: Duration) {
    def cuteness: Int = {
    (Math.random * 100).intValue
    }
    }

    View full-size slide

  36. case class Kitten(
    photo: URL,
    age: Duration) {
    def cuteness: Int = {
    (Math.random * 100).intValue
    }
    }
    // Photo of a kitten to console a trader after a loss, with 

    // cuteness proportional to the magnitude of the loss.





    View full-size slide

  37. case class Kitten(
    photo: URL,
    age: Duration) {
    def cuteness: Int = {
    (Math.random * 100).intValue
    }
    }
    // Photo of a kitten to console a trader after a loss, with 

    // cuteness proportional to the magnitude of the loss.





    // TODO: actually calculate from photo in next iteration.

    View full-size slide

  38. case class Kitten(
    photo: URL,
    age: Duration) {
    def cuteness: Int = {
    (Math.random * 100).intValue
    }
    }
    // Photo of a kitten to console a trader after a loss, with 

    // cuteness proportional to the magnitude of the loss.





    // TODO: actually calculate from photo in next iteration.
    // Note: returns 0-100, 0 for dead kittens.

    View full-size slide

  39. case class Kitten(
    photo: URL,
    age: Duration) {
    def cuteness: Int = {
    (Math.random * 100).intValue
    }
    }
    // Photo of a kitten to console a trader after a loss, with 

    // cuteness proportional to the magnitude of the loss.





    // TODO: actually calculate from photo in next iteration.
    // Note: returns 0-100, 0 for dead kittens.
    // Throws exception for cats too old to be kittens.

    View full-size slide

  40. if (value != null) {
    value.validate();
    }
    optionalValue.ifPresent(value -> value.validate());
    assert (value != null) : "value";
    var value: String = "… or just use Kotlin ¯\_(ϑ)_/¯"

    View full-size slide

  41. public class Kitten {
    public String name;
    public Date born;
    public String colour;
    public String photoUrl;
    public Integer cuteness;
    }

    View full-size slide

  42. public class Kitten {
    public AnimalName name;
    public BirthDate born;
    public Colour colour;
    public URL photoUrl;
    public CutenessScore cuteness;
    }

    View full-size slide

  43. public class Kitten {
    AnimalName name;
    BirthDate born;
    Colour colour;
    URL photoUrl;
    CutenessScore cuteness;
    }

    View full-size slide

  44. public class Kitten {
    final AnimalName name;
    final BirthDate born;
    final Colour colour;
    final URL photoUrl;
    final CutenessScore cuteness;
    }

    View full-size slide

  45. No primitives (coding kata constraint)
    !38
    @PeterHilton •
    1. ‘All primitive values (e.g. booleans, numbers or strings)
    need to be wrapped and must not be visible at object
    boundaries.
    2. ‘Arrays, all kinds of containers like lists or hash-tables and
    even Object (the root class of the language’s class
    hierarchy) are considered primitive as well.’
    http://kata-log.rocks/no-primitives
    https://github.com/codurance/task-list

    View full-size slide

  46. What matters in code style
    !39
    @PeterHilton •
    1. Code formatting
    2. Idiomatic code - more accurate, expressive and inclusive
    3. Identifier naming
    4. Length - concise vs explicit
    5. Paradigm - procedural vs object-oriented vs functional
    6. Comments - none, classes, methods, properties?
    7. Using and handling null (or not)
    8. Data types - primitive vs domain types, value types

    View full-size slide

  47. Other matters
    9. Error handling - checked vs unchecked exceptions
    10. Logging
    11. Using libraries vs avoiding dependencies
    !40
    @PeterHilton •

    View full-size slide

  48. Team coding style

    View full-size slide

  49. Samuel Zeller - Unsplash

    View full-size slide

  50. Which version of C++ do you use?
    1998 ISO/IEC 14882:1998[20] C++98
    2003 ISO/IEC 14882:2003[21] C++03
    2011 ISO/IEC 14882:2011[22] C++11, C++0x
    2014 ISO/IEC 14882:2014[23] C++14, C++1y
    2017 ISO/IEC 14882:2017[8] C++17, C++1z
    !43
    @PeterHilton •

    View full-size slide

  51. Which version of SQL do you use?
    1986 SQL-86
    1989 SQL-89
    1992 SQL-92
    1999 SQL:1999
    2003 SQL:2003
    2006 SQL:2006
    2008 SQL:2008
    2011 SQL:2011
    2016 SQL:2016
    !44
    @PeterHilton •

    View full-size slide

  52. Which version of JavaScript do you use?

    (i.e. ECMA-262 a.k.a. ECMAScript)
    1997 v1
    1998 v2
    1999 v3
    v4*
    2009 v5
    * abandoned
    2011 v5.1
    2015 v6 (ES2015)
    2016 v7 (ES2016)
    2017 v8 (ES2017)
    2018 v9 (ES2018)
    !45
    @PeterHilton •

    View full-size slide

  53. How would you establish
    a team coding style if
    you joined a new team
    that didn’t have one?
    !46
    @PeterHilton •

    View full-size slide

  54. If you want to be
    individual, then pick an
    interesting coding font,
    and don’t bother the rest
    of the team
    !48
    @PeterHilton •

    View full-size slide

  55. Implement a team coding style
    1. Resolve all of the earlier practical matters within the team

    (time-box discussion, then the tech/team lead decides)
    2. Automate as much as possible, and document the rest
    3. Strictly enforce team style in code reviews
    4. Refactor when you change the team style 

    (and actually finish the refactoring!)
    5. Revisit the team style each time the team changes
    6. Learn about new ideas and experiment !49
    @PeterHilton •

    View full-size slide

  56. Maintainable and good
    code style are hard.
    Get it right, and future-
    you will thank you.
    !51
    @PeterHilton •

    View full-size slide

  57. !52
    @PeterHilton •
    ‘The importance of code
    style is proportional to
    the size of the team and 

    the maturity of the
    codebase’ - Steven Myers

    View full-size slide

  58. Why code style matters
    1. Maintainability
    2. Developer experience
    3. Productivity
    !53
    @PeterHilton •

    View full-size slide

  59. 1. Automate formatting
    2. Choose a paradigm
    3. Get better at naming
    4. Write idiomatic code
    5. Team coding style
    !54
    @PeterHilton •

    View full-size slide

  60. @PeterHilton
    http://hilton.org.uk/
    How to write 

    maintainable code
    (company training)
    http://hilton.org.uk/training/

    View full-size slide