Java Style Guide: Logging, Support Annotations
Plus re-ordered content a bit. Change-Id: I363ffcb5c2a4cb36afcb980503a12564d9e7199b Reviewed-on: https://chromium-review.googlesource.com/1162112 Commit-Queue: agrieve <agrieve@chromium.org> Reviewed-by: Tommy Nyquist <nyquist@chromium.org> Cr-Commit-Position: refs/heads/master@{#583120}
This commit is contained in:
agrieve
@ -40,18 +40,79 @@ is encouraged, but there are some gotchas:
|
||||
* java.util.zip.ZipFile (implemented in API 19)
|
||||
* java.net.Socket (implemented in API 19)
|
||||
|
||||
## Other Language Features & APIs
|
||||
|
||||
### Exceptions
|
||||
* As with the Android style guide, we discourage overly broad catches via
|
||||
`Exception` / `Throwable` / `RuntimeException`.
|
||||
* If you need to have a broad catch expression, use a comment to explain why.
|
||||
* Catching multiple exceptions in one line is fine.
|
||||
|
||||
It is OK to do:
|
||||
```java
|
||||
try {
|
||||
somethingThatThrowsIOException(filePath);
|
||||
somethingThatThrowsParseException(filePath);
|
||||
} catch (IOException | ParseException e) {
|
||||
Log.w(TAG, "Failed to read: %s", filePath, e);
|
||||
}
|
||||
```
|
||||
|
||||
### Logging
|
||||
* Use `org.chromium.base.Log` instead of `android.util.Log`.
|
||||
* It provides `%s` support, and ensures log stripping works correctly.
|
||||
* Minimize the use of `Log.w()` and `Log.e()`.
|
||||
* Debug and Info log levels are stripped by ProGuard in release builds, and
|
||||
so have no performance impact for shipping builds. However, Warning and
|
||||
Error log levels are not stripped.
|
||||
* Function calls in log parameters are *not* stripped by ProGuard.
|
||||
|
||||
```java
|
||||
Log.d(TAG, "There are %d cats", countCats()); // countCats() not stripped.
|
||||
```
|
||||
|
||||
### Asserts
|
||||
The Chromium build system strips asserts in release builds (via ProGuard) and
|
||||
enables them in debug builds (or when `dcheck_always_on=true`) (via a [build
|
||||
step](https://codereview.chromium.org/2517203002)). You should use asserts in
|
||||
the [same
|
||||
scenarios](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED)
|
||||
where C++ DCHECK()s make sense. For multi-statement asserts, use
|
||||
`org.chromium.base.BuildConfig.DCHECK_IS_ON` to guard your code.
|
||||
|
||||
Example assert:
|
||||
|
||||
```java
|
||||
assert someCallWithoutSideEffects() : "assert description";
|
||||
```
|
||||
|
||||
Example use of `DCHECK_IS_ON`:
|
||||
|
||||
```java
|
||||
if (org.chromium.base.BuildConfig.DCHECK_IS_ON) {
|
||||
// Any code here will be stripped in Release by ProGuard.
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
### Other Android Support Library Annotations
|
||||
* Use them! They are [documented here](https://developer.android.com/studio/write/annotations).
|
||||
* They generally improve readability.
|
||||
* Some make lint more useful.
|
||||
* `javax.annotation.Nullable` vs `android.support.annotation.Nullable`
|
||||
* Always prefer `android.support.annotation.Nullable`.
|
||||
* It uses `@Retention(SOURCE)` rather than `@Retention(RUNTIME)`.
|
||||
|
||||
## Tools
|
||||
|
||||
### Automatically formatting edited files
|
||||
|
||||
A checkout should give you clang-format to automatically format Java code.
|
||||
It is suggested that Clang's formatting of code should be accepted in code
|
||||
reviews.
|
||||
|
||||
You can run `git cl format` to apply the automatic formatting.
|
||||
|
||||
### IDE setup
|
||||
|
||||
### IDE Setup
|
||||
For automatically using the correct style, follow the guide to set up your
|
||||
favorite IDE:
|
||||
|
||||
@ -59,31 +120,29 @@ favorite IDE:
|
||||
* [Eclipse](https://chromium.googlesource.com/chromium/src/+/master/docs/eclipse.md)
|
||||
|
||||
### Checkstyle
|
||||
|
||||
Checkstyle is automatically run by the build bots, and to ensure you do not have
|
||||
any surprises, you can also set up checkstyle locally using [this
|
||||
guide](https://sites.google.com/a/chromium.org/dev/developers/checkstyle).
|
||||
|
||||
### Lint
|
||||
|
||||
Lint is run as part of the build. For more information, see
|
||||
[here](https://chromium.googlesource.com/chromium/src/+/master/build/android/docs/lint.md).
|
||||
|
||||
## File Headers
|
||||
## Style / Formatting
|
||||
|
||||
Use the same format as in the [C++ style guide](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#File-headers).
|
||||
### File Headers
|
||||
* Use the same format as in the [C++ style guide](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#File-headers).
|
||||
|
||||
## TODOs
|
||||
|
||||
TODO should follow chromium convention i.e. `TODO(username)`.
|
||||
|
||||
## Code formatting
|
||||
### TODOs
|
||||
* TODO should follow chromium convention. Examples:
|
||||
* `TODO(username): Some sentence here.`
|
||||
* `TODO(crbug.com/123456): Even better to use a bug for context.`
|
||||
|
||||
### Code formatting
|
||||
* Fields should not be explicitly initialized to default values (see
|
||||
[here](https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ylbLOvLs0bs/discussion)).
|
||||
|
||||
### Curly braces
|
||||
|
||||
Conditional braces should be used, but are optional if the conditional and the
|
||||
statement can be on a single line.
|
||||
|
||||
@ -109,49 +168,7 @@ if (someConditional)
|
||||
return false;
|
||||
```
|
||||
|
||||
### Exceptions
|
||||
|
||||
Similarly to the Android style guide, we discourage the use of
|
||||
`catch Exception`. It is also allowed to catch multiple exceptions in one line.
|
||||
|
||||
It is OK to do:
|
||||
|
||||
```java
|
||||
try {
|
||||
somethingThatThrowsIOException();
|
||||
somethingThatThrowsParseException();
|
||||
} catch (IOException | ParseException e) {
|
||||
Log.e(TAG, "Failed to do something with exception: ", e);
|
||||
}
|
||||
```
|
||||
|
||||
### Asserts
|
||||
|
||||
The Chromium build system strips asserts in release builds (via ProGuard) and
|
||||
enables them in debug builds (or when `dcheck_always_on=true`) (via a [build
|
||||
step](https://codereview.chromium.org/2517203002)). You should use asserts in
|
||||
the [same
|
||||
scenarios](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED)
|
||||
where C++ DCHECK()s make sense. For multi-statement asserts, use
|
||||
`org.chromium.base.BuildConfig.DCHECK_IS_ON` to guard your code.
|
||||
|
||||
Example assert:
|
||||
|
||||
```java
|
||||
assert someCallWithoutSideEffects() : "assert description";
|
||||
```
|
||||
|
||||
Example use of `DCHECK_IS_ON`:
|
||||
|
||||
```java
|
||||
if (org.chromium.base.BuildConfig.DCHECK_IS_ON) {
|
||||
// Any code here will be stripped in Release by ProGuard.
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
### Import Order
|
||||
|
||||
* Static imports go before other imports.
|
||||
* Each import group must be separated by an empty line.
|
||||
|
||||
@ -172,7 +189,6 @@ This is enforced by the
|
||||
under the ImportOrder module.
|
||||
|
||||
## Location
|
||||
|
||||
"Top level directories" are defined as directories with a GN file, such as
|
||||
[//base](https://chromium.googlesource.com/chromium/src/+/master/base/)
|
||||
and
|
||||
@ -197,5 +213,4 @@ much like
|
||||
[//base/android/OWNERS](https://chromium.googlesource.com/chromium/src/+/master/base/android/OWNERS).
|
||||
|
||||
## Miscellany
|
||||
|
||||
* Use UTF-8 file encodings and LF line endings.
|
||||
|
||||
Reference in New Issue
Block a user