6
\$\begingroup\$

I am developing a unit converter application for Android. The application is working correctly, but my problem is with the optimization. Is there a way to somehow optimize the code?

I have different categories - speed, weight, length and so on, and for each of them I have separate activities. Here, for example, is the code for one of them:

public class Speed extends Activity {

    Map<Integer, Double> fromKmhMap = new HashMap<>();
    Map<Integer, Double> fromKmminMap = new HashMap<>();
    Map<Integer, Double> fromKmsMap = new HashMap<>();
    Map<Integer, Double> fromKnotMap = new HashMap<>();
    Map<Integer, Double> fromMphMap = new HashMap<>();
    private RadioGroup fromRadioGroup, toRadioGroup;
    private TextView from, to;
    private EditText fromInput, toOutput;
    private TextWatcher valueTextWatcher = new TextWatcher() {
        @Override
        public void beforeTextChanged(CharSequence s, int start, int count, int after) {

        }

        @Override
        public void onTextChanged(CharSequence s, int start, int before, int count) {

        }

        @Override
        public void afterTextChanged(Editable s) {
            if (fromInput.getText().toString().trim().length() > 0) {
                checkFrom();
            } else {
                toOutput.setText("");
            }
        }
    };

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

        fromKmhMap.put(R.id.toKmh, 1.0);
        fromKmhMap.put(R.id.toKmmin, 0.01666666666667);
        fromKmhMap.put(R.id.toKms, 0.0002777777777778 );
        fromKmhMap.put(R.id.toKnot, 0.5399568034557);
        fromKmhMap.put(R.id.toMph, 0.6213711922373);

        fromKmminMap.put(R.id.toKmh, 60.0);
        fromKmminMap.put(R.id.toKmmin, 1.0);
        fromKmminMap.put(R.id.toKms, 0.01666666666667);
        fromKmminMap.put(R.id.toKnot, 32.39740820734);
        fromKmminMap.put(R.id.toMph, 37.28227153424);

        fromKmsMap.put(R.id.toKmh, 3600.0);
        fromKmsMap.put(R.id.toKmmin, 60.0);
        fromKmsMap.put(R.id.toKms, 1.0);
        fromKmsMap.put(R.id.toKnot, 1943.844492441);
        fromKmsMap.put(R.id.toMph, 2236.936292054);

        fromKnotMap.put(R.id.toKmh, 1.852);
        fromKnotMap.put(R.id.toKmmin, 0.03086666666667);
        fromKnotMap.put(R.id.toKms, 0.0005144444444444);
        fromKnotMap.put(R.id.toKnot, 1.0);
        fromKnotMap.put(R.id.toMph, 1.150779448024);

        fromMphMap.put(R.id.toKmh, 1.609344);
        fromMphMap.put(R.id.toKmmin, 0.0268224);
        fromMphMap.put(R.id.toKms, 0.00044704);
        fromMphMap.put(R.id.toKnot, 0.8689762419006);
        fromMphMap.put(R.id.toMph, 1.0);

        fromRadioGroup = (RadioGroup) findViewById(R.id.fromSpeedGroup);
        toRadioGroup = (RadioGroup) findViewById(R.id.toSpeedGroup);
        from = (TextView) findViewById(R.id.speedFrom);
        to = (TextView) findViewById(R.id.speedTo);
        fromInput = (EditText) findViewById(R.id.speedInput);
        toOutput = (EditText) findViewById(R.id.speedOutput);
        fromInput.addTextChangedListener(valueTextWatcher);

        fromRadioGroup.setOnCheckedChangeListener(new RadioGroup.OnCheckedChangeListener() {
            @Override
            public void onCheckedChanged(RadioGroup group, int checkedId) {
                if (fromInput.getText().toString().trim().length() > 0) {
                              checkFrom();
                }
            }
        });

