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.

Please review these activities which handle converting units in an Android app:

I would appreciate any guidance and advice on code refactoring and different design patterns or strategies that I could use to improve the code. Thank you!

AngleActivity:

public class AngleActivity extends BaseUnitActivity implements OnItemSelectedListener{

private EditText editConversionValue;
private TextView resultView;
private Spinner spinnerAreaFrom;
private Spinner spinnerAreaTo;
private static float value;
private float result;
private float enteredFloatValue;
private int indexFrom;
private int indexTo;
private String printUnit;

@Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    this.setContentView(R.layout.list_item_view);

    //Receive attached intent data
    Intent i = getIntent();
    final String unitName = i.getStringExtra("unitName");

    resultView = (TextView) findViewById(R.id.resultView);
    editConversionValue = (EditText) findViewById(R.id.editConversionValue);
    Button convertButton = (Button) findViewById(R.id.buttonConvert);
    spinnerAreaFrom = (Spinner) findViewById(R.id.convertFromField);
    spinnerAreaTo = (Spinner) findViewById(R.id.convertToField);

    //Create ArrayAdapter to receive data from angle_units[] array
    ArrayAdapter<CharSequence> adapter1 = ArrayAdapter.createFromResource(this, R.array.angle_units, R.layout.spinner_item);
    adapter1.setDropDownViewResource(R.layout.spinner_dropdown_item);

    spinnerAreaFrom.setAdapter(adapter1);
    spinnerAreaTo.setAdapter(adapter1);
    spinnerAreaFrom.setOnItemSelectedListener(this);
    spinnerAreaTo.setOnItemSelectedListener(this);

    editConversionValue.setText(null);

    /*
     * convertButton handles conversion and checks for entered value conditions
     */
    convertButton.setOnClickListener(new OnClickListener() {
        @Override
        public void onClick(View arg0) throws ArithmeticException {

            indexFrom = spinnerAreaFrom.getSelectedItemPosition();
            indexTo = spinnerAreaTo.getSelectedItemPosition();

            printUnit = String.valueOf(spinnerAreaTo.getSelectedItem());
            String enteredValue = editConversionValue.getText().toString();
            enteredFloatValue = Float.valueOf(editConversionValue.getText().toString());

            if (enteredValue.matches("")) {

                checkForInput();

            } else if (enteredFloatValue == 0) {

                checkForInput();
                Log.d("Incorrect entered value", "Entered value cannot be less than zero");
                throw new ArithmeticException();

            } else if (enteredFloatValue < 0) {

                Log.d("Incorrect entered value", "Entered value cannot be less than zero");
                throw new IllegalArgumentException();

            } else if (enteredFloatValue > 0) {

                value = Float.parseFloat(editConversionValue.getText().toString());
                getFloatRatio(unitName);
                result = value / ratio[indexFrom] * ratio[indexTo];
                resultView.setText(result + " " + printUnit);

            }

        }

    });

    /*
     * resultView presents result of conversion and updates if action_switch is initiated
     */
    resultView.setOnClickListener(new View.OnClickListener() {
        @SuppressWarnings("deprecation")
        @Override
        public void onClick(View view) {

            ClipboardManager copyResult = (ClipboardManager) getApplicationContext().getSystemService(Context.CLIPBOARD_SERVICE);
            String resultingValue = Float.toString(result);
            copyResult.setText(resultingValue);
            Toast toast2 = Toast.makeText(getApplicationContext(), "Copied to clipboard", Toast.LENGTH_SHORT);
            toast2.setGravity(Gravity.BOTTOM, 0, 60);
            toast2.show();

        }

    });

}

/*
 * Creates menu items and action bar items
 */
@Override
public boolean onCreateOptionsMenu(Menu menu) {

    //Inflate the menu items for use in the action bar
    MenuInflater inflater = getMenuInflater();
    inflater.inflate(R.menu.main_list, menu);
    inflater.inflate(R.menu.action_switch, menu);
    return super.onCreateOptionsMenu(menu);

}

/*
 * Handles menu items and action bar items
 */
