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. 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
  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
  3. 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 •
  4. Why code style matters 1. Shared code ownership with consistent

    good coding style improves team productivity, code maintainability, and developer happiness, but … !9 @PeterHilton •
  5. 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 •
  6. 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 }
  7. 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
  8. 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 •
  9. ‘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
  10. 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
  11. 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 •
  12. 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 •
  13. 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
  14. 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
  15. 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 •
  16. 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 •
  17. 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
  18. 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 •
  19. 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
  20. 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
  21. 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
  22. 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.
  23. 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 •
  24. case class Kitten( photo: URL, age: Duration) { def cuteness:

    Int = { (Math.random * 100).intValue } }
  25. 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.
 
 
 
 

  26. 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.
  27. 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.
  28. 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.
  29. if (value != null) { value.validate(); } optionalValue.ifPresent(value -> value.validate());

    assert (value != null) : "value"; var value: String = "… or just use Kotlin ¯\_(ϑ)_/¯"
  30. public class Kitten { public String name; public Date born;

    public String colour; public String photoUrl; public Integer cuteness; }
  31. public class Kitten { public AnimalName name; public BirthDate born;

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

    final Colour colour; final URL photoUrl; final CutenessScore cuteness; }
  33. 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
  34. 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
  35. Other matters 9. Error handling - checked vs unchecked exceptions

    10. Logging 11. Using libraries vs avoiding dependencies !40 @PeterHilton •
  36. 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 •
  37. 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 •
  38. 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 •
  39. How would you establish a team coding style if you

    joined a new team that didn’t have one? !46 @PeterHilton •
  40. If you want to be individual, then pick an interesting

    coding font, and don’t bother the rest of the team !48 @PeterHilton •
  41. 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 •
  42. Maintainable and good code style are hard. Get it right,

    and future- you will thank you. !51 @PeterHilton •
  43. !52 @PeterHilton • ‘The importance of code style is proportional

    to the size of the team and 
 the maturity of the codebase’ - Steven Myers
  44. 1. Automate formatting 2. Choose a paradigm 3. Get better

    at naming 4. Write idiomatic code 5. Team coding style !54 @PeterHilton •