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.

Transcript

1. @PeterHilton http://hilton.org.uk/ Code style matters
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. 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
5. 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 •
6. What matters (and why)
7. David A. Schmidt
8. Nikolay Tarashchenko - Unsplash
9. Robert Bye - Unsplash
10. Why code style matters !9 @PeterHilton •
11. Why code style matters 1. Shared code ownership with consistent good coding style improves team productivity, code maintainability, and developer happiness, but … !9 @PeterHilton •
12. 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 •
13. Why code style matters 1. Maintainability 2. Developer experience 3. Productivity !10 @PeterHilton •
14. Formatting code (getting it out of the way)
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 }
16. 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
17. Austin Nguyen / CC BY-ND
18. 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 •
19. ‘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
20. 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
21. ‘Any color you like’
22. 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 •
23. 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 •
24. Practical matters
 (how to code in style)
25. 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
26. 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
27. 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 •
28. 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 •
29. 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
30. 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 •
31. 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
32. 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
33. 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
34. 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.
35. 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 •
36. case class Kitten( photo: URL, age: Duration) { def cuteness: Int = { (Math.random * 100).intValue } }
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.
 
 
 
 

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.
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.
40. 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.
41. if (value != null) { value.validate(); } optionalValue.ifPresent(value -> value.validate()); assert (value != null) : "value"; var value: String = "… or just use Kotlin ¯\_(ϑ)_/¯"
42. public class Kitten { public String name; public Date born; public String colour; public String photoUrl; public Integer cuteness; }
43. public class Kitten { public AnimalName name; public BirthDate born; public Colour colour; public URL photoUrl; public CutenessScore cuteness; }
44. public class Kitten { AnimalName name; BirthDate born; Colour colour; URL photoUrl; CutenessScore cuteness; }
45. public class Kitten { final AnimalName name; final BirthDate born; final Colour colour; final URL photoUrl; final CutenessScore cuteness; }
46. 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
47. 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
48. Other matters 9. Error handling - checked vs unchecked exceptions 10. Logging 11. Using libraries vs avoiding dependencies !40 @PeterHilton •
49. Team coding style
50. Samuel Zeller - Unsplash
51. 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 •
52. 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 •
53. 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 •
54. How would you establish a team coding style if you joined a new team that didn’t have one? !46 @PeterHilton •
56. If you want to be individual, then pick an interesting coding font, and don’t bother the rest of the team !48 @PeterHilton •
57. 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 •
58. Summary
59. Maintainable and good code style are hard. Get it right, and future- you will thank you. !51 @PeterHilton •
60. !52 @PeterHilton • ‘The importance of code style is proportional to the size of the team and 
 the maturity of the codebase’ - Steven Myers
61. Why code style matters 1. Maintainability 2. Developer experience 3. Productivity !53 @PeterHilton •
62. 1. Automate formatting 2. Choose a paradigm 3. Get better at naming 4. Write idiomatic code 5. Team coding style !54 @PeterHilton •
63. @PeterHilton http://hilton.org.uk/ How to write 
 maintainable code (company training) http://hilton.org.uk/training/