@Override
public boolean onOptionsItemSelected(MenuItem item) {

    //Handles presses on the action bar items
    switch (item.getItemId()) {

        //Action bar item allowing user to switch conversion units
        case R.id.action_switch:

            int indexFromSwitch = spinnerAreaFrom.getSelectedItemPosition();
            int indexToSwitch = spinnerAreaTo.getSelectedItemPosition();

            spinnerAreaFrom.setSelection(indexToSwitch);
            spinnerAreaTo.setSelection(indexFromSwitch);

            if(spinnerAreaFrom.getSelectedItemPosition() == spinnerAreaTo.getSelectedItemPosition()) {

                Toast toast2 = Toast.makeText(getApplicationContext(), "Please choose a different value to convert to.", Toast.LENGTH_SHORT);
                toast2.setGravity(Gravity.BOTTOM, 0, 60);
                toast2.show();
                return true;

            } else if((spinnerAreaFrom != spinnerAreaTo & editConversionValue.getText().toString().matches(""))) {

                Toast toast1 = Toast.makeText(getApplicationContext(), "Please enter a conversion value.", Toast.LENGTH_SHORT);
                toast1.setGravity(Gravity.BOTTOM, 0, 60);
                toast1.show();
                return true;

            }

            result = value / ratio[indexToSwitch] * ratio[indexFromSwitch];
            resultView.setText(result + " " + printUnit);
            break;

        //Menu item that launches email intent to request a unit
        case R.id.request_unit:

            final Intent emailIntent = new Intent(Intent.ACTION_SEND);

            emailIntent.setType("plain/text");
            emailIntent.putExtra(android.content.Intent.EXTRA_EMAIL, new String[]{"kalsigurbir@gmail.com"});

            this.startActivity(Intent.createChooser(emailIntent, "Send mail via"));
            break;

    }
    return true;
}

@Override
public void onItemSelected(AdapterView<?> arg0, View arg1, int arg2,
                           long arg3) {

}

@Override
public void onNothingSelected(AdapterView<?> arg0) {


}

}

BaseUnitActivity:

public class BaseUnitActivity extends Activity {

public float ratio[];

public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

}

public void checkForInput() {

    Toast toast1 = Toast.makeText(getApplicationContext(), "Please enter a conversion value", Toast.LENGTH_SHORT);
    toast1.setGravity(Gravity.BOTTOM, 0, 60);
    toast1.show();

}

public void getFloatRatio(String unitName) {

    if (unitName.equals("Area")) {

        ratio = new float[]{(float) (2.47105381 * 10e-8f), (float) (1.0 * 10e-8f), 1.0f, 0.00107639104f, 0.15500031f, (float) (1.0 * 10e-10f), 0.0001f, (float) (3.86102159 * 10e-11f), 100f, 0.000119599005f};

    } else if (unitName.equals("Angle")) {

        ratio = new float[] {1f, 1.1111f, 0.01745f};

    } else if (unitName.equals("Length")) {

        ratio = new float[]{1f, 0.0328084f, 0.393701f, 1.0e-5f, 0.01f, 6.21371e-6f, 10f, 10,000,000f, 0.0109361f};

    } else if (unitName.equals("Speed")) {

        ratio = new float[]{1f, 60f, 0.018288f, 1.09728f, 0.592484f, 0.3048f, 0.0113636364f, 0.681818f};

    } else if (unitName.equals("Temperature")) {

        ratio = new float[] {1f, 33.8f, 274.15f};

    } else if (unitName.equals("Time")) {

        ratio = new float[]{1f, 24f, 1440f, 30f, 86400f, 0.142857f, 0.00273791f};

    } else if (unitName.equals("Volume")) {

        ratio = new float[]{1f, (float)3.53146667*10e-5f, 0.0610237441f, (float)1.0*10e-6f, (float)1.30795062*10e-6f, 0.00422675284f, 0.270512182f, 0.0338140227f, 0.000264172052f, 0.00105668821f, 0.001f, 1f, (float)6.28981077*10e-6f, 0.00211337642f, 0.0676280454f, 0.202884136f};

    } else if (unitName.equals("Weight")) {

        ratio = new float[]{1f, 0.06479891f, (float)6.479891*10e-5f, (float)6.479891*10e-8f, 64.79891f, 0.00228571429f, 0.000142857143f, (float)7.14285714*10e-8f, (float)1.02040816*10e-5f, 0.00208333333f};
    }

}

}

MainListActivity:

    public class MainListActivity extends ListActivity {

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

    //Store string resources into an array
    String[] unit_names = getResources().getStringArray(R.array.unit_names);

    //Bind string resources to a list adapter
    this.setListAdapter(new ArrayAdapter<String>(this, R.layout.list_item, R.id.label, unit_names));

    //Style the listView
    ListView listView = getListView();
    int[] colors = {0x33999999, 0xff33b5e5, 0x33999999};
    GradientDrawable gradient = new GradientDrawable(Orientation.BL_TR, colors);
    listView.setDivider(gradient);
    listView.setDividerHeight(2);

    listView.setOnItemClickListener(new OnItemClickListener() {
        public void onItemClick(AdapterView<?> parent, View view, int position, long id) {

            String unitName = ((TextView) view).getText().toString();

            if (unitName.equals("Area")) {

                Intent areaActivity = new Intent(getApplicationContext(), AreaActivity.class);
                areaActivity.putExtra("unitName", unitName);
                startActivity(areaActivity);

            } else if (unitName.equals("Angle")) {

                Intent angleActivity = new Intent(getApplicationContext(), AngleActivity.class);
                angleActivity.putExtra("unitName", unitName);
                startActivity(angleActivity);

            } else if (unitName.equals("Length")) {

                Intent lengthView = new Intent(getApplicationContext(), LengthView.class);
                lengthView.putExtra("unitName", unitName);
                startActivity(lengthView);

            } else if (unitName.equals("Speed")) {

                Intent speedView = new Intent(getApplicationContext(), SpeedActivity.class);
                speedView.putExtra("unitName", unitName);
                startActivity(speedView);

            } else if (unitName.equals("Temperature")) {

                Intent temperatureView = new Intent(getApplicationContext(), TemperatureView.class);
                temperatureView.putExtra("unitName", unitName);
                startActivity(temperatureView);

            } else if (unitName.equals("Time")) {

                Intent timeView = new Intent(getApplicationContext(), TimeView.class);
                timeView.putExtra("unitName", unitName);
                startActivity(timeView);

            } else if (unitName.equals("Volume")) {

                Intent volumeView = new Intent(getApplicationContext(), VolumeView.class);
                volumeView.putExtra("unitName", unitName);
                startActivity(volumeView);

            } else if (unitName.equals("Weight")) {

                Intent weightView = new Intent(getApplicationContext(), WeightView.class);
                weightView.putExtra("unitName", unitName);
                startActivity(weightView);

            }
        }
    });
}