        toRadioGroup.setOnCheckedChangeListener(new RadioGroup.OnCheckedChangeListener() {
            @Override
            public void onCheckedChanged(RadioGroup group, int checkedId) {
                if (fromInput.getText().toString().trim().length() > 0) {
                           checkFrom();
                }

            }
        });
    }

    private void fromKmh(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromKmhMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromKmmin(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromKmminMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromKms(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromKmsMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromKnot(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromKnotMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromMph(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromMphMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void checkFrom() {
        if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKmh) {
            fromKmh(Double.parseDouble(fromInput.getText().toString()));
        } else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKmmin) {
            fromKmmin(Double.parseDouble(fromInput.getText().toString()));
        } else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKms) {
            fromKms(Double.parseDouble(fromInput.getText().toString()));
        } else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKnot) {
            fromKnot(Double.parseDouble(fromInput.getText().toString()));
        } else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromMph) {
            fromMph(Double.parseDouble(fromInput.getText().toString()));
        }
    }
}

Here is another activity for weight measurements:

public class Weight extends Activity {

    Map<Integer, Double> fromGramMap = new HashMap<>();
    Map<Integer, Double> fromKilogramMap = new HashMap<>();
    Map<Integer, Double> fromMicrogramMap = new HashMap<>();
    Map<Integer, Double> fromMilligramMap = new HashMap<>();
    Map<Integer, Double> fromOunceMap = new HashMap<>();
    Map<Integer, Double> fromPoundMap = new HashMap<>();
    Map<Integer, Double> fromTroyOunceMap = new HashMap<>();

    private RadioGroup fromRadioGroup, toRadioGroup;
    private TextView from, to;
    private EditText fromInput, toOutput;
    private TextWatcher valueTextWatcher = new TextWatcher() {
        @Override
        public void beforeTextChanged(CharSequence s, int start, int count, int after) {

        }

        @Override
        public void onTextChanged(CharSequence s, int start, int before, int count) {

        }

        @Override
        public void afterTextChanged(Editable s) {
            if (fromInput.getText().toString().trim().length() > 0) {
                checkFrom();
            } else {
                toOutput.setText("");
            }
        }
    };

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

        fromGramMap.put(R.id.toGram, 1.0);
        fromGramMap.put(R.id.toKilogram, 0.001);
        fromGramMap.put(R.id.toMicrogram, 1000000.0);
        fromGramMap.put(R.id.toMilligram, 1000.0);
        fromGramMap.put(R.id.toOunce, 0.03527396194958);
        fromGramMap.put(R.id.toPound, 0.002204622621849);
        fromGramMap.put(R.id.toTroyOunce, 0.03215074656863);

        fromKilogramMap.put(R.id.toGram, 1000.0);
        fromKilogramMap.put(R.id.toKilogram, 1.0);
        fromKilogramMap.put(R.id.toMicrogram, 1000000000.0);
        fromKilogramMap.put(R.id.toMilligram, 1000000.0);
        fromKilogramMap.put(R.id.toOunce, 35.27396194958);
        fromKilogramMap.put(R.id.toPound, 2.204622621849);
        fromKilogramMap.put(R.id.toTroyOunce, 32.15074656863);

        fromMicrogramMap.put(R.id.toGram, 0.000001 );
        fromMicrogramMap.put(R.id.toKilogram, 0.000000001);
        fromMicrogramMap.put(R.id.toMicrogram, 1.0);
        fromMicrogramMap.put(R.id.toMilligram, 0.001 );
        fromMicrogramMap.put(R.id.toOunce, 0.000000035273);
        fromMicrogramMap.put(R.id.toPound, 0.00000000220);
        fromMicrogramMap.put(R.id.toTroyOunce, 0.0000000321);

        fromMilligramMap.put(R.id.toGram, 0.001  );
        fromMilligramMap.put(R.id.toKilogram, 0.000001 );
        fromMilligramMap.put(R.id.toMicrogram, 1000.0);
        fromMilligramMap.put(R.id.toMilligram, 1.0 );
        fromMilligramMap.put(R.id.toOunce, 0.00003527396194958 );
        fromMilligramMap.put(R.id.toPound, 0.000002204622621849 );
        fromMilligramMap.put(R.id.toTroyOunce, 0.00003215074656863 );

