HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Unit conversion activities

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
conversionactivitiesunit

Problem

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 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 = spinnerArea

Solution

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> 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.

Code Snippets

public float ratio[];
public enum UnitType {
    AREA, ANGLE, LENGTH, SPEED, TEMPERATURE, TIME, VOLUME, WEIGHT;
}
Intent intent;
 if (unitName.equals("Area")) {
     intent = new Intent(getApplicationContext(), AreaActivity.class);
 } else if (unitName.equals("Angle")) {
     ....
 }
 intent.putExtra("unitName", unitName);
 startActivity(intent);
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);
ratio = new float[]{1f, 0.0328084f, 0.393701f, 1.0e-5f, 0.01f, 6.21371e-6f, 10f, 10,000,000f, 0.0109361f};

Context

StackExchange Code Review Q#54968, answer score: 8

Revisions (0)

No revisions yet.