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