        fromOunceMap.put(R.id.toGram, 28.349523125   );
        fromOunceMap.put(R.id.toKilogram, 0.028349523125  );
        fromOunceMap.put(R.id.toMicrogram, 28349523.125 );
        fromOunceMap.put(R.id.toMilligram, 28349.523125  );
        fromOunceMap.put(R.id.toOunce, 1.0 );
        fromOunceMap.put(R.id.toPound, 0.0625  );
        fromOunceMap.put(R.id.toTroyOunce, 0.9114583333333  );

        fromPoundMap.put(R.id.toGram, 453.59237    );
        fromPoundMap.put(R.id.toKilogram, 0.45359237   );
        fromPoundMap.put(R.id.toMicrogram, 453592370.0 );
        fromPoundMap.put(R.id.toMilligram, 453592.37   );
        fromPoundMap.put(R.id.toOunce, 16.0 );
        fromPoundMap.put(R.id.toPound, 1.0  );
        fromPoundMap.put(R.id.toTroyOunce, 14.58333333333   );

        fromTroyOunceMap.put(R.id.toGram, 31.1034768     );
        fromTroyOunceMap.put(R.id.toKilogram, 0.0311034768    );
        fromTroyOunceMap.put(R.id.toMicrogram, 31103476.8  );
        fromTroyOunceMap.put(R.id.toMilligram, 31103.4768    );
        fromTroyOunceMap.put(R.id.toOunce, 1.097142857143  );
        fromTroyOunceMap.put(R.id.toPound, 0.06857142857143   );
        fromTroyOunceMap.put(R.id.toTroyOunce, 1.0   );

        fromRadioGroup = (RadioGroup) findViewById(R.id.fromWeightGroup);
        toRadioGroup = (RadioGroup) findViewById(R.id.toWeightGroup);
        from = (TextView) findViewById(R.id.weightFrom);
        to = (TextView) findViewById(R.id.weightTo);
        fromInput = (EditText) findViewById(R.id.weightInput);
        toOutput = (EditText) findViewById(R.id.weightOutput);
        fromInput.addTextChangedListener(valueTextWatcher);

        fromRadioGroup.setOnCheckedChangeListener(new RadioGroup.OnCheckedChangeListener() {
            @Override
            public void onCheckedChanged(RadioGroup group, int checkedId) {
                if (fromInput.getText().toString().trim().length() > 0) {
                    checkFrom();
                }
            }
        });

        toRadioGroup.setOnCheckedChangeListener(new RadioGroup.OnCheckedChangeListener() {
            @Override
            public void onCheckedChanged(RadioGroup group, int checkedId) {
                if (fromInput.getText().toString().trim().length() > 0) {
                   checkFrom();
                }

            }
        });
    }

    private void fromGram(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromGramMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromKilogram(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromKilogramMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromMicrogram(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromMicrogramMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromMilligram(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromMilligramMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromOunce(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromOunceMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromPound(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromPoundMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void fromTroyOunce(double valueToConvert)
    {
        toOutput.setText(String.valueOf(valueToConvert * fromTroyOunceMap.get(toRadioGroup.getCheckedRadioButtonId())));
    }

    private void checkFrom() {
        if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromGram) {
            fromGram(Double.parseDouble(fromInput.getText().toString()));
        } else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKilogram) {
            fromKilogram(Double.parseDouble(fromInput.getText().toString()));
        } else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromMicrogram) {
            fromMicrogram(Double.parseDouble(fromInput.getText().toString()));
        } else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromMilligram) {
            fromMilligram(Double.parseDouble(fromInput.getText().toString()));
        } else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromOunce) {
            fromOunce(Double.parseDouble(fromInput.getText().toString()));
        }else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromPound) {
            fromPound(Double.parseDouble(fromInput.getText().toString()));
        }else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromTroyOunce) {
            fromTroyOunce(Double.parseDouble(fromInput.getText().toString()));
        }
    }
}
Improve this question
\$\endgroup\$
0

