nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 1 | # Chromium Java style guide |
| 2 | |
| 3 | _For other languages, please see the [Chromium style |
| 4 | guides](https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md)._ |
| 5 | |
| 6 | Chromium follows the [Android Open Source style |
| 7 | guide](http://source.android.com/source/code-style.html) unless an exception |
| 8 | is listed below. |
| 9 | |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 10 | You can propose changes to this style guide by sending an email to |
| 11 | `[email protected]`. Ideally, the list will arrive at some consensus and you can |
| 12 | request review for a change to this file. If there's no consensus, |
| 13 | [`//styleguide/java/OWNERS`](https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/OWNERS) |
| 14 | get to decide. |
| 15 | |
agrieve | 0e6bdf2 | 2018-08-03 14:25:24 | [diff] [blame] | 16 | [TOC] |
| 17 | |
| 18 | ## Java 8 Language Features |
| 19 | [Desugar](https://github.com/bazelbuild/bazel/blob/master/src/tools/android/java/com/google/devtools/build/android/desugar/Desugar.java) |
| 20 | is used to rewrite some Java 7 & 8 language constructs in a way that is |
| 21 | compatible with Java 6 (and thus all Android versions). Use of |
| 22 | [these features](https://developer.android.com/studio/write/java8-support) |
| 23 | is encouraged, but there are some gotchas: |
| 24 | |
| 25 | ### Default Interface Methods |
| 26 | * Desugar makes default interface methods work by copy & pasting the default |
| 27 | implementations into all implementing classes. |
| 28 | * This technique is fine for infrequently-used interfaces, but should be |
| 29 | avoided (e.g. via a base class) if it noticeably increases method count. |
| 30 | |
| 31 | ### Lambdas and Method References |
| 32 | * These are syntactic sugar for creating anonymous inner classes. |
| 33 | * Use them only where the cost of an extra class & method definition is |
| 34 | justified. |
| 35 | |
| 36 | ### try-with-resources |
| 37 | * Some library classes do not implement Closeable on older platform APIs. |
| 38 | Runtime exceptions are thrown if you use them with a try-with-resources. |
| 39 | Do not use the following classes in a try-with-resources: |
| 40 | * java.util.zip.ZipFile (implemented in API 19) |
| 41 | * java.net.Socket (implemented in API 19) |
| 42 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 43 | ## Other Language Features & APIs |
| 44 | |
| 45 | ### Exceptions |
| 46 | * As with the Android style guide, we discourage overly broad catches via |
| 47 | `Exception` / `Throwable` / `RuntimeException`. |
| 48 | * If you need to have a broad catch expression, use a comment to explain why. |
| 49 | * Catching multiple exceptions in one line is fine. |
| 50 | |
| 51 | It is OK to do: |
| 52 | ```java |
| 53 | try { |
| 54 | somethingThatThrowsIOException(filePath); |
| 55 | somethingThatThrowsParseException(filePath); |
| 56 | } catch (IOException | ParseException e) { |
| 57 | Log.w(TAG, "Failed to read: %s", filePath, e); |
| 58 | } |
| 59 | ``` |
| 60 | |
agrieve | 50430de | 2018-08-15 17:49:16 | [diff] [blame] | 61 | * Avoid adding messages to exceptions that do not aid in debugging. |
| 62 | |
| 63 | For example: |
| 64 | ```java |
| 65 | try { |
| 66 | somethingThatThrowsIOException(); |
| 67 | } catch (IOException e) { |
| 68 | // Bad - message does not tell you more than the stack trace does: |
| 69 | throw new RuntimeException("Failed to parse a file.", e); |
| 70 | // Good - conveys that this block failed along with the "caused by" exception. |
| 71 | throw new RuntimeException(e); |
| 72 | // Good - adds useful information. |
| 73 | throw new RuntimeException(String.format("Failed to parse %s", fileName), e); |
| 74 | } |
| 75 | ``` |
| 76 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 77 | ### Logging |
| 78 | * Use `org.chromium.base.Log` instead of `android.util.Log`. |
| 79 | * It provides `%s` support, and ensures log stripping works correctly. |
| 80 | * Minimize the use of `Log.w()` and `Log.e()`. |
| 81 | * Debug and Info log levels are stripped by ProGuard in release builds, and |
| 82 | so have no performance impact for shipping builds. However, Warning and |
| 83 | Error log levels are not stripped. |
| 84 | * Function calls in log parameters are *not* stripped by ProGuard. |
| 85 | |
| 86 | ```java |
| 87 | Log.d(TAG, "There are %d cats", countCats()); // countCats() not stripped. |
| 88 | ``` |
| 89 | |
| 90 | ### Asserts |
| 91 | The Chromium build system strips asserts in release builds (via ProGuard) and |
| 92 | enables them in debug builds (or when `dcheck_always_on=true`) (via a [build |
| 93 | step](https://codereview.chromium.org/2517203002)). You should use asserts in |
| 94 | the [same |
| 95 | scenarios](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED) |
| 96 | where C++ DCHECK()s make sense. For multi-statement asserts, use |
| 97 | `org.chromium.base.BuildConfig.DCHECK_IS_ON` to guard your code. |
| 98 | |
| 99 | Example assert: |
| 100 | |
| 101 | ```java |
| 102 | assert someCallWithoutSideEffects() : "assert description"; |
| 103 | ``` |
| 104 | |
| 105 | Example use of `DCHECK_IS_ON`: |
| 106 | |
| 107 | ```java |
| 108 | if (org.chromium.base.BuildConfig.DCHECK_IS_ON) { |
| 109 | // Any code here will be stripped in Release by ProGuard. |
| 110 | ... |
| 111 | } |
| 112 | ``` |
| 113 | |
agrieve | 16c6fe8 | 2018-11-27 17:47:49 | [diff] [blame] | 114 | ### Finalizers |
| 115 | In line with [Google's Java style guide](https://google.github.io/styleguide/javaguide.html#s6.4-finalizers), |
| 116 | never override `Object.finalize()`. |
| 117 | |
| 118 | Custom finalizers: |
| 119 | * are called on a background thread, and at an unpredicatble point in time, |
| 120 | * swallow all exceptions (asserts won't work), |
| 121 | * causes additional garbage collector jank. |
| 122 | |
| 123 | Classes that need destructor logic should provide an explicit `destroy()` |
Bo Liu | 9bb53ca | 2020-09-22 00:48:10 | [diff] [blame^] | 124 | method. Use [LifetimeAssert](https://chromium.googlesource.com/chromium/src/+/master/base/android/java/src/org/chromium/base/LifetimeAssert.java) |
| 125 | to ensure in debug builds and tests that `destroy()` is called. |
agrieve | 16c6fe8 | 2018-11-27 17:47:49 | [diff] [blame] | 126 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 127 | ### Other Android Support Library Annotations |
| 128 | * Use them! They are [documented here](https://developer.android.com/studio/write/annotations). |
| 129 | * They generally improve readability. |
| 130 | * Some make lint more useful. |
| 131 | * `javax.annotation.Nullable` vs `android.support.annotation.Nullable` |
| 132 | * Always prefer `android.support.annotation.Nullable`. |
| 133 | * It uses `@Retention(SOURCE)` rather than `@Retention(RUNTIME)`. |
| 134 | |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 135 | ## Tools |
| 136 | |
| 137 | ### Automatically formatting edited files |
agrieve | 0e6bdf2 | 2018-08-03 14:25:24 | [diff] [blame] | 138 | A checkout should give you clang-format to automatically format Java code. |
| 139 | It is suggested that Clang's formatting of code should be accepted in code |
| 140 | reviews. |
| 141 | |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 142 | You can run `git cl format` to apply the automatic formatting. |
| 143 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 144 | ### IDE Setup |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 145 | For automatically using the correct style, follow the guide to set up your |
| 146 | favorite IDE: |
| 147 | |
| 148 | * [Android Studio](https://chromium.googlesource.com/chromium/src/+/master/docs/android_studio.md) |
| 149 | * [Eclipse](https://chromium.googlesource.com/chromium/src/+/master/docs/eclipse.md) |
| 150 | |
| 151 | ### Checkstyle |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 152 | Checkstyle is automatically run by the build bots, and to ensure you do not have |
| 153 | any surprises, you can also set up checkstyle locally using [this |
| 154 | guide](https://sites.google.com/a/chromium.org/dev/developers/checkstyle). |
| 155 | |
| 156 | ### Lint |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 157 | Lint is run as part of the build. For more information, see |
| 158 | [here](https://chromium.googlesource.com/chromium/src/+/master/build/android/docs/lint.md). |
| 159 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 160 | ## Style / Formatting |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 161 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 162 | ### File Headers |
| 163 | * Use the same format as in the [C++ style guide](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#File-headers). |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 164 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 165 | ### TODOs |
| 166 | * TODO should follow chromium convention. Examples: |
| 167 | * `TODO(username): Some sentence here.` |
| 168 | * `TODO(crbug.com/123456): Even better to use a bug for context.` |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 169 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 170 | ### Code formatting |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 171 | * Fields should not be explicitly initialized to default values (see |
| 172 | [here](https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ylbLOvLs0bs/discussion)). |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 173 | |
| 174 | ### Curly braces |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 175 | Conditional braces should be used, but are optional if the conditional and the |
| 176 | statement can be on a single line. |
| 177 | |
| 178 | Do: |
| 179 | |
| 180 | ```java |
| 181 | if (someConditional) return false; |
| 182 | for (int i = 0; i < 10; ++i) callThing(i); |
| 183 | ``` |
| 184 | |
| 185 | or |
| 186 | |
| 187 | ```java |
| 188 | if (someConditional) { |
| 189 | return false; |
| 190 | } |
| 191 | ``` |
| 192 | |
| 193 | Do NOT do: |
| 194 | |
| 195 | ```java |
| 196 | if (someConditional) |
| 197 | return false; |
| 198 | ``` |
| 199 | |
nyquist | 2d192c4c | 2017-03-06 21:36:51 | [diff] [blame] | 200 | ### Import Order |
nyquist | 2d192c4c | 2017-03-06 21:36:51 | [diff] [blame] | 201 | * Static imports go before other imports. |
| 202 | * Each import group must be separated by an empty line. |
| 203 | |
| 204 | This is the order of the import groups: |
| 205 | |
| 206 | 1. android |
Yun Liu | f40227d9 | 2019-04-04 17:37:46 | [diff] [blame] | 207 | 1. androidx |
nyquist | 2d192c4c | 2017-03-06 21:36:51 | [diff] [blame] | 208 | 1. com (except com.google.android.apps.chrome) |
| 209 | 1. dalvik |
| 210 | 1. junit |
| 211 | 1. org |
| 212 | 1. com.google.android.apps.chrome |
| 213 | 1. org.chromium |
| 214 | 1. java |
| 215 | 1. javax |
| 216 | |
Caitlin Fischer | 210cfab | 2020-05-07 20:04:30 | [diff] [blame] | 217 | ## Test-only Code |
| 218 | Functions used only for testing should be restricted to test-only usages |
Caitlin Fischer | 7776cfd | 2020-05-08 03:15:24 | [diff] [blame] | 219 | with the testing suffixes supported [PRESUMBIT.py](https://chromium.googlesource.com/chromium/src/+/master/PRESUBMIT.py). |
Caitlin Fischer | 210cfab | 2020-05-07 20:04:30 | [diff] [blame] | 220 | `ForTesting` is the conventional suffix although similar patterns, such as |
| 221 | `ForTest`, are also accepted. These suffixes are checked at presubmit time |
| 222 | to ensure the functions are called only by test files. |
| 223 | |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 224 | ## Location |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 225 | "Top level directories" are defined as directories with a GN file, such as |
| 226 | [//base](https://chromium.googlesource.com/chromium/src/+/master/base/) |
| 227 | and |
| 228 | [//content](https://chromium.googlesource.com/chromium/src/+/master/content/), |
| 229 | Chromium Java should live in a directory named |
| 230 | `<top level directory>/android/java`, with a package name |
| 231 | `org.chromium.<top level directory>`. Each top level directory's Java should |
| 232 | build into a distinct JAR that honors the abstraction specified in a native |
| 233 | [checkdeps](https://chromium.googlesource.com/chromium/buildtools/+/master/checkdeps/checkdeps.py) |
| 234 | (e.g. `org.chromium.base` does not import `org.chromium.content`). The full |
| 235 | path of any java file should contain the complete package name. |
| 236 | |
| 237 | For example, top level directory `//base` might contain a file named |
| 238 | `base/android/java/org/chromium/base/Class.java`. This would get compiled into a |
| 239 | `chromium_base.jar` (final JAR name TBD). |
| 240 | |
| 241 | `org.chromium.chrome.browser.foo.Class` would live in |
| 242 | `chrome/android/java/org/chromium/chrome/browser/foo/Class.java`. |
| 243 | |
| 244 | New `<top level directory>/android` directories should have an `OWNERS` file |
| 245 | much like |
| 246 | [//base/android/OWNERS](https://chromium.googlesource.com/chromium/src/+/master/base/android/OWNERS). |
| 247 | |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 248 | ## Miscellany |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 249 | * Use UTF-8 file encodings and LF line endings. |