How to write code more elegantly? (Factorials, BigDecimals, division by BigIntegers)

Question

I've managed to make my code working, but feel like there's a better approach to writing similar stuff, any tips or mistakes to point out?

Here's my code:

  public static void main(String[] args) {
        DecimalFormat df = new DecimalFormat("0.##E0");
        BigDecimal a;
        BigInteger fact;
        int n=10;
        int x=3;

        for (int i=1; i<=n; i++){
            fact=BigInteger.valueOf(1);
            for (int j=1; j<=Math.pow(i,2)+1; j++){
            fact=fact.multiply(BigInteger.valueOf(j));
            }
        a=BigDecimal.valueOf((Math.pow(-1, i+1)*Math.log(i*x))/i).divide(new BigDecimal(fact), 500, BigDecimal.ROUND_HALF_EVEN);
        System.out.println(df.format(a));
        }
    }

I think this belongs on http://codereview.stackexchange.com/ — Koekje, Apr 09 '17 at 13:28
Yes. Most of the things that I could say about this code are "style" issues, which would make this (arguably) a question that requires "opinion based" answers. — Stephen C, Apr 09 '17 at 13:31
The other answer is that there is no substantially "better way" in terms of your use of Java language constructs. (But that is an opinion too.) — Stephen C, Apr 09 '17 at 13:34
One little nitpick: `i*i` is probably faster and easier than `Math.pow(i,2)` . — Rudy Velthuis, Apr 09 '17 at 19:58

score 1 · Accepted Answer · answered Apr 09 '17 at 20:50

I see the following improvements:

You can reduce the number of multiplications from O(n^3) to O(n^2) by starting with the value of fact from the previous iteration and multiply only with the missing values for j.
As mentioned in the comments, Math.pow(i,2) is an overkill; same holds for Math.pow(-1,i+1).
use BigDecimal.ONE

Together with some minor changes this gives:

public static void main(String[] args) {
  DecimalFormat df = new DecimalFormat("0.##E0");
  int n = 10;
  int x = 3;
  int scale = 500;

  BigInteger fact = BigInteger.ONE;
  int rangeEndPrev = 0;
  int sign = 1;
  for (int i = 1; i <= n; i++)
  {
    int rangeEnd = i*i + 1;
    for (int j = rangeEndPrev + 1; j <= rangeEnd; j++)
      fact = fact.multiply(BigInteger.valueOf(j));
    BigDecimal a1 = BigDecimal.valueOf((sign * Math.log(i * x)) / i);
    BigDecimal a  = a1.divide(new BigDecimal(fact), scale, BigDecimal.ROUND_HALF_EVEN);
    System.out.println(df.format(a));
    rangeEndPrev = rangeEnd;
    sign = -sign;
  }
}

How to write code more elegantly? (Factorials, BigDecimals, division by BigIntegers)

1 Answers1