Android "Search" Activity and corresponding XML

Below is an Android Activity and corresponding XML layout. The function of the class is to search an SQLite database to show results to the user.

Can I get a review of the code quality? This is my first Android app so feedback would be greatly appreciated as I do not know standard Android/Java practices to aim for.

Activity Code:

`package com.example.briapp;

import java.util.List;
import android.support.v7.app.ActionBarActivity;
import android.text.method.ScrollingMovementMethod;
import android.content.Context;
import android.os.Bundle;
import android.view.View;
import android.view.inputmethod.InputMethodManager;
import android.widget.Button;
import android.widget.EditText;
import android.widget.TextView;
import android.widget.Toast;

/**
* Activity that contains functionality for searching by average attention value
*
* @author Ross
*
*/
public class AttentionAverageSearch extends ActionBarActivity implements
View.OnClickListener {

// declare variables
TextView instruction;
EditText avgEntered;
TextView tvResults;
DatabaseHelper db = new DatabaseHelper(this);
String log;
Button sub;
List results;

@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.attentionaveragesearch);

initialiseVars();
}

/**
* Method that initialises variables for the activty and sets onclick
* listener for button
*/
public void initialiseVars() {

instruction = (TextView) findViewById(R.id.tvAverageAttSearch1);
avgEntered = (EditText) findViewById(R.id.etEnterSpecificAttAvgVal);
tvResults = (TextView) findViewById(R.id.tvAvgAttSearchResults1);
tvResults.setMovementMethod(new ScrollingMovementMethod());
sub = (Button) findViewById(R.id.btnAvgAttSearch);
sub.setOnClickListener(this);

}

/**
* On click listener for button(s)
*/
public voi

Initializing should be done automatically from your constructor. Right now you're exposing it as a public method that may or may not get called: this will guaranteed cause problems when someone forgets to initialize. Considering it doesn't add value in the first place I would just call it automatically in your constructor.

Use comments to convey why you're doing something, not what. I can tell from the code that you do someVariable + 1 so I don't need a comment for that. What I do need is a comment to tell me //Add offset to make up for .

Applying this to your code I would say that comments like this are not needed:

/**
 * Method that initialises variables for the activty and sets onclick
 * listener for button
 */

because a well-named method will already give me that information: when I read

public void initializeFields()

I already know everything that the comment told me. Likewise,

/**
 * On click listener for button(s)
 */

will not give me any more information than

public void onClick(View v)

already tells me. Yes, you specify that it's about buttons but that's very minor.

This, however, is a good comment because it is less obvious from the code:

// hides keyboard when btn pressed

You have too much whitespace in your .java file for my taste; don't leave these empty lines after switch/case/if/methods/etc. It just makes it harder to digest because the information is spread out too much.

I'm not a fan of intermediate variables that have a short declaration and are only used once. I'd change

final String entry = avgEntered.getText().toString();
int avgValToSearch = Integer.parseInt(entry);

to

int avgValToSearch = Integer.parseInt(avgEntered.getText().toString());

No abbreviations[1]. Change that to averageValueToSearch and initializeVariables.

[1]: almost none. Some very specific ones like db I can live with.

listToString() is a helper method. If it is only used in AttentionAverageSearch then it should be declared there and made private.

If it is used elsewhere as well, then it should be either moved up into the hierarchy (if that makes sense in your structure) or to a separate helper class.

Use a StringBuilder when concatenating strings in a loop.

I don't like using sub.setOnClickListener(this); and then implementing View.OnClickListener on activity level. You end up, as you do, with one big click handler while you might as well just create a method dedicated to that button. It will be clearer and easier to find as well (when you have 20 buttons, you would have them all laid out in your IDE's class view window).

Restrict visibility unless you actually want an outside class to access it. Both initializeVars and click are not things the outside world should access so make them private.

StackExchange Code Review Q#61255, answer score: 10