patternjavaMinor
Condensing code that generates Pascal's triangle
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
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
You can fix that by making
But you should also make
because quite simply, it doesn't need to be
and there's no reason whatsoever to make it
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:
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
Code duplication
Look very suspiciously at code like this:
Notice the duplication in the if-else branches.
Something is fishy there.
What's so special about the
On closer look, the case of
by the
You can change the initialization step of the for loop to
and the if-else can go away:
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
Notice some other improvements there:
Thread safety, robustness
The code as it is, is very fragile.
This is because of the member fields
These fields give instance of your class a state,
and if the
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
and pass them around as parameters to the methods that need them.
In fact, you don't need
so you can always derive it from
With these tips, the code can be rewritten as:
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
getTrimethod is unused
- The
nvariable inpTriangleis unused
- The
zvariable inp2Tis 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 == 0condition to avoid duplicatingSystem.out.print("\t")
- No need for the
""inSystem.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.