This document represents a series of guidelines for writing code that will swiftly pass code review with my team. It doesn't attempt to be fair. It doesn't attempt to listen to your opinion. It does present a series of guidelines. You may chose to ignore those guidelines (after all, if they had to be obeyed, we'd have called them "laws") We may choose to point back to these when reviewing your code. We'll attempt to avoid being (passive) aggressive when we do so.
Hugs and cuddles.
Test first
We're working on a framework for writing automated tests. It's probably a good idea for us to lead by example and write some tests. It hasn't been proven — but it's a scientific fact — that writing tests after the fact is as boring as boring can be. So we write the tests first. This has the added advantage that we can think about the kind of functionality we want independent of how it's implemented.
KISS, YAGNI
Or "Keep It Stupidly Simple, You Ain't Gonna Need It". When writing new code, just write the code you need. Don't attempt to write some über-generic, ultra-lightweight, whizz-bang sub-framework for handling every conceivable edge case when you only need to do "this thing" once. It's another Scientific Fact that engineers are bloody awful at spotting patterns before the patterns emerge. Wait until the third or even fourth time you repeat something before attempting to extract a common API.
A good book for this? "Refactoring to Patterns".
Prefer composition over inheritance
Let's admit this up front: Java's a deeply flawed language. If I had to write this crap in vi or emacs, I'd be sitting in a corner, rocking backwards and forwards, weeping gently and pulling at my hair. Fortunately, we have IDEs, so It's All Okay. One area where Java is flawed is that it's really easy to inherit from a base class, and really clumsy to do proper composition. Nevertheless, as a rule of thumb: inheritance of interfaces is cool, subclassing a concrete base class is not.
Now, there will be times where it'd be Really Useful to be able to share some common functionality between things that appear to be related. For some reason, this always seems to happen with base test classes. It's a weird fetish, and it's one we should be disabused of. There are better ways to handle this, possibly through JUnit's Rules, or by extracting the common functionality into its own class and just newing it up on demand.
Point is: if you're using inheritance to share some common functionality between otherwise unrelated classes (and "it's a test" doesn't make them related) you're not doing inheritance right. You are, in fact, Doing It Wrong.
Role-based interfaces #ftw
Role-based interfaces: we like them, we use them, and we encourage other people to use them. Yes, this will mean that your classes might implement a lot of interfaces. That means that they collaborate with a lot of other classes. That tells you something. It probably tells you your class has too many roles or responsibilities.
Remember, kids, inside every fat class are at least two classes waiting to climb out.
Expose Collaborators
We take the use of Dependency Injection as an article of faith. This does not mean that we whole-heartedly embrace the need for a "DI Container", but it does mean that we're huge fans of exposing the collaborators of a class via its constructor (because "constructor-based DI")
Having said that, "new" is not a dirty word. It's totally pukka to use it, particularly for those handy utility classes we mentioned above. Having said that….
Utility Classes: Just Say No
A utility class represents a severe failure of imagination. They tend to become dumping grounds for only loosely related methods, which are typically static. If you have one of these, a fun exercise is to decompose it into what I like to refer to as "Objects" and use those instead. If you're having trouble with the traditional "noun-based" approach to identifying classes, try a bit of London School TDD and see what shakes loose.
Singletons? Static Methods? Also No
Singletons (in the traditional "implemented as a static field in a class" sense, not in the "ideally we'd only have one of these" sense) destroy our ability to have fun and write tests that can run in parallel, slashing our potential productivity. Also, it leads people to start using the Service Locator pattern instead of Dependency Injection, and we take DI as an article of faith (see above), mainly because it facilitates TDD by making collaborators clear, like we (also) said above.
Use strong types where possible
We're using a high ceremony language. Might as well embrace that properly. We dislike Stringly-typed code, and we like Tiny Types. Why do we like them? Because they allow our code to express intent as clearly as possible, and we can do things like "hang behaviour" off them as the need arises.
BTW, this means that we really should never be returning "WebElement" from a Page Object. Return a class that models the thing the user would expect to be returned, even if that leads to a class with nothing but a constructor.
Use the most abstract type that conveys intent for variables and fields
The most abstract type explains to the reader what you do. The concrete type is how you're going to do it. You should be able to change your mind about "how" without needing to change the "what". The most abstract type that conveys intent for a variable ("Map" instead of "HashMap", "WebDriver" instead of "DroidDriver")
I've been asked whether a method should return an ImmutableSet or just a Set when it returns a set of things that both sorted and immutable. Redundant as this question may seem, I'll still have a bash at answering it. Return the ImmutableSet, as that conveys the intent of the return value. If the caller doesn't care about the mutability of the set, they can assign it to a Set. Everyone's happy.
Use the right naming convention
The naming convention in our codebase is a hangover from the Android coding style, which was created by people who wrote C++ for a living (which also explains why there are so many static methods in the framework too) We don't write C++ for a living, and using a foreign language's coding conventions in Java code makes you look like a clown and an arsehole.
However, it appears we're perfectly happy to present ourselves as people who find it hard to get dressed in the morning without hurting ourselves. Consequently, when writing code that's just for us, use the coding standard of the rest of the codebase and choke back the waves of nausea. If you're writing code that we'll put in front of a public who believe we're competent engineers (that is, OSS) use the Google coding standard (effectively Oracle's, but with a two space indent)
Use the Ubiquitous Language, Luke
If everyone calls it a "Self Aggrandizsing Wattle", don't name the class "AndroidPoweredMultiflexViewPort". We want people to find the classes we write, and we want them to understand how they relate to other classes in the system. Using the names that people call things as class names is Totally Cool.
Also, if you've not done so, go and grab a copy of Domain Driven Design and attempt to wade through as much of it as you can bear. Then skip to the end and read the bit about Anti-Corruption Layers. That bit's good.
Naming Things
Design Patterns are a means of communication, not blueprints. Similarly, the thing that makes classes interesting isn't what pattern they happen to implement, it's the role that they play in our system. Leave the pattern name off the class name, ok? The exception to this is the "Builder" pattern. Everyone expects the "Builder of Thing" to be called "ThingBuilder". We might as well go with the flow on this one and buck our own contrarian ways.
Similarly, every concrete class is an implementation of something, so using the postfix "Impl" (presumably if you're too lazy to name something properly, you're too lazy to type "Implementation") as a class name is a Dumb Thing To Do. Name the class for the particular thing that makes it interesting within the system, or prefix the name with "Default" if there is genuinely nothing interesting about it. Try and avoid naming the class around some obscure implementation detail that no-one using the class cares about.
BTW, it's acceptable to append the name of the interface being implemented to the class name, but it's better to try and name the class for the role it plays in the system.
Keep It SOLID
- Single Responsibility Principle
- Open/Closed Principle
- Liskov Substitution Principle
- Interface Segregation Principle
- Dependency Inversion Principle
Presumably you're a Software Engineer. If any of those are hard to understand, then I'd suggest using your favourite search engine to read up on them.
Document in Proper English That Which Needs Documenting
It's a safe assumption that anyone actually reading your code is a professional software developer. Telling them stuff that they can see just by reading your code in javadocs and comments is Not Helping, so don't do it. Use comments to explain the reasoning behind particular design decisions, or to alert people to odd corner cases that might actually need explanation.
Consider replacing one line comments of a block of code with a sensibly named method containing that block of code. After all, when it comes to maintaining this shit, only the most dedicated of developers will actually update the code and the docs.
It's cool to use correct grammar and punctuation. Please do so, and try and end sentences with a period cretaceous
