HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Condensing code that generates Pascal's triangle

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
generatespascaltrianglecondensingthatcode

Problem

I recently started learning Java for a class and my teacher gave us an assignment in which we had to write up code which would print out the Pascal's triangle to an amount of rows decided by the user, and use recursion to do so. I believe that I'm done with the assignment, but wanted to know, for myself, as to how I could improve and condense this messy code (the array complicates things but I wanted to use it for a different program that expands polynomials (\$(x+b)^n)\$).

import java.io.*;
import java.util.*;

public class PascalTriangleYar
{
private static int[][] xArr;
private static int g =2;

public void printArray(int[][] xArray)
{

  for(int r=0; r<xArray.length; r++){
  System.out.println("");
     for(int c=0; c<xArray[r].length; c++)
        if(xArray[r][c]==0)
           System.out.print("\t");
        else
           System.out.print(xArray[r][c]+"\t");}

}

public int[][] getTri()
{
return xArr;}

public void pTriangle(int r)
{
  int n=r;
  xArr = new int[r][2*r-1];
  xArr[0][r-1]=(1);
  p2T(r, 1);
  printArray(xArr);
}

private void p2T(int r, int n)
{
  int z=0;

  int p=r-g;

    xArr[n][p]=(1);

  for(int fd=0; fd<xArr[0].length-1; fd++){
  if(fd==0)
     xArr[n][fd]= xArr[n-1][0]+xArr[n-1][1];
  else
     xArr[n][fd]= xArr[n-1][fd-1] + xArr[n-1][fd+1];

     xArr[r-1][2*r-2]=1;   

  }

  g++;
  if(n<r-1)
  {
     p2T(r,n+1);
  }
 // else
 // {return;}

}  

public static void main(String [] args)
{
  PascalTriangleYar tri = new PascalTriangleYar();
  Scanner inputReader = new Scanner(System.in);
  System.out.println("Enter a number (x). A pascal's triangle of x rows will be printed.");
  int x = inputReader.nextInt();
  tri.pTriangle(x);
}
}

Solution

Bugs

The variables xArr and g shouldn't be static.
The consequence of them being static is that you cannot use this class twice,
because the static state is not reset per instance.
For example this will cause an ArrayIndexOutOfBoundsException:

new PascalTriangleYar().pTriangle(3);
new PascalTriangleYar().pTriangle(4);


You can fix that by making g non-static.
But you should also make xArr non-static too,
because quite simply, it doesn't need to be static,
and there's no reason whatsoever to make it static.

Condensing the code

Condense code doesn't sound great.
It makes me imagine code with no vertical spaces and no spacing around operators.
That's condense. And that's no good.
Maybe you're looking for a different term.
Maybe want to DRY this up, reducing duplication,
and increasing reusability and modularity.

There's one thing useful to do for "condensing",
is removing unnecessary elements:

  • The getTri method is unused



  • The n variable in pTriangle is unused



  • The z variable in p2T is unused



Another thing for condensing is removing some excessive vertical space.
That is, one blank line between methods is enough,
and there shouldn't be blank lines before the closing braces } of code blocks.

Code duplication

Look very suspiciously at code like this:

xArr[n][p] = 1;

for (int fd = 0; fd < xArr[0].length - 1; fd++) {
    if (fd == 0)
        xArr[n][fd] = xArr[n - 1][0] + xArr[n - 1][1];
    else
        xArr[n][fd] = xArr[n - 1][fd - 1] + xArr[n - 1][fd + 1];

    xArr[r - 1][2 * r - 2] = 1;
}


Notice the duplication in the if-else branches.
Something is fishy there.
What's so special about the fd == 0 case?
On closer look, the case of fd == 0 is already taken care of,
by the xArr[n][p] = 1; statement earlier.
You can change the initialization step of the for loop to int fd = 1,
and the if-else can go away:

