Keeping It Clean

Lessons learned from open source on how to polish code.

From DevNexus 2015

Transcript

1. Keeping It Clean Lessons learned from Open Source on how to polish code
2. $ git log -i --grep=polish --pretty=format:"%an" \ | sort | uniq -c | sort -nr 410 Juergen Hoeller 192 Sam Brannen 182 Keith Donald 126 Chris Beams 119 Rossen Stoyanchev 67 Phillip Webb 23 Stephane Nicoll 8 Thomas Risberg 7 Arjen Poutsma 5 Brian Clozel 3 Sebastien Deleuze 3 Andy Clement 2 Scott Andrews 2 Rob Winch 2 Mark Pollack 1 Vasyl Tretiakov 1 Costin Leau 1 Ben Hale 1 Andy Wilkinson
4. Sorry To everyone who’s code I’ve meddled with
5. Why Clean Code?
6. Craftsmanship?
8. Nope!
9. Three kinds of code • Never used • Contains bugs • Needs to evolve
10. reduce cognitive overhead so that code can be understood easily .
11. Overview 1. Simple Stuff 2. Harder Stuff
12. 1. Simple Stuff
13. www.emacswiki.org/emacs/TabsSpacesBoth
15. Git Color [color] ui = auto ~/.gitconfig
16. Spring Boot Conventions • Pretty similar to the “Google Java Style” • Tabs not spaces • No ‘.*’ imports • Always use braces (no single line ifs) • Always use ‘this.’ when referencing private fields • Wrap at 90 chars
17. Wrap at 90 Chars ? ! !
18. Code Cat Videos
19. Constraints are good • It looks good on GitHub • It’s readable on a phone • It works in your terminal • It makes you write better code
20. Automatic Formatting • Spring Framework is manually formatted • Spring Boot is auto-formatted • Both have project specific eclipse configuration
21. Maven <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-eclipse-plugin</artifactId> <configuration> <additionalConfig> <file> <name>.settings/org.eclipse.jdt.ui.prefs</name> <location>${main.basedir}/eclipse/org.eclipse.jdt.ui.prefs</location> </file> <file> <name>.settings/org.eclipse.jdt.core.prefs</name> <location>${main.basedir}/eclipse/org.eclipse.jdt.core.prefs</location> </file> </additionalConfig> </configuration> </plugin> +github.com/philwebb/m2eclipse-maveneclipse
22. Gradle // Include project specific settings task eclipseSettings(type: Copy) { from rootProject.files( "src/eclipse/org.eclipse.jdt.ui.prefs", "src/eclipse/org.eclipse.wst.common.project.facet.core.xml") into project.file('.settings/') outputs.upToDateWhen { false } } tasks["eclipse"].dependsOn(eclipseSettings) github.com/spring-projects/spring-framework/blob/master/gradle/ide.gradle
23. Demo Auto Formatting in Eclipse
24. Ordering • Constants at the top of the file • Inner classes at the end • Private variables in order of priority • Match constructor arguments first • Optional setters later • Read down ordering for method names
25. Example UndertowEmbeddedServletContainer
26. // Comments
28. Don’t trust anyone that tells you not to comment your code !
29. Code Comments /** * This is probably good. */ // This is probably bad!
30. Comments • Document why it exists • Document what it does • Leave breadcrumbs • Don’t document how it does it
31. Example EmbeddedServletContainer
32. 2. Harder Stuff
33. Tests • Tests give you the confidence to change code • Add tests before changing code • Change code independently of tests • Imagine your tests names start ‘should’… • Use given/when/then format • Add the class under test in the doc-comments
34. Tests • Use @RuleS • OutputCapture • TemporaryFolder • ExpectedException • Use assertThat(…) • Use BDD mockito
35. Example TomcatEmbeddedServletContainerFactoryTests
36. Consistency • Even with auto-formatting it can be hard to keep code consistent • Whitespace, Headers, Javadoc, Names • It’s often easier to apply after a change has been made
37. Random File Game $ brew install coreutils $ find . -type f | grep '\.java' | gshuf | head -n1
38. Naming
40. In defense OF ASPFB • Yes yes, hilarious name since 2006, but… • It’s somewhat internal • All implementations of FactoryBean in Spring have a name ending ‘FactoryBean’ • Most Abstract classes that you extend in Spring start with ‘Abstract’ • It’s for creating singleton-scoped proxies
41. We’ve done worse HasThisTypePatternTriedToSneakInSomeGenericOrParameterizedTypePatternMatchingStuffAnywhereVisitor
42. Naming • Use your tests to guide you • Use Javadoc to guide you • Use “dunno” whilst you think of a name • Consider the class name when naming static methods • e.g. Assert.notEquals(…) rather than assertNotEquals(…)
43. Refactoring
44. Extract Method • Use “extract method” to aid readability • Line comments are often a sign • Whitespace is often a sign • Unset variables are often a sign … String foo; if (this.x) { foo = “x”; else { foo = “y”; } …
45. Parameter Objects • Extract method parameters IF • There are too many of them • They are likely to grow
46. Example package org.springframework.context.annotation public interface Condition { boolean matches(BeanDefinitionRegistry registry, ConfigurableListableBeanFactory beanFactory, Environment environment, ResourceLoader resourceLoader, ClassLoader classLoader, AnnotatedTypeMetadata metadata); // vs boolean matches(ConditionContext context, AnnotatedTypeMetadata metadata); }
47. Extract Inner Class • Too much “Extract Method” • Cluster of related methods • The same parameters again and again • Also Consider creating a Package Private class ConfigFileApplicationListener from Spring Boot is a good example
48. Package Tangles • Seriously hinder future refactoring • You won’t find them until it’s too late • Use a tool • Structure 101 • SonarQube
49. The Hidden Debugger Git History
50. Commit Logs • Commit logs are an amazing debugging tool • Eclipse : Team/Show Annotations • git blame • git log -- /some/file.java • Keep Consistency in your logs • Tell a Story
51. Commit Logs • First line Capitalized, short (50 chars or less) • Second line BLANK • Message wrapped at 72 chars • imperative FORM “Fix bug” not “Fixed”/“Fixing” • reference to a issue number tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html chris.beams.io/posts/git-commit
52. Why? • Easy to read in the terminal • Easy to read in the IDE • Looks good on github.com • If you can’t describe a commit in 50 chars, perhaps it should be two commits!
53. commit 12724bf33211d68899d710fda376bd5636731e4c Author: Dave Syer <dsyer@pivotal.io> Date: Thu Feb 12 10:09:19 2015 +0000 Fix ordering of keys in PropertySourcesPropertyValues Since @ConfigurationProperties binding uses a single instance of PropertySourcesPropertyValues per bean, there doesn't seem to be any issue with using a normal LinkedHashMap. Then the order passed in as PropertySources will be preserved. Fixes gh-2487 commit 0ef3de4d82acc32a3f44d872e852d94feb8cd5da Author: Andy Wilkinson <awilkinson@pivotal.io> Date: Wed Feb 11 13:15:52 2015 +0000 Document how to disable auto registration of a Servlet or Filter bean Closes gh-2173 commit b18330972884e8cb6832b0bdbd6c9f545dc1f501 Author: Andy Wilkinson <awilkinson@pivotal.io> Date: Tue Feb 10 20:14:38 2015 +0000 Stop a BeanPostProcessor from preventing config of packages to scan Previously, if a BeanPostProcessor bean was declared in a configuration class that depended on the persistence context, packages to scan would not be configured on the LocalContainerEntityManagerFactoryBean. This was due to the BeanPostProcessor bean triggering the early instantiation of the LCEMFB before EntityScanBeanPostProcessor, the BeanPostProcessor, that applies the @EntityScan configuration, had been given a chance to configure it. This commit updates EntityScanBeanPostProcessor to implement Ordered with an order of zero. This ensures that its ordering is predictable and that it will be driven before any unordered BeanPostProcessor. This means that, unless the user has specifically ordered their BeanPostProcessor to run before EntityScanBeanPostProcessor, LCEMFB’s packages to scan will be configured as expected. Fixes gh-2478 commit 1035e5b02967d68f4ae607e11a7bf7c67e95af72 Author: Stephane Nicoll <snicoll@pivotal.io> Date: Tue Feb 10 16:00:11 2015 +0100 Expose RepositoryRestMvcBootConfiguration If an application defines a custom RepositoryRestMvcConfiguration, all Spring Boot defaults are lots. While this sounds sensible, it can be confusing as Spring Boot exposes properties (`spring.data.rest.*`) that
54. Git Commit Template [core] editor = subl -w [commit] template = ~/.gitmessage.txt ~/.gitconfig #================================================= #======================================================================= Fixes gh- # Use the template above to compose your commit message. Keep the 1st # line summary 50 chars or less and the body a maximum 72 chars per line ~/.gitmessage.txt
55. Git rebase git checkout -b gh-1234 git commit -m 'something' git commit -m 'something else' git commit -m 'unrelated polish' git rebase -i master
56. Final complete Example UndertowEmbeddedServletContainer
57. Undertow Support • Contribution to Boot 1.2 by Ivan Sopov • Initial polish to follow boot conventions 1864d79 • Whitespace, Exceptions, Extract Methods a641f0c • Further commits add features or fix issues github.com/spring-projects/spring-boot/commits/master/spring-boot/src/main/java/org/ springframework/boot/context/embedded/undertow/UndertowEmbeddedServletContainerFactory.java
58. Thanks! Questions? @phillip_webb speakerdeck.com/philwebb github.com/philwebb