diff --git a/src/java/com/twitter/common/styleguide.md b/src/java/com/twitter/common/styleguide.md index 0e604d9ab..3a2e5960c 100644 --- a/src/java/com/twitter/common/styleguide.md +++ b/src/java/com/twitter/common/styleguide.md @@ -42,119 +42,124 @@ advantage to effectively tell the story to those reading the code. We use the "one true brace style" ([1TBS](http://en.wikipedia.org/wiki/Indent_style#Variant:_1TBS)). Indent size is 2 columns. - :::java - // Like this. - if (x < 0) { - negative(x); - } else { - nonnegative(x); - } - - // Not like this. - if (x < 0) - negative(x); - - // Also not like this. - if (x < 0) negative(x); +```java +// Like this. +if (x < 0) { + negative(x); +} else { + nonnegative(x); +} + +// Not like this. +if (x < 0) + negative(x); + +// Also not like this. +if (x < 0) negative(x); +``` Continuation indent is 4 columns. Nested continuations may add 4 columns or 2 at each level. - :::java - // Bad. - // - Line breaks are arbitrary. - // - Scanning the code makes it difficult to piece the message together. - throw new IllegalStateException("Failed to process request" + request.getId() - + " for user " + user.getId() + " query: '" + query.getText() - + "'"); - - // Good. - // - Each component of the message is separate and self-contained. - // - Adding or removing a component of the message requires minimal reformatting. - throw new IllegalStateException("Failed to process" - + " request " + request.getId() - + " for user " + user.getId() - + " query: '" + query.getText() + "'"); +```java +// Bad. +// - Line breaks are arbitrary. +// - Scanning the code makes it difficult to piece the message together. +throw new IllegalStateException("Failed to process request" + request.getId() + + " for user " + user.getId() + " query: '" + query.getText() + + "'"); + +// Good. +// - Each component of the message is separate and self-contained. +// - Adding or removing a component of the message requires minimal reformatting. +throw new IllegalStateException("Failed to process" + + " request " + request.getId() + + " for user " + user.getId() + + " query: '" + query.getText() + "'"); +``` Don't break up a statement unnecessarily. - :::java - // Bad. - final String value = - otherValue; +```java +// Bad. +final String value = + otherValue; - // Good. - final String value = otherValue; +// Good. +final String value = otherValue; +``` Method declaration continuations. - :::java - // Sub-optimal since line breaks are arbitrary and only filling lines. - String downloadAnInternet(Internet internet, Tubes tubes, - Blogosphere blogs, Amount bandwidth) { - tubes.download(internet); - ... - } - - // Acceptable. - String downloadAnInternet(Internet internet, Tubes tubes, Blogosphere blogs, - Amount bandwidth) { - tubes.download(internet); - ... - } - - // Nicer, as the extra newline gives visual separation to the method body. - String downloadAnInternet(Internet internet, Tubes tubes, Blogosphere blogs, - Amount bandwidth) { - - tubes.download(internet); - ... - } - - // Also acceptable, but may be awkward depending on the column depth of the opening parenthesis. - public String downloadAnInternet(Internet internet, - Tubes tubes, - Blogosphere blogs, - Amount bandwidth) { - tubes.download(internet); - ... - } - - // Preferred for easy scanning and extra column space. - public String downloadAnInternet( - Internet internet, - Tubes tubes, - Blogosphere blogs, - Amount bandwidth) { - - tubes.download(internet); - ... - } +```java +// Sub-optimal since line breaks are arbitrary and only filling lines. +String downloadAnInternet(Internet internet, Tubes tubes, + Blogosphere blogs, Amount bandwidth) { + tubes.download(internet); + ... +} + +// Acceptable. +String downloadAnInternet(Internet internet, Tubes tubes, Blogosphere blogs, + Amount bandwidth) { + tubes.download(internet); + ... +} + +// Nicer, as the extra newline gives visual separation to the method body. +String downloadAnInternet(Internet internet, Tubes tubes, Blogosphere blogs, + Amount bandwidth) { + + tubes.download(internet); + ... +} + +// Also acceptable, but may be awkward depending on the column depth of the opening parenthesis. +public String downloadAnInternet(Internet internet, + Tubes tubes, + Blogosphere blogs, + Amount bandwidth) { + tubes.download(internet); + ... +} + +// Preferred for easy scanning and extra column space. +public String downloadAnInternet( + Internet internet, + Tubes tubes, + Blogosphere blogs, + Amount bandwidth) { + + tubes.download(internet); + ... +} +``` ##### Chained method calls - :::java - // Bad. - // - Line breaks are based on line length, not logic. - Iterable modules = ImmutableList.builder().add(new LifecycleModule()) - .add(new AppLauncherModule()).addAll(application.getModules()).build(); - - // Better. - // - Calls are logically separated. - // - However, the trailing period logically splits a statement across two lines. - Iterable modules = ImmutableList.builder(). - add(new LifecycleModule()). - add(new AppLauncherModule()). - addAll(application.getModules()). - build(); - - // Good. - // - Method calls are isolated to a line. - // - The proper location for a new method call is unambiguous. - Iterable modules = ImmutableList.builder() - .add(new LifecycleModule()) - .add(new AppLauncherModule()) - .addAll(application.getModules()) - .build(); +```java +// Bad. +// - Line breaks are based on line length, not logic. +Iterable modules = ImmutableList.builder().add(new LifecycleModule()) + .add(new AppLauncherModule()).addAll(application.getModules()).build(); + +// Better. +// - Calls are logically separated. +// - However, the trailing period logically splits a statement across two lines. +Iterable modules = ImmutableList.builder(). + add(new LifecycleModule()). + add(new AppLauncherModule()). + addAll(application.getModules()). + build(); + +// Good. +// - Method calls are isolated to a line. +// - The proper location for a new method call is unambiguous. +Iterable modules = ImmutableList.builder() + .add(new LifecycleModule()) + .add(new AppLauncherModule()) + .addAll(application.getModules()) + .build(); +``` #### No tabs An oldie, but goodie. We've found tab characters to cause more harm than good. @@ -180,51 +185,54 @@ ordering (sections [8.3.1](http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.3.1) and [8.4.3](http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.4.3)). - :::java - // Bad. - final volatile private String value; +```java +// Bad. +final volatile private String value; - // Good. - private final volatile String value; +// Good. +private final volatile String value; +``` ### Variable naming #### Extremely short variable names should be reserved for instances like loop indices. - :::java - // Bad. - // - Field names give little insight into what fields are used for. - class User { - private final int a; - private final String m; +```java +// Bad. +// - Field names give little insight into what fields are used for. +class User { + private final int a; + private final String m; - ... - } + ... +} - // Good. - class User { - private final int ageInYears; - private final String maidenName; +// Good. +class User { + private final int ageInYears; + private final String maidenName; - ... - } + ... +} +``` #### Include units in variable names - :::java - // Bad. - long pollInterval; - int fileSize; +```java +// Bad. +long pollInterval; +int fileSize; - // Good. - long pollIntervalMs; - int fileSizeGb. +// Good. +long pollIntervalMs; +int fileSizeGb. - // Better. - // - Unit is built in to the type. - // - The field is easily adaptable between units, readability is high. - Amount pollInterval; - Amount fileSize; +// Better. +// - Unit is built in to the type. +// - The field is easily adaptable between units, readability is high. +Amount pollInterval; +Amount fileSize; +``` #### Don't embed metadata in variable names A variable name should describe the variable's purpose. Adding extra information like scope and @@ -232,54 +240,59 @@ type is generally a sign of a bad variable name. Avoid embedding the field type in the field name. - :::java - // Bad. - Map idToUserMap; - String valueString; +```java +// Bad. +Map idToUserMap; +String valueString; - // Good. - Map usersById; - String value; +// Good. +Map usersById; +String value; +``` Also avoid embedding scope information in a variable. Hierarchy-based naming suggests that a class is too complex and should be broken apart. - :::java - // Bad. - String _value; - String mValue; +```java +// Bad. +String _value; +String mValue; - // Good. - String value; +// Good. +String value; +``` ### Space pad operators and equals. - :::java - // Bad. - // - This offers poor visual separation of operations. - int foo=a+b+1; +```java +// Bad. +// - This offers poor visual separation of operations. +int foo=a+b+1; - // Good. - int foo = a + b + 1; +// Good. +int foo = a + b + 1; +``` ### Be explicit about operator precedence Don't make your reader open the [spec](http://docs.oracle.com/javase/tutorial/java/nutsandbolts/operators.html) to confirm, if you expect a specific operation ordering, make it obvious with parenthesis. - :::java - // Bad. - return a << 8 * n + 1 | 0xFF; +```java +// Bad. +return a << 8 * n + 1 | 0xFF; - // Good. - return (a << (8 * n) + 1) | 0xFF; +// Good. +return (a << (8 * n) + 1) | 0xFF; +``` It's even good to be *really* obvious. - :::java - if ((values != null) && (10 > values.size())) { - ... - } +```java +if ((values != null) && (10 > values.size())) { + ... +} +``` ### Documentation @@ -290,22 +303,23 @@ the more documentation is needed. Your elementary school teacher was right - you should never start a statement this way. Likewise, you shouldn't write documentation this way. - :::java - // Bad. - /** - * This is a class that implements a cache. It does caching for you. - */ - class Cache { - ... - } - - // Good. - /** - * A volatile storage for objects based on a key, which may be invalidated and discarded. - */ - class Cache { - ... - } +```java +// Bad. +/** + * This is a class that implements a cache. It does caching for you. + */ +class Cache { + ... +} + +// Good. +/** + * A volatile storage for objects based on a key, which may be invalidated and discarded. + */ +class Cache { + ... +} +``` #### Documenting a class Documentation for a class may range from a single sentence @@ -314,120 +328,124 @@ blanks in the API, and make it easier to quickly and *correctly* use your API. A thorough class doc usually has a one sentence summary and, if necessary, a more detailed explanation. - :::java - /** - * An RPC equivalent of a unix pipe tee. Any RPC sent to the tee input is guaranteed to have - * been sent to both tee outputs before the call returns. - * - * @param The type of the tee'd service. - */ - public class RpcTee { - ... - } +```java +/** + * An RPC equivalent of a unix pipe tee. Any RPC sent to the tee input is guaranteed to have + * been sent to both tee outputs before the call returns. + * + * @param The type of the tee'd service. + */ +public class RpcTee { + ... +} +``` #### Documenting a method A method doc should tell what the method *does*. Depending on the argument types, it may also be important to document input format. - :::java - // Bad. - // - The doc tells nothing that the method declaration didn't. - // - This is the 'filler doc'. It would pass style checks, but doesn't help anybody. - /** - * Splits a string. - * - * @param s A string. - * @return A list of strings. - */ - List split(String s); - - // Better. - // - We know what the method splits on. - // - Still some undefined behavior. - /** - * Splits a string on whitespace. - * - * @param s The string to split. An {@code null} string is treated as an empty string. - * @return A list of the whitespace-delimited parts of the input. - */ - List split(String s); - - // Great. - // - Covers yet another edge case. - /** - * Splits a string on whitespace. Repeated whitespace characters are collapsed. - * - * @param s The string to split. An {@code null} string is treated as an empty string. - * @return A list of the whitespace-delimited parts of the input. - */ - List split(String s); +```java +// Bad. +// - The doc tells nothing that the method declaration didn't. +// - This is the 'filler doc'. It would pass style checks, but doesn't help anybody. +/** + * Splits a string. + * + * @param s A string. + * @return A list of strings. + */ +List split(String s); + +// Better. +// - We know what the method splits on. +// - Still some undefined behavior. +/** + * Splits a string on whitespace. + * + * @param s The string to split. An {@code null} string is treated as an empty string. + * @return A list of the whitespace-delimited parts of the input. + */ +List split(String s); + +// Great. +// - Covers yet another edge case. +/** + * Splits a string on whitespace. Repeated whitespace characters are collapsed. + * + * @param s The string to split. An {@code null} string is treated as an empty string. + * @return A list of the whitespace-delimited parts of the input. + */ +List split(String s); +``` #### Be professional We've all encountered frustration when dealing with other libraries, but ranting about it doesn't do you any favors. Suppress the expletives and get to the point. - :::java - // Bad. - // I hate xml/soap so much, why can't it do this for me!? - try { - userId = Integer.parseInt(xml.getField("id")); - } catch (NumberFormatException e) { - ... - } - - // Good. - // TODO(Jim): Tuck field validation away in a library. - try { - userId = Integer.parseInt(xml.getField("id")); - } catch (NumberFormatException e) { - ... - } +```java +// Bad. +// I hate xml/soap so much, why can't it do this for me!? +try { + userId = Integer.parseInt(xml.getField("id")); +} catch (NumberFormatException e) { + ... +} + +// Good. +// TODO(Jim): Tuck field validation away in a library. +try { + userId = Integer.parseInt(xml.getField("id")); +} catch (NumberFormatException e) { + ... +} +``` #### Don't document overriding methods (usually) - :::java - interface Database { - /** - * Gets the installed version of the database. - * - * @return The database version identifier. - */ - String getVersion(); - } - - // Bad. - // - Overriding method doc doesn't add anything. - class PostgresDatabase implements Database { - /** - * Gets the installed version of the database. - * - * @return The database version identifier. - */ - @Override - public String getVersion() { - ... - } - } - - // Good. - class PostgresDatabase implements Database { - @Override - public int getVersion(); - } - - // Great. - // - The doc explains how it differs from or adds to the interface doc. - class TwitterDatabase implements Database { - /** - * Semantic version number. - * - * @return The database version in semver format. - */ - @Override - public String getVersion() { - ... - } - } +```java +interface Database { + /** + * Gets the installed version of the database. + * + * @return The database version identifier. + */ + String getVersion(); +} + +// Bad. +// - Overriding method doc doesn't add anything. +class PostgresDatabase implements Database { + /** + * Gets the installed version of the database. + * + * @return The database version identifier. + */ + @Override + public String getVersion() { + ... + } +} + +// Good. +class PostgresDatabase implements Database { + @Override + public int getVersion(); +} + +// Great. +// - The doc explains how it differs from or adds to the interface doc. +class TwitterDatabase implements Database { + /** + * Semantic version number. + * + * @return The database version in semver format. + */ + @Override + public String getVersion() { + ... + } +} +``` #### Use javadoc features @@ -442,44 +460,46 @@ history and `OWNERS` files to determine ownership of a body of code. Imports are grouped by top-level package, with blank lines separating groups. Static imports are grouped in the same way, in a section below traditional imports. - :::java - import java.* - import javax.* +```java +import java.* +import javax.* - import scala.* +import scala.* - import com.* +import com.* - import net.* +import net.* - import org.* +import org.* - import com.twitter.* +import com.twitter.* - import static * +import static * +``` #### No wildcard imports Wildcard imports make the source of an imported class less clear. They also tend to hide a high class [fan-out](http://en.wikipedia.org/wiki/Coupling_(computer_programming)#Module_coupling).
*See also [texas imports](#stay-out-of-texas)* - :::java - // Bad. - // - Where did Foo come from? - import com.twitter.baz.foo.*; - import com.twitter.*; +```java +// Bad. +// - Where did Foo come from? +import com.twitter.baz.foo.*; +import com.twitter.*; - interface Bar extends Foo { - ... - } +interface Bar extends Foo { + ... +} - // Good. - import com.twitter.baz.foo.BazFoo; - import com.twitter.Foo; +// Good. +import com.twitter.baz.foo.BazFoo; +import com.twitter.Foo; - interface Bar extends Foo { - ... - } +interface Bar extends Foo { + ... +} +``` ### Use annotations wisely @@ -489,19 +509,20 @@ be explicit about it by marking [@Nullable](http://code.google.com/p/jsr-305/source/browse/trunk/ri/src/main/java/javax/annotation/Nullable.java?r=24). This is advisable even for fields/methods with private visibility. - :::java - class Database { - @Nullable private Connection connection; +```java +class Database { + @Nullable private Connection connection; - @Nullable - Connection getConnection() { - return connection; - } + @Nullable + Connection getConnection() { + return connection; + } - void setConnection(@Nullable Connection connection) { - this.connection = connection; - } - } + void setConnection(@Nullable Connection connection) { + this.connection = connection; + } +} +``` #### @VisibleForTesting Sometimes it makes sense to hide members and functions in general, but they may still be required @@ -511,41 +532,42 @@ to indicate the purpose for visibility. Constants are a great example of things that are frequently exposed in this way. - :::java - // Bad. - // - Any adjustments to field names need to be duplicated in the test. - class ConfigReader { - private static final String USER_FIELD = "user"; - - Config parseConfig(String configData) { - ... - } - } - public class ConfigReaderTest { - @Test - public void testParseConfig() { - ... - assertEquals(expectedConfig, reader.parseConfig("{user: bob}")); - } - } - - // Good. - // - The test borrows directly from the same constant. - class ConfigReader { - @VisibleForTesting static final String USER_FIELD = "user"; - - Config parseConfig(String configData) { - ... - } - } - public class ConfigReaderTest { - @Test - public void testParseConfig() { - ... - assertEquals(expectedConfig, - reader.parseConfig(String.format("{%s: bob}", ConfigReader.USER_FIELD))); - } - } +```java +// Bad. +// - Any adjustments to field names need to be duplicated in the test. +class ConfigReader { + private static final String USER_FIELD = "user"; + + Config parseConfig(String configData) { + ... + } +} +public class ConfigReaderTest { + @Test + public void testParseConfig() { + ... + assertEquals(expectedConfig, reader.parseConfig("{user: bob}")); + } +} + +// Good. +// - The test borrows directly from the same constant. +class ConfigReader { + @VisibleForTesting static final String USER_FIELD = "user"; + + Config parseConfig(String configData) { + ... + } +} +public class ConfigReaderTest { + @Test + public void testParseConfig() { + ... + assertEquals(expectedConfig, + reader.parseConfig(String.format("{%s: bob}", ConfigReader.USER_FIELD))); + } +} +``` ### Use interfaces Interfaces decouple functionality from implementation, allowing you to use multiple implementations @@ -556,40 +578,42 @@ implementations package private. Many small interfaces can seem heavyweight, since you end up with a large number of source files. Consider the pattern below as an alternative. - :::java - interface FileFetcher { - File getFile(String name); +```java +interface FileFetcher { + File getFile(String name); - // All the benefits of an interface, with little source management overhead. - // This is particularly useful when you only expect one implementation of an interface. - static class HdfsFileFetcher implements FileFetcher { - @Override File getFile(String name) { - ... - } - } - } + // All the benefits of an interface, with little source management overhead. + // This is particularly useful when you only expect one implementation of an interface. + static class HdfsFileFetcher implements FileFetcher { + @Override File getFile(String name) { + ... + } + } +} +``` #### Leverage or extend existing interfaces Sometimes an existing interface allows your class to easily 'plug in' to other related classes. This leads to highly [cohesive](http://en.wikipedia.org/wiki/Cohesion_(computer_science)) code. - :::java - // An unfortunate lack of consideration. Anyone who wants to interact with Blobs will need to - // write specific glue code. - class Blobs { - byte[] nextBlob() { - ... - } - } - - // Much better. Now the caller can easily adapt this to standard collections, or do more - // complex things like filtering. - class Blobs implements Iterable { - @Override - Iterator iterator() { - ... - } - } +```java +// An unfortunate lack of consideration. Anyone who wants to interact with Blobs will need to +// write specific glue code. +class Blobs { + byte[] nextBlob() { + ... + } +} + +// Much better. Now the caller can easily adapt this to standard collections, or do more +// complex things like filtering. +class Blobs implements Iterable { + @Override + Iterator iterator() { + ... + } +} +``` Warning - don't bend the definition of an existing interface to make this work. If the interface doesn't conceptually apply cleanly, it's best to avoid this. @@ -604,88 +628,90 @@ for real-world behavior. For example, rather than fetching a row from a real da a test row that you want to return. This is most commonly performed with a fake object or a mock object. While the difference sounds subtle, mocks have major benefits over fakes. - :::java - class RpcClient { - RpcClient(HttpTransport transport) { - ... - } - } - - // Bad. - // - Our test has little control over method call order and frequency. - // - We need to be careful that changes to HttpTransport don't disable FakeHttpTransport. - // - May require a significant amount of code. - class FakeHttpTransport extends HttpTransport { - @Override - void writeBytes(byte[] bytes) { - ... - } - - @Override - byte[] readBytes() { - ... - } - } - - public class RpcClientTest { - private RpcClient client; - private FakeHttpTransport transport; - - @Before - public void setUp() { - transport = new FakeHttpTransport(); - client = new RpcClient(transport); - } - - ... - } - - interface Transport { - void writeBytes(byte[] bytes); - byte[] readBytes(); - } - - class RpcClient { - RpcClient(Transport transport) { - ... - } - } - - // Good. - // - We can mock the interface and have very fine control over how it is expected to be used. - public class RpcClientTest { - private RpcClient client; - private Transport transport; - - @Before - public void setUp() { - transport = EasyMock.createMock(Transport.class); - client = new RpcClient(transport); - } - - ... - } +```java +class RpcClient { + RpcClient(HttpTransport transport) { + ... + } +} + +// Bad. +// - Our test has little control over method call order and frequency. +// - We need to be careful that changes to HttpTransport don't disable FakeHttpTransport. +// - May require a significant amount of code. +class FakeHttpTransport extends HttpTransport { + @Override + void writeBytes(byte[] bytes) { + ... + } + + @Override + byte[] readBytes() { + ... + } +} + +public class RpcClientTest { + private RpcClient client; + private FakeHttpTransport transport; + + @Before + public void setUp() { + transport = new FakeHttpTransport(); + client = new RpcClient(transport); + } + + ... +} + +interface Transport { + void writeBytes(byte[] bytes); + byte[] readBytes(); +} + +class RpcClient { + RpcClient(Transport transport) { + ... + } +} + +// Good. +// - We can mock the interface and have very fine control over how it is expected to be used. +public class RpcClientTest { + private RpcClient client; + private Transport transport; + + @Before + public void setUp() { + transport = EasyMock.createMock(Transport.class); + client = new RpcClient(transport); + } + + ... +} +``` ### Let your callers construct support objects - :::java - // Bad. - // - A unit test needs to manage a temporary file on disk to test this class. - class ConfigReader { - private final InputStream configStream; - ConfigReader(String fileName) throws IOException { - this.configStream = new FileInputStream(fileName); - } - } - - // Good. - // - Testing this class is as easy as using ByteArrayInputStream with a String. - class ConfigReader { - private final InputStream configStream; - ConfigReader(InputStream configStream){ - this.configStream = checkNotNull(configStream); - } - } +```java +// Bad. +// - A unit test needs to manage a temporary file on disk to test this class. +class ConfigReader { + private final InputStream configStream; + ConfigReader(String fileName) throws IOException { + this.configStream = new FileInputStream(fileName); + } +} + +// Good. +// - Testing this class is as easy as using ByteArrayInputStream with a String. +class ConfigReader { + private final InputStream configStream; + ConfigReader(InputStream configStream){ + this.configStream = checkNotNull(configStream); + } +} +``` ### Testing multithreaded code Testing code that uses multiple threads is notoriously hard. When approached carefully, however, @@ -763,33 +789,34 @@ always be checked against null, unless null is explicitly allowed. *See also [be wary of null](#be-wary-of-null), [@Nullable](#nullable)* - :::java - // Bad. - // - If the file or callback are null, the problem isn't noticed until much later. - class AsyncFileReader { - void readLater(File file, Closure callback) { - scheduledExecutor.schedule(new Runnable() { - @Override public void run() { - callback.execute(readSync(file)); - } - }, 1L, TimeUnit.HOURS); - } - } - - // Good. - class AsyncFileReader { - void readLater(File file, Closure callback) { - checkNotNull(file); - checkArgument(file.exists() && file.canRead(), "File must exist and be readable."); - checkNotNull(callback); - - scheduledExecutor.schedule(new Runnable() { - @Override public void run() { - callback.execute(readSync(file)); - } - }, 1L, TimeUnit.HOURS); - } - } +```java +// Bad. +// - If the file or callback are null, the problem isn't noticed until much later. +class AsyncFileReader { + void readLater(File file, Closure callback) { + scheduledExecutor.schedule(new Runnable() { + @Override public void run() { + callback.execute(readSync(file)); + } + }, 1L, TimeUnit.HOURS); + } +} + +// Good. +class AsyncFileReader { + void readLater(File file, Closure callback) { + checkNotNull(file); + checkArgument(file.exists() && file.canRead(), "File must exist and be readable."); + checkNotNull(callback); + + scheduledExecutor.schedule(new Runnable() { + @Override public void run() { + callback.execute(readSync(file)); + } + }, 1L, TimeUnit.HOURS); + } +} +``` #### Minimize visibility @@ -797,68 +824,70 @@ In a class API, you should support access to any methods and fields that you mak Therefore, only expose what you intend the caller to use. This can be imperative when writing thread-safe code. - :::java - public class Parser { - // Bad. - // - Callers can directly access and mutate, possibly breaking internal assumptions. - public Map rawFields; - - // Bad. - // - This is probably intended to be an internal utility function. - public String readConfigLine() { - .. - } - } - - // Good. - // - rawFields and the utility function are hidden - // - The class is package-private, indicating that it should only be accessed indirectly. - class Parser { - private final Map rawFields; - - private String readConfigLine() { - .. - } - } +```java +public class Parser { + // Bad. + // - Callers can directly access and mutate, possibly breaking internal assumptions. + public Map rawFields; + + // Bad. + // - This is probably intended to be an internal utility function. + public String readConfigLine() { + .. + } +} + +// Good. +// - rawFields and the utility function are hidden +// - The class is package-private, indicating that it should only be accessed indirectly. +class Parser { + private final Map rawFields; + + private String readConfigLine() { + .. + } +} +``` #### Favor immutability Mutable objects carry a burden - you need to make sure that those who are *able* to mutate it are not violating expectations of other users of the object, and that it's even safe for them to modify. - :::java - // Bad. - // - Anyone with a reference to User can modify the user's birthday. - // - Calling getAttributes() gives mutable access to the underlying map. - public class User { - public Date birthday; - private final Map attributes = Maps.newHashMap(); - - ... - - public Map getAttributes() { - return attributes; - } - } - - // Good. - public class User { - private final Date birthday; - private final Map attributes = Maps.newHashMap(); - - ... - - public Map getAttributes() { - return ImmutableMap.copyOf(attributes); - } - - // If you realize the users don't need the full map, you can avoid the map copy - // by providing access to individual members. - @Nullable - public String getAttribute(String attributeName) { - return attributes.get(attributeName); - } - } +```java +// Bad. +// - Anyone with a reference to User can modify the user's birthday. +// - Calling getAttributes() gives mutable access to the underlying map. +public class User { + public Date birthday; + private final Map attributes = Maps.newHashMap(); + + ... + + public Map getAttributes() { + return attributes; + } +} + +// Good. +public class User { + private final Date birthday; + private final Map attributes = Maps.newHashMap(); + + ... + + public Map getAttributes() { + return ImmutableMap.copyOf(attributes); + } + + // If you realize the users don't need the full map, you can avoid the map copy + // by providing access to individual members. + @Nullable + public String getAttribute(String attributeName) { + return attributes.get(attributeName); + } +} +``` #### Be wary of null Use `@Nullable` where prudent, but favor @@ -867,49 +896,51 @@ over `@Nullable`. `Optional` provides better semantics around absence of a valu #### Clean up with finally - :::java - FileInputStream in = null; - try { - ... - } catch (IOException e) { - ... - } finally { - Closeables.closeQuietly(in); - } +```java +FileInputStream in = null; +try { + ... +} catch (IOException e) { + ... +} finally { + Closeables.closeQuietly(in); +} +``` Even if there are no checked exceptions, there are still cases where you should use try/finally to guarantee resource symmetry. - :::java - // Bad. - // - Mutex is never unlocked. - mutex.lock(); - throw new NullPointerException(); - mutex.unlock(); - - // Good. - mutex.lock(); - try { - throw new NullPointerException(); - } finally { - mutex.unlock(); - } - - // Bad. - // - Connection is not closed if sendMessage throws. - if (receivedBadMessage) { - conn.sendMessage("Bad request."); - conn.close(); - } - - // Good. - if (receivedBadMessage) { - try { - conn.sendMessage("Bad request."); - } finally { - conn.close(); - } - } +```java +// Bad. +// - Mutex is never unlocked. +mutex.lock(); +throw new NullPointerException(); +mutex.unlock(); + +// Good. +mutex.lock(); +try { + throw new NullPointerException(); +} finally { + mutex.unlock(); +} + +// Bad. +// - Connection is not closed if sendMessage throws. +if (receivedBadMessage) { + conn.sendMessage("Bad request."); + conn.close(); +} + +// Good. +if (receivedBadMessage) { + try { + conn.sendMessage("Bad request."); + } finally { + conn.close(); + } +} +``` ### Clean code @@ -917,13 +948,14 @@ to guarantee resource symmetry. #### Disambiguate Favor readability - if there's an ambiguous and unambiguous route, always favor unambiguous. - :::java - // Bad. - // - Depending on the font, it may be difficult to discern 1001 from 100l. - long count = 100l + n; +```java +// Bad. +// - Depending on the font, it may be difficult to discern 1001 from 100l. +long count = 100l + n; - // Good. - long count = 100L + n; +// Good. +long count = 100L + n; +``` #### Remove dead code Delete unused code (imports, fields, parameters, methods, classes). They will only rot. @@ -933,19 +965,20 @@ When declaring fields and methods, it's better to use general types whenever pos This avoids implementation detail leak via your API, and allows you to change the types used internally without affecting users or peripheral code. - :::java - // Bad. - // - Implementations of Database must match the ArrayList return type. - // - Changing return type to Set or List could break implementations and users. - interface Database { - ArrayList fetchUsers(String query); - } +```java +// Bad. +// - Implementations of Database must match the ArrayList return type. +// - Changing return type to Set or List could break implementations and users. +interface Database { + ArrayList fetchUsers(String query); +} - // Good. - // - Iterable defines the minimal functionality required of the return. - interface Database { - Iterable fetchUsers(String query); - } +// Good. +// - Iterable defines the minimal functionality required of the return. +interface Database { + Iterable fetchUsers(String query); +} +``` #### Always use type parameters @@ -997,20 +1030,21 @@ as you can end up catching more than you really wanted to deal with. For exampl `catch Exception` would capture `NullPointerException`, and `catch Throwable` would capture `OutOfMemoryError`. - :::java - // Bad. - // - If a RuntimeException happens, the program continues rather than aborting. - try { - storage.insertUser(user); - } catch (Exception e) { - LOG.error("Failed to insert user."); - } - - try { - storage.insertUser(user); - } catch (StorageException e) { - LOG.error("Failed to insert user."); - } +```java +// Bad. +// - If a RuntimeException happens, the program continues rather than aborting. +try { + storage.insertUser(user); +} catch (Exception e) { + LOG.error("Failed to insert user."); +} + +try { + storage.insertUser(user); +} catch (StorageException e) { + LOG.error("Failed to insert user."); +} +``` ##### Don't swallow exceptions An empty `catch` block is usually a bad idea, as you have no signal of a problem. Coupled with @@ -1025,23 +1059,24 @@ it is good practice to ensure that the thread interrupted state is preserved. IBM has a good [article](http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html) on this topic. - :::java - // Bad. - // - Surrounding code (or higher-level code) has no idea that the thread was interrupted. - try { - lock.tryLock(1L, TimeUnit.SECONDS) - } catch (InterruptedException e) { - LOG.info("Interrupted while doing x"); - } - - // Good. - // - Interrupted state is preserved. - try { - lock.tryLock(1L, TimeUnit.SECONDS) - } catch (InterruptedException e) { - LOG.info("Interrupted while doing x"); - Thread.currentThread().interrupt(); - } +```java +// Bad. +// - Surrounding code (or higher-level code) has no idea that the thread was interrupted. +try { + lock.tryLock(1L, TimeUnit.SECONDS) +} catch (InterruptedException e) { + LOG.info("Interrupted while doing x"); +} + +// Good. +// - Interrupted state is preserved. +try { + lock.tryLock(1L, TimeUnit.SECONDS) +} catch (InterruptedException e) { + LOG.info("Interrupted while doing x"); + Thread.currentThread().interrupt(); +} +``` ##### Throw appropriate exception types Let your API users obey [catch narrow exceptions](#catch-narrow-exceptions), don't throw Exception. @@ -1049,29 +1084,30 @@ Even if you are calling another naughty API that throws Exception, at least hide bubble up even further. You should also make an effort to hide implementation details from your callers when it comes to exceptions. - :::java - // Bad. - // - Caller is forced to catch Exception, trapping many unnecessary types of issues. - interface DataStore { - String fetchValue(String key) throws Exception; - } - - // Better. - // - The interface leaks details about one specific implementation. - interface DataStore { - String fetchValue(String key) throws SQLException, UnknownHostException; - } - - // Good. - // - A custom exception type insulates the user from the implementation. - // - Different implementations aren't forced to abuse irrelevant exception types. - interface DataStore { - String fetchValue(String key) throws StorageException; - - static class StorageException extends Exception { - ... - } - } +```java +// Bad. +// - Caller is forced to catch Exception, trapping many unnecessary types of issues. +interface DataStore { + String fetchValue(String key) throws Exception; +} + +// Better. +// - The interface leaks details about one specific implementation. +interface DataStore { + String fetchValue(String key) throws SQLException, UnknownHostException; +} + +// Good. +// - A custom exception type insulates the user from the implementation. +// - Different implementations aren't forced to abuse irrelevant exception types. +interface DataStore { + String fetchValue(String key) throws StorageException; + + static class StorageException extends Exception { + ... + } +} +``` ### Use newer/better libraries @@ -1130,13 +1166,14 @@ debugging. #### Leave no TODO unassigned TODOs should have owners, otherwise they are unlikely to ever be resolved. - :::java - // Bad. - // - TODO is unassigned. - // TODO: Implement request backoff. +```java +// Bad. +// - TODO is unassigned. +// TODO: Implement request backoff. - // Good. - // TODO(George Washington): Implement request backoff. +// Good. +// TODO(George Washington): Implement request backoff. +``` #### Adopt TODOs You should adopt an orphan if the owner has left the company/project, or if you make @@ -1153,27 +1190,28 @@ but it can also hide in constructors or methods that take few parameters. The k to defer assembly to the layers of the code that know enough to assemble and instead just take the minimal interface you need to get your work done. - :::java - // Bad. - // - Weigher uses hosts and port only to immediately construct another object. - class Weigher { - private final double defaultInitialRate; - - Weigher(Iterable hosts, int port, double defaultInitialRate) { - this.defaultInitialRate = validateRate(defaultInitialRate); - this.weightingService = createWeightingServiceClient(hosts, port); - } - } - - // Good. - class Weigher { - private final double defaultInitialRate; - - Weigher(WeightingService weightingService, double defaultInitialRate) { - this.defaultInitialRate = validateRate(defaultInitialRate); - this.weightingService = checkNotNull(weightingService); - } - } +```java +// Bad. +// - Weigher uses hosts and port only to immediately construct another object. +class Weigher { + private final double defaultInitialRate; + + Weigher(Iterable hosts, int port, double defaultInitialRate) { + this.defaultInitialRate = validateRate(defaultInitialRate); + this.weightingService = createWeightingServiceClient(hosts, port); + } +} + +// Good. +class Weigher { + private final double defaultInitialRate; + + Weigher(WeightingService weightingService, double defaultInitialRate) { + this.defaultInitialRate = validateRate(defaultInitialRate); + this.weightingService = checkNotNull(weightingService); + } +} +``` If you want to provide a convenience constructor, a factory method or an external factory in the form of a builder you still can, but by making the fundamental constructor of a @@ -1187,50 +1225,52 @@ like code and more like english, the extracted sites are often easier to flow-an human eyes. The classic case is branched variable assignment. In the extreme, never do this: - :::java - void calculate(Subject subject) { - double weight; - if (useWeightingService(subject)) { - try { - weight = weightingService.weight(subject.id); - } catch (RemoteException e) { - throw new LayerSpecificException("Failed to look up weight for " + subject, e) - } - } else { - weight = defaultInitialRate * (1 + onlineLearnedBoost); - } - - // Use weight here for further calculations - } +```java +void calculate(Subject subject) { + double weight; + if (useWeightingService(subject)) { + try { + weight = weightingService.weight(subject.id); + } catch (RemoteException e) { + throw new LayerSpecificException("Failed to look up weight for " + subject, e) + } + } else { + weight = defaultInitialRate * (1 + onlineLearnedBoost); + } + + // Use weight here for further calculations +} +``` Instead do this: - :::java - void calculate(Subject subject) { - double weight = calculateWeight(subject); - - // Use weight here for further calculations - } - - private double calculateWeight(Subject subject) throws LayerSpecificException { - if (useWeightingService(subject)) { - return fetchSubjectWeight(subject.id) - } else { - return currentDefaultRate(); - } - } - - private double fetchSubjectWeight(long subjectId) { - try { - return weightingService.weight(subjectId); - } catch (RemoteException e) { - throw new LayerSpecificException("Failed to look up weight for " + subject, e) - } - } - - private double currentDefaultRate() { - defaultInitialRate * (1 + onlineLearnedBoost); - } +```java +void calculate(Subject subject) { + double weight = calculateWeight(subject); + + // Use weight here for further calculations +} + +private double calculateWeight(Subject subject) throws LayerSpecificException { + if (useWeightingService(subject)) { + return fetchSubjectWeight(subject.id) + } else { + return currentDefaultRate(); + } +} + +private double fetchSubjectWeight(long subjectId) { + try { + return weightingService.weight(subjectId); + } catch (RemoteException e) { + throw new LayerSpecificException("Failed to look up weight for " + subject, e) + } +} + +private double currentDefaultRate() { + defaultInitialRate * (1 + onlineLearnedBoost); +} +``` A code reader that generally trusts methods do what they say can scan calculate quickly now and drill down only to those methods where I want to learn more. @@ -1264,40 +1304,43 @@ registering with ### Avoid unnecessary code #### Superfluous temporary variables. - :::java - // Bad. - // - The variable is immediately returned, and just serves to clutter the code. - List strings = fetchStrings(); - return strings; +```java +// Bad. +// - The variable is immediately returned, and just serves to clutter the code. +List strings = fetchStrings(); +return strings; - // Good. - return fetchStrings(); +// Good. +return fetchStrings(); +``` #### Unneeded assignment. - :::java - // Bad. - // - The null value is never realized. - String value = null; - try { - value = "The value is " + parse(foo); - } catch (BadException e) { - throw new IllegalStateException(e); - } - - // Good - String value; - try { - value = "The value is " + parse(foo); - } catch (BadException e) { - throw new IllegalStateException(e); - } +```java +// Bad. +// - The null value is never realized. +String value = null; +try { + value = "The value is " + parse(foo); +} catch (BadException e) { + throw new IllegalStateException(e); +} + +// Good +String value; +try { + value = "The value is " + parse(foo); +} catch (BadException e) { + throw new IllegalStateException(e); +} +``` ### The 'fast' implementation Don't bewilder your API users with a 'fast' or 'optimized' implementation of a method. - :::java - int fastAdd(Iterable ints); +```java +int fastAdd(Iterable ints); - // Why would the caller ever use this when there's a 'fast' add? - int add(Iterable ints); +// Why would the caller ever use this when there's a 'fast' add? +int add(Iterable ints); +```