3
\$\begingroup\$

I am a newbie at writing Android apps and using Java in general. I have went through most of the Android hello view tutorials but still seem to be lacking some understanding of the basics. Here is a snippet of code I wrote for an app. My goal was to have three edittext boxes for the user to input information. I want the user to put information in two of the three boxes and when they hit the calculate button, it will calculate the third edittext value based on a specific equation. Is there anyway that I can avoid initializing these 3 edittext objects in each of my methods within this class? Any other suggestions for cleaning up or improving this code?

public class PlantPopulation extends Activity {

    /** Called when the activity is first created. */    
    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);

        EditText seedSpacing = (EditText) 
                findViewById(R.id.seedSpacing);
        EditText rowSpacing = (EditText) 
                findViewById(R.id.rowSpacing);
        EditText population = (EditText) 
                findViewById(R.id.population);

        seedSpacing.setText("");  //clear values
        rowSpacing.setText("");
        population.setText("");

    }

    //check how many boxes are empty
    public int checkifempty(String array[]) {

        int i = 0;
        for (String string : array) {
            if (string.equals("")) {
                i = i+1;
            }   
        }
        return i;
    }

    //run the calculation
    public void calc(View v) {  

        EditText seedSpacing = (EditText) 
                findViewById(R.id.seedSpacing);
        EditText rowSpacing = (EditText) 
                findViewById(R.id.rowSpacing);
        EditText population = (EditText) 
                findViewById(R.id.population);

        String sS = seedSpacing.getText().toString();
        String rS = rowSpacing.getText().toString();
        String pop = population.getText().toString();

        String boxes[] = {sS,rS,pop}; 
        //determine which box is empty
        if (checkifempty(boxes) <2) {

            if (sS.equals("")) {
                double calc1=(43560*144)/new Double(rS)/
                        new Double(pop);
                calc1 = Math.floor(calc1 * 100 +.5)/100;
                seedSpacing.setText(Double.toString(calc1));
            } else if (rS.equals("")) {
                double calc2=(43560*144)/new Double(sS)/
                        new Double(pop);
                calc2 = Math.round(calc2);
                rowSpacing.setText(Double.toString(calc2));
            } else if (pop.equals(""))  {
                Double calc3=((43560*144)/new Double(rS)/
                        new Double(sS));
                Integer calc = calc3.intValue();
                //calc3 = Math.round(calc3);
                population.setText(calc.toString());
            } else {
                Toast.makeText(PlantPopulation.this, 
                    "Leave one item blank.", Toast.LENGTH_SHORT).show();
            }   
        } else {
            Toast.makeText(PlantPopulation.this, 
                    "You must fill in two of the three boxes.", Toast.LENGTH_SHORT).show();
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ One suggestion... use the camel-case to name your functions/variables as you did in you class, though I only put in capital the first character on classes. \$\endgroup\$ – SeRPRo Nov 11 '11 at 15:32
  • \$\begingroup\$ Better fit for CodeReview. \$\endgroup\$ – ObscureRobot Nov 11 '11 at 22:02
2
\$\begingroup\$

Declare and initialise the edit texts like this:

public class PlantPopulation extends Activity {
    EditText seedSpacing;
    EditText rowSpacing;
    EditText population;

/** Called when the activity is first created. */    
    @Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.main);

    seedSpacing = (EditText) 
            findViewById(R.id.seedSpacing);
    rowSpacing = (EditText) 
            findViewById(R.id.rowSpacing);
    population = (EditText) 
            findViewById(R.id.population);

.....

You now will not need to re-declare and initialise them in other methods, namely your calc method in this example.

Hope that helps, Barry

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ When I tried that, i get a null point exception and the program will not run on the emulator. Maybe I am missing something? \$\endgroup\$ – Rick Nov 11 '11 at 16:18
  • \$\begingroup\$ Hmm... where are you calling calc from? \$\endgroup\$ – barry Nov 11 '11 at 16:39
  • \$\begingroup\$ i am calling it with the xml attribute android:onClick="calc" \$\endgroup\$ – Rick Nov 11 '11 at 18:01
  • \$\begingroup\$ I tried changing everything back to the way i had it and now can't get it to work. it seems like i keep getting the null point exception at the statement of seedSpacing.setText(""); or at any other edittext object. If i have this statement in the onCreate method, the program won't run. if i have this statement in either my calc or clear method, those crash the program when i click the corresponding button. \$\endgroup\$ – Rick Nov 11 '11 at 18:27
  • 1
    \$\begingroup\$ What do you mean by "the way you had it" and "this statement"? \$\endgroup\$ – barry Nov 12 '11 at 18:16
3
\$\begingroup\$
  • Replace "43560*144" by a constant.
  • Store new Double(sS), new Double(srS), new Double(pop) in variables before if (checkifempty(boxes) <2). It'll improve speed and clarify the code.
  • Correct indentation in the onCreate method
| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

Replace horrible checkIfEmpty method, which by the way doesn't do what you plan it to do. Use addTextChangedListener for your EditTexts.
In your xml, set property android:text to an empty value, so you can avoid doing it in the code.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ the checkIfEmpty is providing me a way to see how many boxes are left empty by the user, to avoid any calculation errors if more than one box is empty or if non are empty. how would i implement the addTextChangedListener to accomplish this same task? \$\endgroup\$ – Rick Nov 11 '11 at 15:53
3
\$\begingroup\$

Some general (Java) idea without any Android specific stuff:

1, Extract the get*Spacing methods, for example:

publiv EditText getSeedSpacing() {
    return (EditText) findViewById(R.id.seedSpacing);
}

It removes some code duplication.

2, Consider using i++ instead of i = i+1.

3, Use longer names:

String seedSpacingValue = seedSpacing.getText().toString();
String rowSpacingValue = rowSpacing.getText().toString();
String populationValue = population.getText().toString();

It makes the code more readable.

2, Rename the checkifempty method. It does not check anything, it returns the count of empty fields. So for the current functionality it should be countEmptyFields or something like this. Anyway, I'd write the following:

public int countEmptyFields(final EditText... fields) {
    int count = 0;
    for (EditText field: fields) {
        final String value = field.getText();
        if ("".equals(value)) {
            count++
        }   
    }
    return count;
}

public void showError(final String msg) {
    Toast.makeText(PlantPopulation.this, msg, Toast.LENGTH_SHORT).show();
}

then in the calc method:

...
final int emptyFields = countEmptyFields(seedSpacing, rowSpacing, population);  
if (emptyFields == 0) {
    showError("Leave one item blank.");
    return;
}
if (emptyFields > 2) {
    showError("You must fill in two of the three boxes.");
    return;
}
if (seedSpacingValue.equals("")) ...
...

The showError removes some code duplication while the == 0 and > 2 checks at the beginning replaces the nested blocks to an easier to follow structure.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ thank you for the suggestions. they make sense and your explanations were very helpful \$\endgroup\$ – Rick Nov 14 '11 at 15:22
2
\$\begingroup\$

Use Andject to get rid of something = getViewById():

https://github.com/ko5tik/andject

( Shameles self-advertizing )

Speciofy desired views via annotations:

  class WithInjectableViews extends Activity {
    // shall be injected
    @InjectView(id = 239)
    private android.view.View asView;
    @InjectView(id = 555)
    private Button button;
    // shall be left alone
    private View notInjected = null;

}

And in your onCreate() say:

  ViewInjector.startActivity(injectable);

There is also work undeway to inject shared preference values ( also provides cleaner code and declarative style )

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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