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.

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<Session> 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 void onClick(View v) {

        switch (v.getId()) {

        case R.id.btnAvgAttSearch:

            // validation to ensure a blank entry isn't given
            if (avgEntered.getText().toString().length() < 1) {

                Toast.makeText(getApplicationContext(),
                        "Please enter something !", Toast.LENGTH_LONG).show();
            }

            else {

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

                // /setting results equal to return value of search
                results = db.getSpecificAverageAtt(avgValToSearch);

                // hides keyboard when btn pressed
                InputMethodManager inputManager = (InputMethodManager) getSystemService(Context.INPUT_METHOD_SERVICE);
                inputManager.hideSoftInputFromWindow(getCurrentFocus()
                        .getWindowToken(), InputMethodManager.HIDE_NOT_ALWAYS);

                tvResults.setText(listToString(results));

            }

            break;
        }

    }

    /**
     * Method used to format the output of a List
     * 
     * @param list
     * @return String
     */
    public static String listToString(List<?> list) {
        String result = "";
        for (int i = 0; i < list.size(); i++) {
            result += "" + list.get(i) + "\n\n";
        }
        return result;
    }

}

Corresponding XML:

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:background="@drawable/menubackground"
    android:orientation="vertical" >

    <TextView
        android:id="@+id/tvAverageAttSearch1"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:layout_marginBottom="12dp"
        android:layout_marginTop="7dp"
        android:text="Enter Minimum Average value..."
        android:textSize="25dp" />

    <EditText
        android:id="@+id/etEnterSpecificAttAvgVal"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content"
        android:ems="10"
        android:hint="Enter value..."
        android:inputType="number" >
    </EditText>

    <Button
        android:id="@+id/btnAvgAttSearch"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:layout_gravity="center"
        android:layout_marginTop="6dp"
        android:background="@drawable/circle2"
        android:shadowColor="#A8A8A8"
        android:shadowDx="0"
        android:shadowDy="0"
        android:shadowRadius="10"
        android:text="Submit"
        android:textSize="20sp" >
    </Button>

    <TextView
        android:id="@+id/tvAvgAttSearchResults1"
        android:layout_width="fill_parent"
        android:layout_height="fill_parent"
        android:ellipsize="marquee"
        android:focusable="true"
        android:focusableInTouchMode="true"
        android:marqueeRepeatLimit="marquee_forever"
        android:maxLines="30"
        android:minLines="5"
        android:scrollbars="vertical"
        android:text=" Results..."
        android:textSize="17dp" />

</LinearLayout>
share|improve this question
    
Welcome to Code Review, Since you have added the sqlite tag you might want to include "DatabaseHelper" as well. –  JaDogg Aug 27 at 20:14
    
The database helper class is very large (over 1000 lines), that's why it isn't included? I didn't want to ask for a review of a class that large! –  user3968848 Aug 27 at 20:20
    
youre feedback on my code quality would be greatly appreciated! –  user3968848 Aug 27 at 20:30
    
You know there is a code smell if a class is 1000+ lines –  JaDogg Aug 28 at 3:54

1 Answer 1

up vote 10 down vote accepted

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 <insert-crazy-edgecase>.

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.

share|improve this answer
    
Thank you that is great feedback! Could you possibly give me a bit more detail/example on the commenting? –  user3968848 Aug 27 at 20:49
    
@user3968848: I elaborated on it, does that answer any doubts left? –  Jeroen Vannevel Aug 27 at 20:56
    
It does surely,generally would you say that the code is of a decent standard? thanks –  user3968848 Aug 27 at 20:59
    
Also, should I initialise all my vars in the constructor? –  user3968848 Aug 27 at 21:00
    
Note: I have a javadoc comment on every method as I was told, in college, it was good practice? Perhaps this is not true –  user3968848 Aug 27 at 21:02

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.