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.

Here is my take on (lighter version of) Java's BigDecimal.

Originally I wrote this as a replacement for BigDecimal in order to improve performance during serialisation and data manipulation.

public class SimpleDecimal implements Comparable<SimpleDecimal> {
    private long    value = 0L;
    private int     fractionalPrecision = 0;
    private boolean debug = false;


    public static SimpleDecimal simplify(SimpleDecimal o) {
        if (!((o.getFractionalPrecision() > 0) && ((o.getValue() % 10) == 0))) {
            return o;
        }
        SimpleDecimal sd = new SimpleDecimal(o);
        do {
                sd.setValue(sd.getValue() / 10);
                sd.setFractionalPrecision(sd.getFractionalPrecision() - 1);
        } while ((sd.getFractionalPrecision() > 0) && ((sd.getValue() % 10) == 0));
        return sd;
    }

    public static SimpleDecimal adjust(SimpleDecimal o, int fractionalPrecision) {
        if (o.getFractionalPrecision() == fractionalPrecision) {
            return o;
        }
        if (o.getFractionalPrecision() > fractionalPrecision) {
            return simplify(o);
        }
        SimpleDecimal sd = new SimpleDecimal(o);
        sd.incFractionalPrecision(fractionalPrecision - sd.getFractionalPrecision());
        return sd;
    }



    // Various constructors
    public SimpleDecimal(int value) {
        this(String.valueOf(value));
    }

    public SimpleDecimal(long value) {
        this(String.valueOf(value));
    }

    public SimpleDecimal(float value) {
        this(String.valueOf(value));
    }

    public SimpleDecimal(double value) {
        this(String.valueOf(value));
    }

    /**
     * This method will expect standard representation of numerical value as returned by
     * String.valueOf(...)
     * @param value
     */
    public SimpleDecimal(String value) {
        int fpsl = value.lastIndexOf('.');
        String strValue;
        if (fpsl > 0) {
            strValue = value.substring(0, fpsl) + value.substring(fpsl + 1);
        } else {
            strValue = value.substring(fpsl + 1);
        }
        setValue(Long.valueOf(strValue));

        if (fpsl < 0) {
            fpsl = 0;
        } else {
            fpsl = value.length() - fpsl - 1;
        }
        setFractionalPrecision(fpsl);
    }

    public SimpleDecimal(SimpleDecimal o) {
        this(o.getValue(), o.getFractionalPrecision());
    }

    public SimpleDecimal add(SimpleDecimal augend) {
        SimpleDecimal result = new SimpleDecimal(this);
        int fp = result.getFractionalPrecision();
        if (fp < augend.getFractionalPrecision()) {
            fp = augend.getFractionalPrecision();
            result = adjust(result, fp);
        } else {
            augend = adjust(augend, fp);
        }
        result.setValue(result.getValue() + augend.getValue());
        return result;
    }

    public SimpleDecimal subtract(SimpleDecimal subtrahend) {
        SimpleDecimal result = new SimpleDecimal(this);
        int fp = result.getFractionalPrecision();
        if (fp < subtrahend.getFractionalPrecision()) {
            fp = subtrahend.getFractionalPrecision();
            result = adjust(result, fp);
        } else {
            subtrahend = adjust(subtrahend, fp);
        }
        result.setValue(result.getValue() - subtrahend.getValue());
        return result;
    }

    public SimpleDecimal multiply(SimpleDecimal multiplicand) {
        SimpleDecimal result = new SimpleDecimal(this);
        result.setValue(result.getValue() * multiplicand.getValue());
        result.setFractionalPrecision(result.getFractionalPrecision() + multiplicand.getFractionalPrecision());
        return result;
    }

    public SimpleDecimal divide(SimpleDecimal divisor) {
        SimpleDecimal result = new SimpleDecimal(this);
        int fp = result.getFractionalPrecision();
        if (fp < divisor.getFractionalPrecision()) {
            fp = divisor.getFractionalPrecision();
            result = adjust(result, fp + fp);
        } else {
            result = adjust(result, fp + fp);
            divisor = adjust(divisor, fp);
        }
        result.setValue(result.getValue() / divisor.getValue());
        result.setFractionalPrecision(fp);
        return result;
    }

