Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm an upstart, been only professionally programming for a bit less than 2 years, mainly in Java as I am currently working as an Android developer.

I often find myself thinking about how to improve the readability and maintainability of my code, and establish some style guidelines, for the project that I am working on, but also in general. On this project, I am using git and expect to have a few more developers joining me in the near future, and would like to keep our code style as readable/consistent/easily maintainable as possible. I have the (rare, I'm told) opportunity to establish some guidelines and create some configuration files for the code formatting which would then be used team-wide and keep things consistent.

I guess I want to:

  1. keep the lines relatively short, preferably less than 80 characters (e.g. coding on an ultrabook with a 14 inch display with excessively long lines is not fun)
  2. avoid refactoring variable/method names resulting in misalignment of method parameters
  3. keep chained method calls readable
  4. keep complex logical conditions readable
  5. keep the list of implemented interfaces readable
  6. Any other suggestions?

I have some ideas on how to accomplish this, but would like to hear if there's something that I can improve. Here's an example of a method which features points 2, 3, 4:

@Override
public void onScroll(
    AbsListView view,
    int firstVisibleItem,
    int visibleItemCount,
    int totalItemCount
) {
    super.onScroll(
        view,
        firstVisibleItem,
        visibleItemCount,
        totalItemCount
    );
    if(mUpdateFixedHeader &&
       !(
           mAdapter.isFiltered() &&
           mAdapter.isEmpty()
       ) &&
       mDateHeader != null
    ) {
       mDateHeaderTextView.setText(
           mDateHeaderLabel.concat(
              ((MeetsListAdapter) mAdapter)
                  .getSectionForPosition(firstVisibleItem)
           )
       );
    }
}

I'm not too sure about the way I've chained the logical conditions here, and I wanted to have the opening curly braces in a new lines but felt it makes the whole thing very disjointed. I would do it for the opening braces of a class though.

And, for a class declaration example, for a class that implements several interfaces, I'm thinking something along the lines of this (point 5):

public abstract class AbstractListFragment<T> 
    extends 
        AbstractFragment 
    implements 
        OnItemClickListener,
        OnScrollListener,
        SearchListener
{
    ...
}

Looking forward to your thoughts/suggestions!

share|improve this question
add comment

2 Answers 2

up vote 5 down vote accepted

Code style guidelines are available from a number of places. The 'old faithful' official Java ones have been pulled down, for some reason. Oh, they have been restored and are available in an archive format.... There are two other references that are commonly used for Java code style:

This StackOverflow question seems particularly relevant: Official Java Code Guidelines/Conventions.

What all of these guides have in common is, and is contrary to your code, is:

  • don't put each parameter to the method on its own line. The limiting factor is the number of characters, not the number of parameters, on the line. This is true for both when you declare the method, and call the method.
  • never start a line with a close-parenthesis ). The close-parenthesis goes at the end of the previous line.
  • conditions in an if-block should be grouped where convenient, and line-breaks, if any, should be at logical breaks in the conditions. On the other hand, you are too generous with line-breaks in your conditions... !( on it's own line is bizarre.
  • when splitting long lines (string constants, long conditionals), put the 'operator' at the start of the next line, not the end of the previous one. For example, you have:

     mAdapter.isFiltered() &&
     mAdapter.isEmpty()    
    

    and that should be:

      mAdapter.isFiltered()
      && mAdapter.isEmpty()    
    

Really, there are only a few areas of real controversy in Java code style guidelines:

  • indentation using tabs, or spaces: all code-style guidelines I have read for Java indicate that spaces should be used. For example, the Official (old) Java spec: "Four spaces should be used as the unit of indentation", and the Google spec "Each time a new block or block-like construct is opened, the indent increases by two spaces."

    For some reason (and I am not sure why), a number of IDE's ship using tabs as the default indentation mechanism (I'm looking at you, Eclipse).

    The amount of indentation is often site-specific, whether 2 or 4 spaces. Personally I am happy with either (as long as it is consistent), but I prefer 4.

    Tabs vs. Spaces can become a battle of cultures, and it is seldom resolved without someone feeling upset.... but, when working as a team, using a version-control-system, you have to decide on one system, and then you have to be consistent with all developers. Messing with indentation after the fact is a very messy process in a version-control system.

  • End-of-line open-brace {, or new-line brace: this is one of those style things that is hotly debated, but Java conventions are clear. Brace at the end of the line. The thing is, that many other languages recommend a brace on the next line, especially 'punctuated' C languages like C, C++, C#. Because people often write in multiple languages and they like their habits to be the same in both, they often code Java with a brace on the next line. For Java, this is wrong though, even if it is inconvenient for those folk. Java has lines like:

    if (condition) {
        .... do something....
    }
    

    the C* languages put the brace on the new line:

    if (condition)
    {
        .... do something....
    }
    

    Let's be clear on this one. A lot of people use newline-brace, but, in Java, that is not right, even if it is somewhat common.

    Further, there are arguments for, and against the brace style, but it's a pointless argument. As much as there may be meaningful arguments about one, or the other, the consistency of usage is more important than any one argument.

  • indentation of switch-blocks: for some reason, there is a lot of inconsistency in switch-block indentation. I prefer the case/default blocks to be indented 4 from the switch, and the code for the case indented 4 from the case (Google style agrees with this indentation of the case, but uses 2 spaces). For example:

    switch (somefowl) {
        case DUCK:
            System.out.println("Duck!");
            break;
        case GOOSE:
            System.out.println("Goose!");
            break;
        default:
            System.out.println("What?");
    }
    

    Many people, and some IDE defaults, do not indent the case/default (and the original Java guidelines use this style), and the code looks like:

    switch (somefowl) {
    case DUCK:
        System.out.println("Duck!");
        break;
    case GOOSE:
        System.out.println("Goose!");
        break;
    default:
        System.out.println("What?");
    }
    

    It's not a big deal. But, again, be consistent.

  • Braced 1-liners: One additional controversy is braces around 1-liners. You often see code like:

    if (badDog)
        return "Sit! Stay!";
    

    all code style guidelines recommend using braces even on 1-liners, and the code should look like:

    if (badDog) {
          return "Sit! Stay!";
    }
    

    Google has this to say:

    Braces are used with if, else, for, do and while statements, even when the body is empty or contains only a single statement

    The Java style guide says this:

    Note: if statements always use braces {}. Avoid the following error-prone form:

    if (condition) //AVOID! THIS OMITS THE BRACES {}!
        statement;
    

In general I use Eclipse. The best thing for consistency in a team is for the whole team to use the same IDE for their code, and to install the same code-style 'rules'. Then, it becomes 'easy', you just type (in eclipse): Ctrl-A and Ctrl-Shift-F. Doing that, with your code, in my Eclipse, produces:

@Override
public void onScroll(AbsListView view, int firstVisibleItem,
        int visibleItemCount, int totalItemCount) {
    super.onScroll(view, firstVisibleItem, visibleItemCount, totalItemCount);
    if (mUpdateFixedHeader
            && !(mAdapter.isFiltered() && mAdapter.isEmpty())
            && mDateHeader != null) {
        mDateHeaderTextView.setText(mDateHeaderLabel
                .concat(((MeetsListAdapter) mAdapter)
                        .getSectionForPosition(firstVisibleItem)));
    }
}

That looks about right, to me. No change needed.

When I auto-format your class, I get:

public abstract class AbstractListFragment<T> extends AbstractFragment
        implements OnItemClickListener, OnScrollListener, SearchListener {
    ...
}

Which also looks right to me.

Finally, for the above code formatting, I 'turned off' the Eclipse option to 'don't join lines'. Eclipse will 'collapse' lines when auto formatting. I often add additional empty lines in my code to make the code look like paragraphs. I want the auto-formatter to keep these empty lines, so I tell it not to format them out.

If I were to write this code, I would add some 'paragraphs' in to the code manually, and it would look like:

@Override
public void onScroll(AbsListView view, int firstVisibleItem,
        int visibleItemCount, int totalItemCount) {

    super.onScroll(view, firstVisibleItem, visibleItemCount, totalItemCount);

    if (mUpdateFixedHeader
            && !(mAdapter.isFiltered() && mAdapter.isEmpty())
            && mDateHeader != null) {

        mDateHeaderTextView.setText(mDateHeaderLabel
                .concat(((MeetsListAdapter) mAdapter)
                        .getSectionForPosition(firstVisibleItem)));

    }
}
share|improve this answer
    
Thank you very much for this detailed and well referenced answer, I really appreciate it! I will take the time to read through the answer and the resources you linked to carefully, then I'll come back and add a short conclusion to my question. –  Matej Jul 3 at 15:35
    
You're welcome, I added two additional sections to my answer (operator on next line, and 1-liner blocks). –  rolfl Jul 3 at 15:46
    
Fantastic! This will give me something to read for the next couple of days and hopefully I'll finally have this aspect of (Java) programming sorted out for the foreseeable future. –  Matej Jul 3 at 15:49
add comment

Condition

I will add a small note to the awesome answer of @rolfl. I always like to put the most significant condition when I use short-circuit operator or the less heavy calculation first (depending on what the condition is).

  mAdapter.isFiltered()
  && mAdapter.isEmpty()

In this case, is it relevant to test for isFiltered() if mAdapter is empty ? If the answer is no then I would write :

  mAdapter.isEmpty()
  && mAdapter.isFiltered()

If mAdapter is empty I will not check if it's filtered. It will rarely change the performance of the code (except in few cases), but it will the meaning of the code.

share|improve this answer
1  
You are absolutely right, the original order of checks doesn't make sense. If the adapter is empty, it doesn't matter if it's filtered or not. –  Matej Jul 3 at 16:44
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.