Condensing code that generates Pascal's triangle

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); } } 

There is a lot that can be improved in this code. First and foremost - please use longer, descriptive variable names - p doesn't really tell you anything. The only time I would say it is acceptable to use a single-letter variable name is the traditional i and c variables used like this:

for (int i = 0; i < val; i++) for (char c : string) 

Second, please indent your code following the standard indentation levels of (usually) 4 spaces per indent level, with the inside of classes, methods, loops, if statements, and a few other instances adding one indentation level.

Third, be consistent with your styles. Right here, you use braces around a one-statement if block:

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

And here, you do not:

if(fd==0) xArr[n][fd]= xArr[n-1][0]+xArr[n-1][1]; 

I recommend you always use braces - this applies to loops too.

Fourth, you should use spaces around operators:

if(fd == 0) 

Fifth, again use consistent styling, and do not wrap braces around content like this:

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

That should be like this:

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

Traditionally, you use i and j for nested for loops:

 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");} 

Also, use the correct indentation, fix your braces, and you should add braces to your if/else block like this:

if (condition) { // if statements... } else { // else statements... } 

This follows standard Java brace formatting, although some programmers place the else on a separate line.

I am sure your code can be improved in its execution as well, but these are more pressing issues for now - make your code easy to read, and you will catch more bugs and be able to improve it on your own, as well as getting reviews on content instead of style.

I think you may have slightly misunderstood the assignment given to you: the point is to print the triangle recursively, i.e. print each line of the triangle as it is calculated, without keeping any state (state management is a major concern in recursive algorithms).

In your case, you should apply the recursion on the rows of the triangle, as a row should be constructed thanks to the previous row.

import java.util.Arrays; import java.util.Scanner; public class PascalTriangle { public static void main(String[] args) { System.out.println("Enter a positive integer (x). A Pascal's triangle of x rows will be printed."); int numRows = new Scanner(System.in).nextInt(); new PascalTriangle().generate(numRows); } public int[] generate(int numRows) { int[] result = new int[numRows]; result[0] = 1; if (numRows > 1) { int[] previousRow = generate(numRows - 1); for (int ii = 1; ii < numRows - 1; ii++) { result[ii] = previousRow[ii - 1] + previousRow[ii]; } result[numRows - 1] = 1; } System.out.println(Arrays.toString(result)); return result; } } 

This code is probably acceptable as a solution to the problem you have provided. From a professional point of view, it is still not satisfactory, because the recursive function mixes two concerns (calculating the line and printing it). One should thus refactor it by passing a consumer to be applied on the intermediate result (but that would be off-topic here).

Others have made quite useful comments regarding your code. If you like, spend some time and read about Kent Beck's four rules of simple design. In my opinion, these 4 rules are essential to write good code.