    public SimpleDecimal simplify() {
        return simplify(this);
    }

    @Override
    public int compareTo(SimpleDecimal o) {
        SimpleDecimal sd1 = new SimpleDecimal(this);
        return compare(sd1, o);
    }

    // If you need to handle big values use BigDecimal instead
    public static int compare(SimpleDecimal o1, SimpleDecimal o2) {
        int result = 0;
        if (o1.getFractionalPrecision() == o2.getFractionalPrecision()) {
            return (int) (o1.getValue() - o2.getValue());
        }
        SimpleDecimal sd1 = simplify(o1);
        SimpleDecimal sd2 = simplify(o2);
        int maxFractionalPrecision = sd1.getFractionalPrecision();
        if (sd2.getFractionalPrecision() > maxFractionalPrecision) {
            maxFractionalPrecision = sd2.getFractionalPrecision();
        }
        sd1 = adjust(sd1, maxFractionalPrecision);
        sd2 = adjust(sd2, maxFractionalPrecision);
        if (sd1.getFractionalPrecision() == sd2.getFractionalPrecision()) {
            return (int) (sd1.getValue() - sd2.getValue());
        }
        return result;
    }

    @Override
    public String toString() {
        String str;
        if (isDebug()) {
        // Use this for debugging
            str = debugToString();
        } else {
        // Use this for normal output
            str = stdToString();
        }
        return str;
    }

    @Override
    public boolean equals(Object o) {
        if (null == o)
            return false;
        if (!this.getClass().isInstance(o))
            return false;
        SimpleDecimal sd = (SimpleDecimal) o;
        return equals(sd);
    }

    public boolean equals(SimpleDecimal o) {
        boolean result = false;
        if (compareTo(o) == 0) {
            result = true;
        }
        return result;
    }

    public SimpleDecimal movePointLeft(int n) {
        SimpleDecimal sd = new SimpleDecimal(this);
        sd.incFractionalPrecision(n);
        return sd;
    }

    public SimpleDecimal movePointRight(int n) {
        SimpleDecimal sd = new SimpleDecimal(this);
        sd.decFractionalPrecision(n);
        return sd;
    }




    // Standard getters/setters
    public long getValue() {
        return value;
    }

    public void setValue(long value) {
        this.value = value;
    }

    public int getFractionalPrecision() {
        return fractionalPrecision;
    }

    public void setFractionalPrecision(int fractionalPrecision) {
        this.fractionalPrecision = fractionalPrecision;
    }

    public void incFractionalPrecision() {
        int fractionalPrecision = 1;
        incFractionalPrecision(fractionalPrecision);
    }

    public synchronized void incFractionalPrecision(int fractionalPrecision) {
        for (int i = 0; i < fractionalPrecision; i++) {
            this.value *= 10;
        }
        this.fractionalPrecision += fractionalPrecision;
    }

    public void decFractionalPrecision() {
        int fractionalPrecision = 1;
        decFractionalPrecision(fractionalPrecision);
    }

    public synchronized void decFractionalPrecision(int fractionalPrecision) {
        if (fractionalPrecision > this.fractionalPrecision) {
            fractionalPrecision = this.fractionalPrecision;
        }
        for (int i = 0; i < fractionalPrecision; i++) {
            this.value /= 10;
        }
        this.fractionalPrecision -= fractionalPrecision;
    }

    public boolean isDebug() {
        return debug;
    }

    public void setDebug(boolean debug) {
        this.debug = debug;
    }

    private SimpleDecimal(long value, int fractionalPrecision) {
        super();
        this.value = value;
        this.fractionalPrecision = fractionalPrecision;
    }