for (int fd = 1; fd < xArr[0].length - 1; fd++) {
    xArr[n][fd] = xArr[n - 1][fd - 1] + xArr[n - 1][fd + 1];
    xArr[r - 1][2 * r - 2] = 1;
}


Coding style

As @Hosch already covered many coding style issues I shouldn't repeat that,
but they are actually so important that I want to reemphasize:
please do everything that he suggested,
it will make a huge difference in the readability of your code.
Note that you don't actually have to manually make those corrections by yourself,
any decent IDE (Eclipse, Netbeans, IntelliJ) has shortcuts to do at least most of it for you at the touch of a few keystrokes.

In addition, prefer to use for-each loops instead of counting loops whenever possible. For example printArray can be rewritten as:

public void printArray(int[][] xArray) {
    for (int[] row : xArray) {
        System.out.println();
        for (int item : row) {
            if (item != 0) {
                System.out.print(item);
            }
            System.out.print("\t");
        }
    }
}


Notice some other improvements there:

  • Flipped the item == 0 condition to avoid duplicating System.out.print("\t")



  • No need for the "" in System.out.println("")



Thread safety, robustness

The code as it is, is very fragile.
This is because of the member fields g and xArr.
These fields give instance of your class a state,
and if the pTriangle method gets called by multiple threads at the same time,
they can break each other.

It's ok to have state and have thread-unsafe code when necessary.
But in this program it's not necessary.
You can completely eliminate the g and xArr member fields,
and pass them around as parameters to the methods that need them.
In fact, you don't need g at all, because its value is always n + 1,
so you can always derive it from n.

With these tips, the code can be rewritten as:

public void pTriangle(int r) {
    int[][] xArr = new int[r][2 * r - 1];
    xArr[0][r - 1] = 1;
    p2T(xArr, r, 1);
    printArray(xArr);
}

private void p2T(int[][] xArr, int r, int n) {
    int p = r - n - 1;

    xArr[n][p] = 1;

    for (int fd = 1; fd < xArr[0].length - 1; fd++) {
        xArr[n][fd] = xArr[n - 1][fd - 1] + xArr[n - 1][fd + 1];
        xArr[r - 1][2 * r - 2] = 1;
    }

    if (n < r - 1) {
        p2T(xArr, r, n + 1);
    }
}

Code Snippets

new PascalTriangleYar().pTriangle(3);
new PascalTriangleYar().pTriangle(4);
xArr[n][p] = 1;

for (int fd = 0; fd < xArr[0].length - 1; fd++) {
    if (fd == 0)
        xArr[n][fd] = xArr[n - 1][0] + xArr[n - 1][1];
    else
        xArr[n][fd] = xArr[n - 1][fd - 1] + xArr[n - 1][fd + 1];

    xArr[r - 1][2 * r - 2] = 1;
}
for (int fd = 1; fd < xArr[0].length - 1; fd++) {
    xArr[n][fd] = xArr[n - 1][fd - 1] + xArr[n - 1][fd + 1];
    xArr[r - 1][2 * r - 2] = 1;
}
public void printArray(int[][] xArray) {
    for (int[] row : xArray) {
        System.out.println();
        for (int item : row) {
            if (item != 0) {
                System.out.print(item);
            }
            System.out.print("\t");
        }
    }
}
public void pTriangle(int r) {
    int[][] xArr = new int[r][2 * r - 1];
    xArr[0][r - 1] = 1;
    p2T(xArr, r, 1);
    printArray(xArr);
}

private void p2T(int[][] xArr, int r, int n) {
    int p = r - n - 1;

    xArr[n][p] = 1;

    for (int fd = 1; fd < xArr[0].length - 1; fd++) {
        xArr[n][fd] = xArr[n - 1][fd - 1] + xArr[n - 1][fd + 1];
        xArr[r - 1][2 * r - 2] = 1;
    }

    if (n < r - 1) {
        p2T(xArr, r, n + 1);
    }
}

Context

StackExchange Code Review Q#80551, answer score: 4

Revisions (0)

No revisions yet.