Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 1 | page.title=Code Style for Contributors |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 2 | @jd:body |
| 3 | |
| 4 | <!-- |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 5 | Copyright 2015 The Android Open Source Project |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 6 | |
| 7 | Licensed under the Apache License, Version 2.0 (the "License"); |
| 8 | you may not use this file except in compliance with the License. |
| 9 | You may obtain a copy of the License at |
| 10 | |
| 11 | http://www.apache.org/licenses/LICENSE-2.0 |
| 12 | |
| 13 | Unless required by applicable law or agreed to in writing, software |
| 14 | distributed under the License is distributed on an "AS IS" BASIS, |
| 15 | WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. |
| 16 | See the License for the specific language governing permissions and |
| 17 | limitations under the License. |
| 18 | --> |
| 19 | <div id="qv-wrapper"> |
| 20 | <div id="qv"> |
| 21 | <h2>In this document</h2> |
| 22 | <ol id="auto-toc"> |
| 23 | </ol> |
| 24 | </div> |
| 25 | </div> |
| 26 | |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 27 | <p>The code styles below are strict rules, not guidelines or recommendations. |
| 28 | Contributions to Android that do not adhere to these rules are generally <em>not |
| 29 | accepted</em>. We recognize that not all existing code follows these rules, but |
| 30 | we expect all new code to be compliant.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 31 | |
| 32 | <h2 id="java-language-rules">Java Language Rules</h2> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 33 | <p>Android follows standard Java coding conventions with the additional rules |
| 34 | described below.</p> |
| 35 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 36 | <h3 id="dont-ignore-exceptions">Don't Ignore Exceptions</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 37 | <p>It can be tempting to write code that completely ignores an exception, such |
| 38 | as:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 39 | <pre><code>void setServerPort(String value) { |
| 40 | try { |
| 41 | serverPort = Integer.parseInt(value); |
| 42 | } catch (NumberFormatException e) { } |
| 43 | } |
| 44 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 45 | <p>Do not do this. While you may think your code will never encounter this error |
| 46 | condition or that it is not important to handle it, ignoring exceptions as above |
| 47 | creates mines in your code for someone else to trigger some day. You must handle |
| 48 | every Exception in your code in a principled way; the specific handling varies |
| 49 | depending on the case.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 50 | <p><em>Anytime somebody has an empty catch clause they should have a |
| 51 | creepy feeling. There are definitely times when it is actually the correct |
| 52 | thing to do, but at least you have to think about it. In Java you can't escape |
| 53 | the creepy feeling.</em> -<a href="http://www.artima.com/intv/solid4.html">James Gosling</a></p> |
| 54 | <p>Acceptable alternatives (in order of preference) are:</p> |
| 55 | <ul> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 56 | <li>Throw the exception up to the caller of your method. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 57 | <pre><code>void setServerPort(String value) throws NumberFormatException { |
| 58 | serverPort = Integer.parseInt(value); |
| 59 | } |
| 60 | </code></pre> |
| 61 | </li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 62 | <li>Throw a new exception that's appropriate to your level of abstraction. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 63 | <pre><code>void setServerPort(String value) throws ConfigurationException { |
| 64 | try { |
| 65 | serverPort = Integer.parseInt(value); |
| 66 | } catch (NumberFormatException e) { |
| 67 | throw new ConfigurationException("Port " + value + " is not valid."); |
| 68 | } |
| 69 | } |
| 70 | </code></pre> |
| 71 | </li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 72 | <li>Handle the error gracefully and substitute an appropriate value in the |
| 73 | catch {} block. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 74 | <pre><code>/** Set port. If value is not a valid number, 80 is substituted. */ |
| 75 | |
| 76 | void setServerPort(String value) { |
| 77 | try { |
| 78 | serverPort = Integer.parseInt(value); |
| 79 | } catch (NumberFormatException e) { |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 80 | serverPort = 80; // default port for server |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 81 | } |
| 82 | } |
| 83 | </code></pre> |
| 84 | </li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 85 | <li>Catch the Exception and throw a new <code>RuntimeException</code>. This is |
| 86 | dangerous, so do it only if you are positive that if this error occurs the |
| 87 | appropriate thing to do is crash. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 88 | <pre><code>/** Set port. If value is not a valid number, die. */ |
| 89 | |
| 90 | void setServerPort(String value) { |
| 91 | try { |
| 92 | serverPort = Integer.parseInt(value); |
| 93 | } catch (NumberFormatException e) { |
| 94 | throw new RuntimeException("port " + value " is invalid, ", e); |
| 95 | } |
| 96 | } |
| 97 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 98 | <p class="note"><strong>Note</strong> The original exception is passed to the |
| 99 | constructor for RuntimeException. If your code must compile under Java 1.3, you |
| 100 | must omit the exception that is the cause.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 101 | </li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 102 | <li>As a last resort, if you are confident that ignoring the exception is |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 103 | appropriate then you may ignore it, but you must also comment why with a good |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 104 | reason: |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 105 | <pre><code>/** If value is not a valid number, original port number is used. */ |
| 106 | void setServerPort(String value) { |
| 107 | try { |
| 108 | serverPort = Integer.parseInt(value); |
| 109 | } catch (NumberFormatException e) { |
| 110 | // Method is documented to just ignore invalid user input. |
| 111 | // serverPort will just be unchanged. |
| 112 | } |
| 113 | } |
| 114 | </code></pre> |
| 115 | </li> |
| 116 | </ul> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 117 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 118 | <h3 id="dont-catch-generic-exception">Don't Catch Generic Exception</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 119 | <p>It can also be tempting to be lazy when catching exceptions and do |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 120 | something like this:</p> |
| 121 | <pre><code>try { |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 122 | someComplicatedIOFunction(); // may throw IOException |
| 123 | someComplicatedParsingFunction(); // may throw ParsingException |
| 124 | someComplicatedSecurityFunction(); // may throw SecurityException |
| 125 | // phew, made it all the way |
| 126 | } catch (Exception e) { // I'll just catch all exceptions |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 127 | handleError(); // with one generic handler! |
| 128 | } |
| 129 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 130 | <p>Do not do this. In almost all cases it is inappropriate to catch generic |
| 131 | Exception or Throwable (preferably not Throwable because it includes Error |
| 132 | exceptions). It is very dangerous because it means that Exceptions |
| 133 | you never expected (including RuntimeExceptions like ClassCastException) get |
| 134 | caught in application-level error handling. It obscures the failure handling |
| 135 | properties of your code, meaning if someone adds a new type of Exception in the |
| 136 | code you're calling, the compiler won't help you realize you need to handle the |
| 137 | error differently. In most cases you shouldn't be handling different types of |
| 138 | exception the same way.</p> |
| 139 | <p>The rare exception to this rule is test code and top-level code where you |
| 140 | want to catch all kinds of errors (to prevent them from showing up in a UI, or |
| 141 | to keep a batch job running). In these cases you may catch generic Exception |
| 142 | (or Throwable) and handle the error appropriately. Think very carefully before |
| 143 | doing this, though, and put in comments explaining why it is safe in this place.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 144 | <p>Alternatives to catching generic Exception:</p> |
| 145 | <ul> |
| 146 | <li> |
| 147 | <p>Catch each exception separately as separate catch blocks after a single |
| 148 | try. This can be awkward but is still preferable to catching all Exceptions. |
| 149 | Beware repeating too much code in the catch blocks.</li></p> |
| 150 | </li> |
| 151 | <li> |
| 152 | <p>Refactor your code to have more fine-grained error handling, with multiple |
| 153 | try blocks. Split up the IO from the parsing, handle errors separately in each |
| 154 | case.</p> |
| 155 | </li> |
| 156 | <li> |
| 157 | <p>Rethrow the exception. Many times you don't need to catch the exception at |
| 158 | this level anyway, just let the method throw it.</p> |
| 159 | </li> |
| 160 | </ul> |
| 161 | <p>Remember: exceptions are your friend! When the compiler complains you're |
| 162 | not catching an exception, don't scowl. Smile: the compiler just made it |
| 163 | easier for you to catch runtime problems in your code.</p> |
| 164 | <h3 id="dont-use-finalizers">Don't Use Finalizers</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 165 | <p>Finalizers are a way to have a chunk of code executed when an object is |
| 166 | garbage collected. While they can be handy for doing cleanup (particularly of |
| 167 | external resources, there are no guarantees as to when a finalizer will be |
| 168 | called (or even that it will be called at all).</p> |
| 169 | <p>Android doesn't use finalizers. In most cases, you can do what |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 170 | you need from a finalizer with good exception handling. If you absolutely need |
| 171 | it, define a close() method (or the like) and document exactly when that |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 172 | method needs to be called (see InputStream for an example). In this case it is |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 173 | appropriate but not required to print a short log message from the finalizer, |
| 174 | as long as it is not expected to flood the logs.</p> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 175 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 176 | <h3 id="fully-qualify-imports">Fully Qualify Imports</h3> |
| 177 | <p>When you want to use class Bar from package foo,there |
| 178 | are two possible ways to import it:</p> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 179 | <ul> |
| 180 | <li><code>import foo.*;</code> |
| 181 | <p>Potentially reduces the number of import statements.</p></li> |
| 182 | <li><code>import foo.Bar;</code> |
| 183 | <p>Makes it obvious what classes are actually used and the code is more readable |
| 184 | for maintainers.</p></li></ul> |
| 185 | <p>Use <code>import foo.Bar;</code> for importing all Android code. An explicit |
| 186 | exception is made for java standard libraries (<code>java.util.*</code>, |
| 187 | <code>java.io.*</code>, etc.) and unit test code |
| 188 | (<code>junit.framework.*</code>).</p> |
| 189 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 190 | <h2 id="java-library-rules">Java Library Rules</h2> |
| 191 | <p>There are conventions for using Android's Java libraries and tools. In some |
| 192 | cases, the convention has changed in important ways and older code might use a |
| 193 | deprecated pattern or library. When working with such code, it's okay to |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 194 | continue the existing style. When creating new components however, never use |
| 195 | deprecated libraries.</p> |
Clay Murphy | b61f06c | 2014-05-20 10:15:06 -0700 | [diff] [blame] | 196 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 197 | <h2 id="java-style-rules">Java Style Rules</h2> |
Clay Murphy | b61f06c | 2014-05-20 10:15:06 -0700 | [diff] [blame] | 198 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 199 | <h3 id="use-javadoc-standard-comments">Use Javadoc Standard Comments</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 200 | <p>Every file should have a copyright statement at the top, followed by package |
| 201 | and import statements (each block separated by a blank line) and finally the |
| 202 | class or interface declaration. In the Javadoc comments, describe what the class |
| 203 | or interface does.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 204 | <pre><code>/* |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 205 | * Copyright (C) 2015 The Android Open Source Project |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 206 | * |
| 207 | * Licensed under the Apache License, Version 2.0 (the "License"); |
| 208 | * you may not use this file except in compliance with the License. |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 209 | * You may obtain a copy of the License at |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 210 | * |
| 211 | * http://www.apache.org/licenses/LICENSE-2.0 |
| 212 | * |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 213 | * Unless required by applicable law or agreed to in writing, software |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 214 | * distributed under the License is distributed on an "AS IS" BASIS, |
| 215 | * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 216 | * See the License for the specific language governing permissions and |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 217 | * limitations under the License. |
| 218 | */ |
| 219 | |
| 220 | package com.android.internal.foo; |
| 221 | |
| 222 | import android.os.Blah; |
| 223 | import android.view.Yada; |
| 224 | |
| 225 | import java.sql.ResultSet; |
| 226 | import java.sql.SQLException; |
| 227 | |
| 228 | /** |
| 229 | * Does X and Y and provides an abstraction for Z. |
| 230 | */ |
| 231 | |
| 232 | public class Foo { |
| 233 | ... |
| 234 | } |
| 235 | </code></pre> |
| 236 | <p>Every class and nontrivial public method you write <em>must</em> contain a |
| 237 | Javadoc comment with at least one sentence describing what the class or method |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 238 | does. This sentence should start with a third person descriptive verb.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 239 | <p>Examples:</p> |
| 240 | <pre><code>/** Returns the correctly rounded positive square root of a double value. */ |
| 241 | static double sqrt(double a) { |
| 242 | ... |
| 243 | } |
| 244 | </code></pre> |
| 245 | <p>or</p> |
| 246 | <pre><code>/** |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 247 | * Constructs a new String by converting the specified array of |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 248 | * bytes using the platform's default character encoding. |
| 249 | */ |
| 250 | public String(byte[] bytes) { |
| 251 | ... |
| 252 | } |
| 253 | </code></pre> |
| 254 | <p>You do not need to write Javadoc for trivial get and set methods such as |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 255 | <code>setFoo()</code> if all your Javadoc would say is "sets Foo". If the method |
| 256 | does something more complex (such as enforcing a constraint or has an important |
| 257 | side effect), then you must document it. If it's not obvious what the property |
| 258 | "Foo" means, you should document it. |
| 259 | <p>Every method you write, public or otherwise, would benefit from Javadoc. |
| 260 | Public methods are part of an API and therefore require Javadoc. Android does |
| 261 | not currently enforce a specific style for writing Javadoc comments, but you |
| 262 | should follow the instructions <a |
Clay Murphy | b61f06c | 2014-05-20 10:15:06 -0700 | [diff] [blame] | 263 | href="http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html">How |
| 264 | to Write Doc Comments for the Javadoc Tool</a>.</p> |
| 265 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 266 | <h3 id="write-short-methods">Write Short Methods</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 267 | <p>When feasible, keep methods small and focused. We recognize that long methods |
| 268 | are sometimes appropriate, so no hard limit is placed on method length. If a |
| 269 | method exceeds 40 lines or so, think about whether it can be broken up without |
| 270 | harming the structure of the program.</p> |
| 271 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 272 | <h3 id="define-fields-in-standard-places">Define Fields in Standard Places</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 273 | <p>Define fields either at the top of the file or immediately before the |
| 274 | methods that use them.</p> |
| 275 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 276 | <h3 id="limit-variable-scope">Limit Variable Scope</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 277 | <p>Keep the scope of local variables to a minimum. By doing so, you |
| 278 | increase the readability and maintainability of your code and reduce the |
| 279 | likelihood of error. Each variable should be declared in the innermost block |
| 280 | that encloses all uses of the variable.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 281 | <p>Local variables should be declared at the point they are first used. Nearly |
| 282 | every local variable declaration should contain an initializer. If you don't |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 283 | yet have enough information to initialize a variable sensibly, postpone the |
| 284 | declaration until you do.</p> |
| 285 | <p>The exception is try-catch statements. If a variable is initialized with the |
| 286 | return value of a method that throws a checked exception, it must be initialized |
| 287 | inside a try block. If the value must be used outside of the try block, then it |
| 288 | must be declared before the try block, where it cannot yet be sensibly |
| 289 | initialized:</p> |
| 290 | <pre><code>// Instantiate class cl, which represents some sort of Set |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 291 | Set s = null; |
| 292 | try { |
| 293 | s = (Set) cl.newInstance(); |
| 294 | } catch(IllegalAccessException e) { |
| 295 | throw new IllegalArgumentException(cl + " not accessible"); |
| 296 | } catch(InstantiationException e) { |
| 297 | throw new IllegalArgumentException(cl + " not instantiable"); |
| 298 | } |
| 299 | |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 300 | // Exercise the set |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 301 | s.addAll(Arrays.asList(args)); |
| 302 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 303 | <p>However, even this case can be avoided by encapsulating the try-catch block |
| 304 | in a method:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 305 | <pre><code>Set createSet(Class cl) { |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 306 | // Instantiate class cl, which represents some sort of Set |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 307 | try { |
| 308 | return (Set) cl.newInstance(); |
| 309 | } catch(IllegalAccessException e) { |
| 310 | throw new IllegalArgumentException(cl + " not accessible"); |
| 311 | } catch(InstantiationException e) { |
| 312 | throw new IllegalArgumentException(cl + " not instantiable"); |
| 313 | } |
| 314 | } |
| 315 | |
| 316 | ... |
| 317 | |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 318 | // Exercise the set |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 319 | Set s = createSet(cl); |
| 320 | s.addAll(Arrays.asList(args)); |
| 321 | </code></pre> |
| 322 | <p>Loop variables should be declared in the for statement itself unless there |
| 323 | is a compelling reason to do otherwise:</p> |
Clay Murphy | 07c3d64 | 2013-05-30 18:14:34 -0700 | [diff] [blame] | 324 | <pre><code>for (int i = 0; i < n; i++) { |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 325 | doSomething(i); |
| 326 | } |
| 327 | </code></pre> |
| 328 | <p>and</p> |
| 329 | <pre><code>for (Iterator i = c.iterator(); i.hasNext(); ) { |
| 330 | doSomethingElse(i.next()); |
| 331 | } |
| 332 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 333 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 334 | <h3 id="order-import-statements">Order Import Statements</h3> |
| 335 | <p>The ordering of import statements is:</p> |
| 336 | <ol> |
| 337 | <li> |
| 338 | <p>Android imports</p> |
| 339 | </li> |
| 340 | <li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 341 | <p>Imports from third parties (<code>com</code>, <code>junit</code>, |
| 342 | <code>net</code>, <code>org</code>)</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 343 | </li> |
| 344 | <li> |
| 345 | <p><code>java</code> and <code>javax</code></p> |
| 346 | </li> |
| 347 | </ol> |
| 348 | <p>To exactly match the IDE settings, the imports should be:</p> |
| 349 | <ul> |
| 350 | <li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 351 | <p>Alphabetical within each grouping, with capital letters before lower case |
| 352 | letters (e.g. Z before a).</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 353 | </li> |
| 354 | <li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 355 | <p>Separated by a blank line between each major grouping (<code>android</code>, |
| 356 | <code>com</code>, <code>junit</code>, <code>net</code>, <code>org</code>, |
| 357 | <code>java</code>, <code>javax</code>).</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 358 | </li> |
| 359 | </ul> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 360 | <p>Originally, there was no style requirement on the ordering, meaning IDEs were |
| 361 | either always changing the ordering or IDE developers had to disable the |
| 362 | automatic import management features and manually maintain the imports. This was |
| 363 | deemed bad. When java-style was asked, the preferred styles varied wildly and it |
| 364 | came down to Android needing to simply "pick an ordering and be consistent." So |
| 365 | we chose a style, updated the style guide, and made the IDEs obey it. We expect |
| 366 | that as IDE users work on the code, imports in all packages will match this |
| 367 | pattern without extra engineering effort.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 368 | <p>This style was chosen such that:</p> |
| 369 | <ul> |
| 370 | <li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 371 | <p>The imports people want to look at first tend to be at the top |
| 372 | (<code>android</code>).</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 373 | </li> |
| 374 | <li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 375 | <p>The imports people want to look at least tend to be at the bottom |
| 376 | (<code>java</code>).</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 377 | </li> |
| 378 | <li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 379 | <p>Humans can easily follow the style.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 380 | </li> |
| 381 | <li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 382 | <p>IDEs can follow the style.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 383 | </li> |
| 384 | </ul> |
| 385 | <p>The use and location of static imports have been mildly controversial |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 386 | issues. Some people prefer static imports to be interspersed with the |
| 387 | remaining imports, while some prefer them to reside above or below all |
| 388 | other imports. Additionally, we have not yet determined how to make all IDEs use |
| 389 | the same ordering. Since many consider this a low priority issue, just use your |
| 390 | judgement and be consistent.</p> |
| 391 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 392 | <h3 id="use-spaces-for-indentation">Use Spaces for Indentation</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 393 | <p>We use four (4) space indents for blocks and never tabs. When in doubt, be |
| 394 | consistent with the surrounding code.</p> |
| 395 | <p>We use eight (8) space indents for line wraps, including function calls and |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 396 | assignments. For example, this is correct:</p> |
| 397 | <pre><code>Instrument i = |
| 398 | someLongExpression(that, wouldNotFit, on, one, line); |
| 399 | </code></pre> |
| 400 | <p>and this is not correct:</p> |
| 401 | <pre><code>Instrument i = |
| 402 | someLongExpression(that, wouldNotFit, on, one, line); |
| 403 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 404 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 405 | <h3 id="follow-field-naming-conventions">Follow Field Naming Conventions</h3> |
| 406 | <ul> |
| 407 | <li> |
| 408 | <p>Non-public, non-static field names start with m.</p> |
| 409 | </li> |
| 410 | <li> |
| 411 | <p>Static field names start with s.</p> |
| 412 | </li> |
| 413 | <li> |
| 414 | <p>Other fields start with a lower case letter.</p> |
| 415 | </li> |
| 416 | <li> |
| 417 | <p>Public static final fields (constants) are ALL_CAPS_WITH_UNDERSCORES.</p> |
| 418 | </li> |
| 419 | </ul> |
| 420 | <p>For example:</p> |
| 421 | <pre><code>public class MyClass { |
| 422 | public static final int SOME_CONSTANT = 42; |
| 423 | public int publicField; |
| 424 | private static MyClass sSingleton; |
| 425 | int mPackagePrivate; |
| 426 | private int mPrivate; |
| 427 | protected int mProtected; |
| 428 | } |
| 429 | </code></pre> |
| 430 | <h3 id="use-standard-brace-style">Use Standard Brace Style</h3> |
| 431 | <p>Braces do not go on their own line; they go on the same line as the code |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 432 | before them:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 433 | <pre><code>class MyClass { |
| 434 | int func() { |
| 435 | if (something) { |
| 436 | // ... |
| 437 | } else if (somethingElse) { |
| 438 | // ... |
| 439 | } else { |
| 440 | // ... |
| 441 | } |
| 442 | } |
| 443 | } |
| 444 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 445 | <p>We require braces around the statements for a conditional. Exception: If the |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 446 | entire conditional (the condition and the body) fit on one line, you may (but |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 447 | are not obligated to) put it all on one line. For example, this is acceptable:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 448 | <pre><code>if (condition) { |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 449 | body(); |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 450 | } |
| 451 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 452 | <p>and this is acceptable:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 453 | <pre><code>if (condition) body(); |
| 454 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 455 | <p>but this is not acceptable:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 456 | <pre><code>if (condition) |
| 457 | body(); // bad! |
| 458 | </code></pre> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 459 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 460 | <h3 id="limit-line-length">Limit Line Length</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 461 | <p>Each line of text in your code should be at most 100 characters long. While |
| 462 | much discussion has surrounded this rule, the decision remains that 100 |
| 463 | characters is the maximum <em>with the following exceptions</em>:</p> |
| 464 | <ul> |
| 465 | <li>If a comment line contains an example command or a literal URL |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 466 | longer than 100 characters, that line may be longer than 100 characters for |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 467 | ease of cut and paste.</li> |
| 468 | <li>Import lines can go over the limit because humans rarely see them (this also |
| 469 | simplifies tool writing).</li> |
| 470 | </ul> |
| 471 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 472 | <h3 id="use-standard-java-annotations">Use Standard Java Annotations</h3> |
| 473 | <p>Annotations should precede other modifiers for the same language element. |
| 474 | Simple marker annotations (e.g. @Override) can be listed on the same line with |
| 475 | the language element. If there are multiple annotations, or parameterized |
| 476 | annotations, they should each be listed one-per-line in alphabetical |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 477 | order.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 478 | <p>Android standard practices for the three predefined annotations in Java are:</p> |
| 479 | <ul> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 480 | <li><code>@Deprecated</code>: The @Deprecated annotation must be used whenever |
| 481 | the use of the annotated element is discouraged. If you use the @Deprecated |
| 482 | annotation, you must also have a @deprecated Javadoc tag and it should name an |
| 483 | alternate implementation. In addition, remember that a @Deprecated method is |
| 484 | <em>still supposed to work</em>. If you see old code that has a @deprecated |
| 485 | Javadoc tag, please add the @Deprecated annotation. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 486 | </li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 487 | <li><code>@Override</code>: The @Override annotation must be used whenever a |
| 488 | method overrides the declaration or implementation from a super-class. For |
| 489 | example, if you use the @inheritdocs Javadoc tag, and derive from a class (not |
| 490 | an interface), you must also annotate that the method @Overrides the parent |
| 491 | class's method.</li> |
| 492 | <li><code>@SuppressWarnings</code>: The @SuppressWarnings annotation should be |
| 493 | used only under circumstances where it is impossible to eliminate a warning. If |
| 494 | a warning passes this "impossible to eliminate" test, the @SuppressWarnings |
| 495 | annotation <em>must</em> be used, so as to ensure that all warnings reflect |
| 496 | actual problems in the code. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 497 | <p>When a @SuppressWarnings annotation is necessary, it must be prefixed with |
| 498 | a TODO comment that explains the "impossible to eliminate" condition. This |
| 499 | will normally identify an offending class that has an awkward interface. For |
| 500 | example:</p> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 501 | <pre><code>// TODO: The third-party class com.third.useful.Utility.rotate() needs generics |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 502 | @SuppressWarnings("generic-cast") |
| 503 | List<String> blix = Utility.rotate(blax); |
| 504 | </code></pre> |
| 505 | <p>When a @SuppressWarnings annotation is required, the code should be |
| 506 | refactored to isolate the software elements where the annotation applies.</p> |
| 507 | </li> |
| 508 | </ul> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 509 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 510 | <h3 id="treat-acronyms-as-words">Treat Acronyms as Words</h3> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 511 | <p>Treat acronyms and abbreviations as words in naming variables, methods, and |
| 512 | classes to make names more readable:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 513 | <table> |
| 514 | <thead> |
| 515 | <tr> |
| 516 | <th>Good</th> |
| 517 | <th>Bad</th> |
| 518 | </tr> |
| 519 | </thead> |
| 520 | <tbody> |
| 521 | <tr> |
| 522 | <td>XmlHttpRequest</td> |
| 523 | <td>XMLHTTPRequest</td> |
| 524 | </tr> |
| 525 | <tr> |
| 526 | <td>getCustomerId</td> |
| 527 | <td>getCustomerID</td> |
| 528 | </tr> |
| 529 | <tr> |
| 530 | <td>class Html</td> |
| 531 | <td>class HTML</td> |
| 532 | </tr> |
| 533 | <tr> |
| 534 | <td>String url</td> |
| 535 | <td>String URL</td> |
| 536 | </tr> |
| 537 | <tr> |
| 538 | <td>long id</td> |
| 539 | <td>long ID</td> |
| 540 | </tr> |
| 541 | </tbody> |
| 542 | </table> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 543 | <p>As both the JDK and the Android code bases are very inconsistent around |
| 544 | acronyms, it is virtually impossible to be consistent with the surrounding |
| 545 | code. Therefore, always treat acronyms as words.</p> |
Clay Murphy | 74a4dcc | 2014-09-09 16:26:00 -0700 | [diff] [blame] | 546 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 547 | <h3 id="use-todo-comments">Use TODO Comments</h3> |
| 548 | <p>Use TODO comments for code that is temporary, a short-term solution, or |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 549 | good-enough but not perfect. TODOs should include the string TODO in all caps, |
| 550 | followed by a colon:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 551 | <pre><code>// TODO: Remove this code after the UrlTable2 has been checked in. |
| 552 | </code></pre> |
| 553 | <p>and</p> |
| 554 | <pre><code>// TODO: Change this to use a flag instead of a constant. |
| 555 | </code></pre> |
| 556 | <p>If your TODO is of the form "At a future date do something" make sure that |
| 557 | you either include a very specific date ("Fix by November 2005") or a very |
| 558 | specific event ("Remove this code after all production mixers understand |
| 559 | protocol V7.").</p> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 560 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 561 | <h3 id="log-sparingly">Log Sparingly</h3> |
Clay Murphy | 74a4dcc | 2014-09-09 16:26:00 -0700 | [diff] [blame] | 562 | <p>While logging is necessary, it has a significantly negative impact on |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 563 | performance and quickly loses its usefulness if not kept reasonably |
Clay Murphy | 74a4dcc | 2014-09-09 16:26:00 -0700 | [diff] [blame] | 564 | terse. The logging facilities provides five different levels of logging:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 565 | <ul> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 566 | <li><code>ERROR</code>: |
| 567 | Use when something fatal has happened, i.e. something will have user-visible |
| 568 | consequences and won't be recoverable without explicitly deleting some data, |
| 569 | uninstalling applications, wiping the data partitions or reflashing the entire |
| 570 | device (or worse). This level is always logged. Issues that justify some logging |
| 571 | at the ERROR level are typically good candidates to be reported to a |
| 572 | statistics-gathering server.</li> |
| 573 | <li><code>WARNING</code>: |
| 574 | Use when something serious and unexpected happened, i.e. something that will |
| 575 | have user-visible consequences but is likely to be recoverable without data loss |
| 576 | by performing some explicit action, ranging from waiting or restarting an app |
| 577 | all the way to re-downloading a new version of an application or rebooting the |
| 578 | device. This level is always logged. Issues that justify some logging at the |
| 579 | WARNING level might also be considered for reporting to a statistics-gathering |
| 580 | server.</li> |
| 581 | <li><code>INFORMATIVE:</code> |
| 582 | Use note that something interesting to most people happened, i.e. when a |
| 583 | situation is detected that is likely to have widespread impact, though isn't |
| 584 | necessarily an error. Such a condition should only be logged by a module that |
| 585 | reasonably believes that it is the most authoritative in that domain (to avoid |
| 586 | duplicate logging by non-authoritative components). This level is always logged. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 587 | </li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 588 | <li><code>DEBUG</code>: |
| 589 | Use to further note what is happening on the device that could be relevant to |
| 590 | investigate and debug unexpected behaviors. You should log only what is needed |
| 591 | to gather enough information about what is going on about your component. If |
| 592 | your debug logs are dominating the log then you probably should be using verbose |
| 593 | logging. |
| 594 | <p>This level will be logged, even on release builds, and is required to be |
| 595 | surrounded by an <code>if (LOCAL_LOG)</code> or <code>if (LOCAL_LOGD)</code> |
| 596 | block, where <code>LOCAL_LOG[D]</code> is defined in your class or subcomponent, |
| 597 | so that there can exist a possibility to disable all such logging. There must |
| 598 | therefore be no active logic in an <code>if (LOCAL_LOG)</code> block. All the |
| 599 | string building for the log also needs to be placed inside the <code>if |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 600 | (LOCAL_LOG)</code> block. The logging call should not be re-factored out into a |
| 601 | method call if it is going to cause the string building to take place outside |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 602 | of the <code>if (LOCAL_LOG)</code> block.</p> |
| 603 | <p>There is some code that still says <code>if (localLOGV)</code>. This is |
| 604 | considered acceptable as well, although the name is nonstandard.</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 605 | </li> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 606 | <li><code>VERBOSE</code>: |
| 607 | Use for everything else. This level will only be logged on debug builds and |
| 608 | should be surrounded by an <code>if (LOCAL_LOGV)</code> block (or equivalent) so |
| 609 | it can be compiled out by default. Any string building will be stripped out of |
| 610 | release builds and needs to appear inside the <code>if (LOCAL_LOGV)</code> block. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 611 | </li> |
| 612 | </ul> |
| 613 | <p><em>Notes:</em> </p> |
| 614 | <ul> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 615 | <li>Within a given module, other than at the VERBOSE level, an |
| 616 | error should only be reported once if possible. Within a single chain of |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 617 | function calls within a module, only the innermost function should return the |
| 618 | error, and callers in the same module should only add some logging if that |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 619 | significantly helps to isolate the issue.</li> |
| 620 | <li>In a chain of modules, other than at the VERBOSE level, when a |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 621 | lower-level module detects invalid data coming from a higher-level module, the |
| 622 | lower-level module should only log this situation to the DEBUG log, and only |
| 623 | if logging provides information that is not otherwise available to the caller. |
| 624 | Specifically, there is no need to log situations where an exception is thrown |
| 625 | (the exception should contain all the relevant information), or where the only |
| 626 | information being logged is contained in an error code. This is especially |
| 627 | important in the interaction between the framework and applications, and |
| 628 | conditions caused by third-party applications that are properly handled by the |
| 629 | framework should not trigger logging higher than the DEBUG level. The only |
| 630 | situations that should trigger logging at the INFORMATIVE level or higher is |
| 631 | when a module or application detects an error at its own level or coming from |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 632 | a lower level.</li> |
| 633 | <li>When a condition that would normally justify some logging is |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 634 | likely to occur many times, it can be a good idea to implement some |
| 635 | rate-limiting mechanism to prevent overflowing the logs with many duplicate |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 636 | copies of the same (or very similar) information.</li> |
| 637 | <li>Losses of network connectivity are considered common, fully expected, and |
| 638 | should not be logged gratuitously. A loss of network connectivity |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 639 | that has consequences within an app should be logged at the DEBUG or VERBOSE |
| 640 | level (depending on whether the consequences are serious enough and unexpected |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 641 | enough to be logged in a release build).</li> |
| 642 | <li>A full filesystem on a filesystem that is accessible to or on |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 643 | behalf of third-party applications should not be logged at a level higher than |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 644 | INFORMATIVE.</li> |
| 645 | <li>Invalid data coming from any untrusted source (including any |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 646 | file on shared storage, or data coming through just about any network |
| 647 | connections) is considered expected and should not trigger any logging at a |
| 648 | level higher then DEBUG when it's detected to be invalid (and even then |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 649 | logging should be as limited as possible).</li> |
| 650 | <li>Keep in mind that the <code>+</code> operator, when used on Strings, |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 651 | implicitly creates a <code>StringBuilder</code> with the default buffer size (16 |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 652 | characters) and potentially other temporary String objects, i.e. |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 653 | that explicitly creating StringBuilders isn't more expensive than relying on |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 654 | the default '+' operator (and can be a lot more efficient in fact). Keep |
| 655 | in mind that code that calls <code>Log.v()</code> is compiled and executed on |
| 656 | release builds, including building the strings, even if the logs aren't being |
| 657 | read.</li> |
| 658 | <li>Any logging that is meant to be read by other people and to be |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 659 | available in release builds should be terse without being cryptic, and should |
| 660 | be reasonably understandable. This includes all logging up to the DEBUG |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 661 | level.</li> |
| 662 | <li>When possible, logging should be kept on a single line if it |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 663 | makes sense. Line lengths up to 80 or 100 characters are perfectly acceptable, |
| 664 | while lengths longer than about 130 or 160 characters (including the length of |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 665 | the tag) should be avoided if possible.</li> |
| 666 | <li>Logging that reports successes should never be used at levels |
| 667 | higher than VERBOSE.</li> |
| 668 | <li>Temporary logging used to diagnose an issue that is hard to reproduce should |
| 669 | be kept at the DEBUG or VERBOSE level and should be enclosed by if blocks that |
| 670 | allow for disabling it entirely at compile time.</li> |
| 671 | <li>Be careful about security leaks through the log. Private |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 672 | information should be avoided. Information about protected content must |
| 673 | definitely be avoided. This is especially important when writing framework |
| 674 | code as it's not easy to know in advance what will and will not be private |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 675 | information or protected content.</li> |
| 676 | <li><code>System.out.println()</code> (or <code>printf()</code> for native code) |
| 677 | should never be used. System.out and System.err get redirected to /dev/null, so |
| 678 | your print statements will have no visible effects. However, all the string |
| 679 | building that happens for these calls still gets executed.</li> |
| 680 | <li><em>The golden rule of logging is that your logs may not |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 681 | unnecessarily push other logs out of the buffer, just as others may not push |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 682 | out yours.</em></li> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 683 | </ul> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 684 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 685 | <h3 id="be-consistent">Be Consistent</h3> |
| 686 | <p>Our parting thought: BE CONSISTENT. If you're editing code, take a few |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 687 | minutes to look at the surrounding code and determine its style. If that code |
| 688 | uses spaces around the if clauses, you should too. If the code comments have |
| 689 | little boxes of stars around them, make your comments have little boxes of stars |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 690 | around them too.</p> |
| 691 | <p>The point of having style guidelines is to have a common vocabulary of |
| 692 | coding, so people can concentrate on what you're saying, rather than on how |
| 693 | you're saying it. We present global style rules here so people know the |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 694 | vocabulary, but local style is also important. If the code you add to a file |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 695 | looks drastically different from the existing code around it, it throws |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 696 | readers out of their rhythm when they go to read it. Try to avoid this.</p> |
| 697 | |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 698 | <h2 id="javatests-style-rules">Javatests Style Rules</h2> |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 699 | <p>Follow test method naming conventions and use an underscore to separate what |
| 700 | is being tested from the specific case being tested. This style makes it easier |
| 701 | to see exactly what cases are being tested. For example:</p> |
Robert Ly | 35f2fda | 2013-01-29 16:27:05 -0800 | [diff] [blame] | 702 | <pre><code>testMethod_specificCase1 testMethod_specificCase2 |
| 703 | |
| 704 | void testIsDistinguishable_protanopia() { |
| 705 | ColorMatcher colorMatcher = new ColorMatcher(PROTANOPIA) |
| 706 | assertFalse(colorMatcher.isDistinguishable(Color.RED, Color.BLACK)) |
| 707 | assertTrue(colorMatcher.isDistinguishable(Color.X, Color.Y)) |
| 708 | } |
Heidi von Markham | d206f4d | 2015-12-08 13:44:34 -0800 | [diff] [blame^] | 709 | </code></pre> |