    private String stdToString() {
        String strValue = String.valueOf(getValue());
        String str;
        if (getFractionalPrecision() > 0) {
            StringBuilder sb = new StringBuilder();
            for (int i = strValue.length(); i < getFractionalPrecision() + 1; i++) {
                sb.append('0');
            }
            sb.append(strValue);
            strValue = sb.toString();

            int dpp = strValue.length() - getFractionalPrecision();
            int p = 0;
            str = 
                ((getValue() >= 0) ? "" : strValue.substring(0, ++p))
                + ((dpp == p) ? "0" : strValue.substring(p, dpp))
                + '.' + strValue.substring(dpp);
        } else {
            str = strValue;
        }
        return str;
    }

    private String debugToString() {
        return "SimpleDecimal [" +
                "value=" + value
                + ", fractionalPrecision=" + fractionalPrecision
                + "]";
    }
}
share|improve this question
    
@200_success: thanks for a good question title. –  Germann Arlington 2 hours ago

2 Answers 2

up vote 6 down vote accepted

Your assertion that this is a replacement for BigDecimal, is.... ambitious.

You are limited to about 18 significant figures, and your code will throw odd exceptions for values that exceed that will throw a NumberFormatException, as far as I can tell.

Additionally, your class is not immutable, which is slightly concerning, since by convention, Java classes are immutable.

You override the equals method, but do not override the hashCode() method, which means you cannot safely use this value in a HashSet, HashMap, or many other constructs.

The simplify method is duplicated as both a public static method, as well as an instance method. There should be only one (the instance method, I presume), or the other should not be public.

The adjust method looks broken:

public static SimpleDecimal adjust(SimpleDecimal o, int fractionalPrecision) {
    if (o.getFractionalPrecision() == fractionalPrecision) {
        return o;
    }
    if (o.getFractionalPrecision() > fractionalPrecision) {
        return simplify(o);
    }
    SimpleDecimal sd = new SimpleDecimal(o);
    sd.incFractionalPrecision(fractionalPrecision - sd.getFractionalPrecision());
    return sd;
}

If the o's precision is greater than the input, you simplify it, but, simplify may do nothing..... so, the adjust does nothing? Also, what is it adjusting? The method is badly named, and I suspect it should just go away.

Your class does not extend java.lang.Number. This is not really essential, but, it would be nice.

Your primitive-based constructors do things the wrong way around... they all convert the primitives to String, then convert the string back to the source value. This is slow. You should be converting directly to long, and then have a more comprehensive String constructor.

All your arithmetic methods are adjusting the values to be stored in the SimpleDecimal as the calculation is done. This is against the immutable concept for Java numbers. You should instead calculate what the number-parts should be, and then construct the final result from those parts with no additional adjustment.

One final observation, your code is not what I would consider to be 'production ready'. It does not deal with common issues that arise, like, the value -.01 should be legal, but I believe will cause your code to fail, and fail in a consistent way with other failure modes. Additionally, I have, in the past, attempted to write a mutable-version of BigDecimal to reduce the memory impact it has.... and failed. The code is complex, and I fear your SimpleDecimal has oversimplified too many things, and made too many compromises and assumptions.

share|improve this answer
    
Thanks a lot for the comments. Originally the class was written to handle money/quantity calculations in my own project, so the precision was enough for the purpose. I will follow on all comments made here (can't promise that it will be immediately though). –  Germann Arlington 2 hours ago

Only things I can personally see:

  • Initializing to 0/false/0L is redundant/clutter
  • Don't indent your variables to be at the same point in text as other variables. It takes more effort to realign the variables every time you make a new one than to read the text on it's own (although this is personal preference)
  • I don't like how SimpleDecimal is mutable (i.e, setValue). Generally mathematical constructs are immutable and return new copies of themselves instead of mutating.
  • Remove the debug code.
share|improve this answer
    
Thanks, I did not even notice that setters are still public... –  Germann Arlington 20 hours ago
1  
You should remove the setters entirely. Public or private. Mutating mathematical constructs seems like a bad idea to me. –  Dan Pantry 20 hours ago
    
Indenting variables to be aligned is only a persona preference as long you use no VCS. If you do, it's clearly a pure nonsense. –  maaartinus 7 hours ago
    
Why would you not use a VCS? –  Dan Pantry 3 hours ago
    
Sorry but what is VCS in this case? –  Germann Arlington 2 hours ago

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.