2 Answers 2

Reset to default
3
\$\begingroup\$

Avoid copy-paste coding. Take for example this piece of code:

    fromRadioGroup.setOnCheckedChangeListener(new RadioGroup.OnCheckedChangeListener() {
        @Override
        public void onCheckedChanged(RadioGroup group, int checkedId) {
            if (fromInput.getText().toString().trim().length() > 0) {
                          checkFrom();
            }
        }
    });

    toRadioGroup.setOnCheckedChangeListener(new RadioGroup.OnCheckedChangeListener() {
        @Override
        public void onCheckedChanged(RadioGroup group, int checkedId) {
            if (fromInput.getText().toString().trim().length() > 0) {
                       checkFrom();
            }

        }
    });

The condition inside the two anonymous implementations was clearly copy-pasted. It would be better to create a helper method with the common logic, in particular:

private String normalizedInput(EditText input) {
    return input.getText().toString().trim();
}

And then rewrite the condition like this:

String normalized = normalizedInput(fromInput);
if (!normalized.isEmpty()) {
    checkFrom(normalized);
}

I did more than just extracted to a helper method:

  • Replaced the .length() > 0 with the more natural !.isEmpty()
  • Pass the normalized string to checkFrom

This second point can be handy to prevent number parsing issues in the checkFrom method, or to make the number parsing more efficient thanks to the trimming.

Alternatively, as @Simon pointed out in a comment, you could create a single instance of RadioGroup.OnCheckedChangeListener and set that on both the setOnCheckedChangeListener calls.

You can eliminate the duplication similarly from this code too:

if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKmh) {
    fromKmh(Double.parseDouble(fromInput.getText().toString()));
} else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKmmin) {
    fromKmmin(Double.parseDouble(fromInput.getText().toString()));
} else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKms) {
    fromKms(Double.parseDouble(fromInput.getText().toString()));
} else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromKnot) {
    fromKnot(Double.parseDouble(fromInput.getText().toString()));
} else if (fromRadioGroup.getCheckedRadioButtonId() == R.id.fromMph) {
    fromMph(Double.parseDouble(fromInput.getText().toString()));
}

That is, I suggest to assign the result of Double.parseDouble(fromInput.getText().toString()) (or actually, Double.parseDouble(normalized) if you follow my recommendation earlier) to a variable, before entering the if-else chain.

Actually, it would be better to re-write this last if-else chain as a switch (fromRadioGroup.getCheckedRadioButtonId()) { ... }. I think that would be slightly more readable. (Thanks @Simon for the tip!)

Improve this answer
\$\endgroup\$
0
6
\$\begingroup\$

What you need is to use a common base unit that all your other units converts to.

For example, let's take a look at this:

fromGramMap.put(R.id.toGram, 1.0);
...
fromKilogramMap.put(R.id.toGram, 1000.0);
...
fromMicrogramMap.put(R.id.toGram, 0.000001 );
...
fromMilligramMap.put(R.id.toGram, 0.001  );

These are clearly related. 1 kilogram = 1000 gram = 1 000 000 milligram = 1 000 000 000 microgram. So instead of using multiple maps for gram, kilogram, microgram etc. Just use one base unit: Gram.

gramRatioMap.put(R.id.toGram, 1.0);
gramRatioMap.put(R.id.toKilogram, 0.001);
gramRatioMap.put(R.id.toMicrogram, 1000000.0);
gramRatioMap.put(R.id.toMilligram, 1000.0);
gramRatioMap.put(R.id.toOunce, 0.03527396194958);
gramRatioMap.put(R.id.toPound, 0.002204622621849);
gramRatioMap.put(R.id.toTroyOunce, 0.03215074656863);

Now, to convert from Kilogram to gram, you can lookup the value for R.id.toKilogram and then use 1 / (that value). In this case, \$1 / 0.001 = 1000\$

Then, once you have converted to gram, you use your gramRatioMap as normal and convert to that specific unit.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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