3
\$\begingroup\$

Here is my progress calculator class. It is used for downloads/uploads. It takes bytes in and calculates the percentage(to the nearest 5%) that should show on a progress bar. Please let me know how I could improve it. Thanks a lot!!

public class ProgressCalculator {

    private int totalAmount;
    private int progressAmount;
    private int amountProgressItems;
    private int progressBarPercentage;

    //returns if the progress bar needs to change
    public boolean progress(int amount) {
        progressAmount = progressAmount + amount;
        return updateProgressbarPercentage();
    }

    public int getCurrentValue() {
        return progressBarPercentage;
    }

    //returns if the progressbar needs to change
    public boolean addProgressItem(int itemAmount) {
        totalAmount = totalAmount + itemAmount;
        amountProgressItems++;
        return updateProgressbarPercentage();
    }

    //returns true if has been updated;
    private boolean updateProgressbarPercentage() {
        int newProgressBarPercentage = round(calculateProgressPercentage());
        if (progressBarPercentage == newProgressBarPercentage) {
            return false;
        } else {
            progressBarPercentage = newProgressBarPercentage;
            return true;
        }
    }

    public int getAmountOfItems() {
        return amountProgressItems;
    }

    public boolean removeProgressItem(int byteAmount) {
        if (amountProgressItems > 0) {
            totalAmount = totalAmount - byteAmount;
            amountProgressItems--;
        }
        return updateProgressbarPercentage();
    }

    private int calculateProgressPercentage() {
        double x = progressAmount;
        double y = totalAmount;
        double result = (x / y) * 100;
        return (int) result;
    }

    private int round(int num) {
        int temp = num % 5;
        if (temp < 3)
            return num - temp;
        else
            return num + 5 - temp;
    }

    public void clear() {
        totalAmount = 0;
        progressAmount = 0;
        amountProgressItems = 0;
        progressBarPercentage = 0;
    }

}
\$\endgroup\$
4
\$\begingroup\$

It's really hard to tell what this class does. From what you say, I feel like you have a number of TransferTasks and want to have a sort of ProgressTracker for them. So, as for me, your goal is to make user's code look like this:

ProgressTracker progressTracker = new ProgressTracker();
progressTracker.setListener(new ProgressTrackerListener() {
  @Override
  public void onProgressChanged(double progressPercentage) {
    // TODO: update your UI here
  }
});
...
TransferTask transferTask = new UploadTask("/home/jiduvah/1.txt");
progressTracker.track(transferTask);
transferTask.start();

Why? Because it doesn't need any comments to describe what happens here: you just write it in English.

Update - In case you're absolutely sure you like your approach.

First, in this code:

private int round(int num) {
    int temp = num % 5;
    if (temp < 3)
        return num - temp;
    else
        return num + 5 - temp;
}

I see magic like 5 and 3. Looks like it does exactly what you need to do, and you've even wrote about meaning of 5, but 3 is still a mystery.

Then:

double x = progressAmount;
double y = totalAmount;
double result = (x / y) * 100;
return (int) result;

When you divide int by int, you get int. When you divide int by double or double by int, you get double. So you may just write:

double result = 100 * progressAmount / (double)totalAmount;

Regarding the whole idea, I'm not sure whether this code works at all, because it's hard to understand how one should use it.

\$\endgroup\$
  • \$\begingroup\$ Sorry I wasn't so clear but you got it right. I have a number of transferTasks and I need to track the overall progress. It looks like a very logical way to do it. Is the maths side of it ok? I was unsure with casting the int from a double to work out the percentage. EDIT - never mind, it was answered below \$\endgroup\$ – jiduvah Jun 27 '12 at 13:18
  • \$\begingroup\$ Updated my answer. \$\endgroup\$ – Andrey Agibalov Jun 27 '12 at 13:28
  • \$\begingroup\$ with regards to the 3 and 5, Its rounding to the nearest 5%. Is there a better way to do it? \$\endgroup\$ – jiduvah Jun 27 '12 at 13:30
  • \$\begingroup\$ ideone.com/pI4TN \$\endgroup\$ – Andrey Agibalov Jun 27 '12 at 13:34
  • 1
    \$\begingroup\$ To get the same that you already have, fix like this: quantizer * ((i + quantizer / 2) / quantizer), where quantizer is still 5. \$\endgroup\$ – Andrey Agibalov Jun 27 '12 at 14:00
3
\$\begingroup\$

The only thing that sticks out to me is amountProgressItems. Why do you need it?

It's only internal use is to prevent removeProgressItem from being called more times than addProgressItem was called. Furthermore, we do not have an guarantee that remove will be called with the same byteAmount parameters that add was called.

Personally, I would change these methods into increaseTotalAmount(int) and decreaseTotalAmount(int).

All that said, if you have an external use for progress items, it's not horrible, so don't feel the need to change it.


One last thing, don't divide by zero, and calculateProgressPercentage can be one lined:

private int calculateProgressPercentage() {
    if(totalAmount == 0) return 0;
    return (int) (progressAmount * 100.0 / totalAmount);
}

Using 100.0 (not 100) will result in the numerator being a double, causing the result to likewise be a double.

\$\endgroup\$
  • \$\begingroup\$ thanks for that, I up voted instead of marking correct as the other answer was a little more involved \$\endgroup\$ – jiduvah Jun 27 '12 at 14:17

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.