4
\$\begingroup\$

I am setting the sum total of all the items in an array to a text view in android.The values contained in the array are all currency values.

@Override
        public void afterTextChanged(Editable s) {

            int id = 0;

            id = (int) itemtotal.getTag();
            String itemTot = itemtotal.getText().toString();


            if (id >= 0 && id <= totals.size()) {
                totals.set(id - 1, itemTot);

                double[] doubleList = new double[totals.size()];
                double newsum = 0.0d;
                for (int i = 0; i < totals.size(); ++i) {
                    doubleList[i] = Double.parseDouble(totals.get(i));
                    newsum += (double) Math.round((doubleList[i]) * 100.0) / 100.0;
                }
                textViewSum.setText(((double) Math.round((newsum * (0.02) + newsum) * 100.0) / 100.0) + "");//set total text to sum
                textViewVat.setText(((double) Math.round((newsum * (0.02)) * 100.0) / 100.0) + "");
            }
        }

So I am basically calculating the sum total and the vat which is 2%.

The code is pretty straightforward, but I am interested in any possible improvement.

\$\endgroup\$
6
\$\begingroup\$

Naming

s is a bad parameter name. If there's nothing better, use editable.

Avoid abbreviations. itemTot might be better as itemTotalValue.

Use camelCase. newsum should be newSum.

It's confusing that textViewSum doesn't actually hold the sum.

Implementation

Don't declare id as 0 and then reassign it right away. Just declare it as what it should be, and make it final because it won't be reassigned.

Guard clauses are very helpful for keeping code from looking like a big > sign. Not an issue now, but it might be if more code gets added to this method.

Don't declare itemtot until it's actually needed. You might leave the method before using the value.

You don't need an entire double[] to hold your computations. You only need one double variable declared inside the for loop.

It might be nice if there were parens to clarify the cast is before the division. Also, you don't need to cast to double. Dividing by 100.0 makes the result a double even though the dividend is a long.

It's ugly to use string concatenation to turn a double into a String. Java provides a method (Double.toString()) for simple conversions, and a class (NumberFormat) for complex conversions. Or you can use String.format(), which is a bit newfangled for me, but I hear the kids love it.

Result

If you were to apply all these suggestions your code might look something like:

@Override
public void afterTextChanged(final Editable editable) {

    final int id = (int) itemtotal.getTag();
    if ((id < 0) || (id > totals.size())) {
        return;
    }

    final String itemTotalValue = itemtotal.getText().toString();
    totals.set(id - 1, itemTotalValue);

    double newSum = 0.0d;
    for (int i = 0; i < totals.size(); ++i) {
        final double value = Double.parseDouble(totals.get(i));
        newSum += Math.round(value * 100) / 100.0;
    }

    final double sum = Math.round((newSum * 0.02 + newSum) * 100.0) / 100.0;
    textViewSum.setText(Double.toString(sum));

    final double vat = Math.round((newSum * 0.02) * 100.0) / 100.0;
    textViewVat.setText(Double.toString(vat));
}
\$\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.