@Override
public boolean onCreateOptionsMenu(Menu menu) {
    // Inflate the menu; this adds items to the action bar if it is present.
    getMenuInflater().inflate(R.menu.main_list, menu);
    return true;
}

    }
share|improve this question
    
Why float instead of double or even better some high precision type? –  CodesInChaos Jun 23 at 9:00
add comment

1 Answer 1

up vote 8 down vote accepted

Here's some things I've found by skimming through your code a bit. There is more things that can be said, but this should get you started:


public float ratio[];

Don't use public fields. I expect you want this to be accessible to subclasses, so you can use protected instead. If it needs access elsewhere, it's a better practice to expose a public get method.


The method checkForInput has a misleading name. It doesn't check anything, it shows a toast. A better name would be displayInputRequestToast or similar.


Use @Override on methods whenever possible, such as for onCreate in BaseUnitActivity. Speaking about onCreate in BaseUnitActivity, it is not needed at all since all it does is call the super method.


When you need a context in an activity, you can use this instead of getApplicationContext(). All Activities extends Context.


Use Strings.xml also in the checkForInput method. You're using it at other places, but you should use it everywhere possible (Except for Log.d stuff)


Use an ENUM type instead of unitName.equals(...)

public enum UnitType {
    AREA, ANGLE, LENGTH, SPEED, TEMPERATURE, TIME, VOLUME, WEIGHT;
}

Remember that enums can have constructors and methods in Java! Look at Oracle's Planet example. Take a look at Oracle's tutorial for an example on how enums can be used.

Then you can use a switch on the enum, or compare using ==.


It's unclear how the ratio is being used. It looks like a whole bunch of magic numbers to me.

Consider adding a comment where you're initializing it to explain it better.


Your if-elseif-elseif-elseif-else sequence can be improved dramatically.

First step:

 Intent intent;
 if (unitName.equals("Area")) {
     intent = new Intent(getApplicationContext(), AreaActivity.class);
 } else if (unitName.equals("Angle")) {
     ....
 }
 intent.putExtra("unitName", unitName);
 startActivity(intent);

Second step: Use something like this:

Map<UnitType, Class<? extends Activity>> unitTypeActivities = new HashMap<...>();
unitTypeActivities.put(UnitType.AREA, AreaActivity.class);
unitTypeActivities.put(...);
....

Intent intent = new Intent(this, unitTypeActivities.get(unitType));
intent.putExtra("unitName", unitName);
startActivity(intent);

You're currently using a lot of Activities, consider using Fragments instead!


You're throwing exceptions in an onClick method for a button. Those aren't caught anywhere and is likely to cause your application to crash. That is not recommended. Do not throw exceptions there, show an Alert dialogs or Toast instead!


There's a little problem with this line:

ratio = new float[]{1f, 0.0328084f, 0.393701f, 1.0e-5f, 0.01f, 6.21371e-6f, 10f, 10,000,000f, 0.0109361f};

10,000,000f is not one float value, it's actually three floats. 10, 000 and 000. In Java 8 you can use _ as separator. Android however, doesn't really support Java 8 code yet.

share|improve this answer
    
Thank you! Your comments are very helpful and I'm looking over my code. Would this code sample be acceptable for a high school student to show an employer? –  Novo Jun 22 at 20:55
    
AFAIR you can use underscore as of Java7 already –  Vogel612 Jun 22 at 23:03
    
@Novo Honestly, I don't know. There's quite a lot of room for improvement in the code, but if you're in high school then you have plenty of time and possibility to improve. Also, if you show the code now and they would reject you, show some code in about six months from now and you have hopefully improved quite a bit. –  Simon André Forsberg Jun 22 at 23:30
    
@Vogel612 At first I didn't believe you, but it seems like you're right –  Simon André Forsberg Jun 22 at 23:33
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.