7
\$\begingroup\$

I created a Pascal's triangle. I'm in 2nd semester, so I'm not that professional in programming. If there are some suggestions for improvement, please tell (took me 2 & 1/2 hrs to find the logic).

//Pascals Triangle
package logicBuilding;
import java.util.*;
public class PascalsTriangle {
 static void print(int[] arr,int r) {
 int size=2,t=1,n=0;
 int [] arr1=null;
 int temp1=1,temp2=1,sizeArr1=2;
 for (int i=0;i<r;i++) {
 for (int k = r - i - 1; k > 0; k--) {
 System.out.print(" ");
 }
 for (int j = 0; j <= i; j++) {
 if (i == 0) {
 System.out.print(arr[i]);
 break;
 } else if (i == 1) {
 System.out.print(arr[i - 1] + " " + arr[i]);
 break;
 }
 System.out.print(arr[j] + " ");
 }
 if(i > 2) {
 arr1 = new int[sizeArr1];
 for(int a=1;a<size-1;a++) {
 arr1[n]=arr[a];
 n++;
 }
 n=0;
 }
 if (i > 0) {
 arr = new int[++size];
 arr[1] = ++temp1;
 arr[size - 2] = ++temp2;
 arr[size - 1] = 1;
 arr[0] = 1;
 }
 if (i > 2) {
 for (int g = 0; g < sizeArr1; g++) {
 if (t < sizeArr1) {
 arr1[g] += arr1[t];
 t++;
 } else {
 break;
 }
 }
 int w = 0;
 while (w < sizeArr1) {
 for (int h = 2; h < size - 2; h++) {
 arr[h] = arr1[w];
 w++;
 }
 break;
 }
 t = 1;
 sizeArr1++;
 }
 System.out.println("");
 }
 }
 public static void main(String[] args) {
 Scanner s1=new Scanner(System.in);
 int [] arr;
 int r;
 System.out.println("Enter the number of rows you want : ");
 r=s1.nextInt();
 s1.nextLine();
 arr=new int[2];
 arr[0]=1;
 arr[1]=1;
 print(arr,r);
 }
}
Reinderien
70.9k5 gold badges76 silver badges256 bronze badges
asked Feb 11, 2024 at 14:39
\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

Avoid importing * - it pollutes your namespace.

The structure of the program is not very Java-idiomatic. The typical Java pattern would be to instantiate an object that represents a triangle of a certain size.

You should separate your calculation code from your rendering (print()) code.

In lines like print(arr[i - 1] + " " + arr[i]), you should avoid concatenation and make better use of formatting.

Consider:

  • Making an object instance with a final nrows to represent your triangle
  • Making an iterator object to generate your rows
  • Making a render method that accepts a PrintStream

Suggested

package com.stackexchange;
import java.io.PrintStream;
import java.util.Arrays;
import java.util.Iterator;
import java.util.Scanner;
import java.util.stream.Collectors;
public class PascalsTriangle {
 public class RowIterator implements Iterator<int[]>, Iterable<int[]> {
 private int n = 0;
 @Override
 public Iterator<int[]> iterator() {
 return new RowIterator();
 }
 @Override
 public boolean hasNext() {
 return n < nrows;
 }
 @Override
 public int[] next() {
 int[] row = new int[n + 1];
 int nck = 1;
 for (int k = 0; k <= n; ++k) {
 row[k] = nck;
 nck = nck*(n - k)/(k + 1);
 }
 ++n;
 return row;
 }
 }
 public final int nrows;
 public static int element(int n, int k) {
 // n! / k!(n - k)!
 int x = 1;
 for (int i = 1+k; i <= n; ++i)
 x *= i;
 for (int i = n-k; i > 1; --i)
 x /= i;
 return x;
 }
 public int maxElement() {
 return element(nrows - 1, nrows/2);
 }
 public PascalsTriangle(int nrows) {
 this.nrows = nrows;
 }
 public Iterable<int[]> rows() {
 return new RowIterator();
 }
 public void render(PrintStream out) {
 int width = 1 + (int)Math.floor(Math.log10(maxElement()));
 String format = "%%%dd".formatted(width);
 for (int[] row: rows()) {
 String rowstr = Arrays.stream(row)
 .mapToObj(format::formatted)
 .collect(Collectors.joining(" "));
 out.print(" ".repeat(
 ((width+1)*nrows - rowstr.length())/2
 ));
 out.println(rowstr);
 }
 }
 public static PascalsTriangle fromConsole() {
 Scanner scanner = new Scanner(System.in);
 System.out.print("Enter the number of rows you want: ");
 int r = scanner.nextInt();
 scanner.nextLine();
 return new PascalsTriangle(r);
 }
 public static void main(String[] args) {
 PascalsTriangle tri = PascalsTriangle.fromConsole();
 tri.render(System.out);
 }
}
Enter the number of rows you want: 18
 1
 1 1
 1 2 1
 1 3 3 1
 1 4 6 4 1
 1 5 10 10 5 1
 1 6 15 20 15 6 1
 1 7 21 35 35 21 7 1
 1 8 28 56 70 56 28 8 1
 1 9 36 84 126 126 84 36 9 1
 1 10 45 120 210 252 210 120 45 10 1
 1 11 55 165 330 462 462 330 165 55 11 1
 1 12 66 220 495 792 924 792 495 220 66 12 1
 1 13 78 286 715 1287 1716 1716 1287 715 286 78 13 1
 1 14 91 364 1001 2002 3003 3432 3003 2002 1001 364 91 14 1
 1 15 105 455 1365 3003 5005 6435 6435 5005 3003 1365 455 105 15 1
 1 16 120 560 1820 4368 8008 11440 12870 11440 8008 4368 1820 560 120 16 1
 1 17 136 680 2380 6188 12376 19448 24310 24310 19448 12376 6188 2380 680 136 17 1
answered Feb 11, 2024 at 22:33
\$\endgroup\$
4
\$\begingroup\$

cite your reference

(took me 2 & 1/2 hrs to find the logic)

Please don't make your reviewers and maintenance engineers go hunting for the reference you eventually found. Include the URL in the source code as a comment.

Hopefully there is some correspondence between the identifiers you're using and what appears in that reference.

javadoc

There isn't any.

Please attach at least a one-sentence /** description */ to the PascalsTriangle class.

main

This is an OK local variable name.

 Scanner s1 = new Scanner(System.in);

It suggests there's a second Scanner somewhere, but there isn't. Consider using letters instead, one of {sc, scn, scnr, scanner}.

design of API

This function is well named. It's pretty clear that we shall evaluate it for side effects.

 static void print(int[] arr, int r) {

It is unclear why caller is responsible for passing in a (1, 1) tuple. Shouldn't this routine be responsible for creating arr? If there is some design rationale, write a comment describing it.

The identifier r is manifestly the wrong name. It's not like this is a local variable, so the documentation burden is much greater. You are responsible for explaining the meaning of this parameter to an engineer who is calling into this routine. Name it something like numRows.

local variables

 int size=2, t=1, n=0;
 int [] arr1=null;
 int temp1=1, temp2=1, sizeArr1=2;

The i, j are fine. Otherwise these are not terrific name choices. Imagine you have to return to this code in six months and maintain it. You will wish you were staring at more helpful names.

It's unclear why inventing the concept of sizeArr1 is even helpful; we could have just allocated a (non-null) integer array of size 2 and then relied on arr1.length.

I suspect that the concept modeled by some of these is actually i, that is, currentRowNum.

Try to use identifiers drawn from the Business Domain when feasible. Here, I was expecting to maybe see a coeff vector full of numRows binomial coefficients. Not sure if arr or arr1 is supposed to play that role.

Inventing local temp vars like temp1 and temp2 can be fine. But strive to keep the scope so small that they're self-explanatory. Here, they're set to unity, and then you're asking a maintenance engineer to keep those cryptic numbers in mind until way down below. The bigger the scope, the greater your responsibility to explain what's going on, typically via an informative name.

extract helper

 for (int k = r - i - 1; k > 0; k--) {
 System.out.print(" ");
 }

Aha! Now here is some wonderful code. It's clear what it does.

But this loop appears alongside some bigger loops, and it's a little distracting. Replacing it with a single-line System.out.print(indent(numRows - i - 1)) would be nicer. And then that helper could use StringBuilder or whatever.

display of each numeric cell

You might want to take advantage of the ^ caret formatting character, to help with centering each displayed coefficient within a fixed-width cell. In the current code, log_10 of a coefficient influences the horizontal layout.

murky logic

 for (int j = 0; j <= i; j++) {
 if (i == 0) { ...
 } else if (i == 1) { ...
 }
 if (i > 2) { ...

It's unclear why the first two rows should be so special. And then we encounter the general case, but oddly it is outside the loop. Consider turning that for j loop into a helper, just so you can name it, explaining what it accomplishes.

Yes, I know this is a "print" function. But I'm a little surprised it doesn't first produce an array containing a prevRow or currRow of binomial coefficients, ideally by calling a helper. With the array in hand, the print() routine could focus on its single responsibility of formatting those numbers on stdout.

loop index

 for (int g = 0; g < sizeArr1; g++) {

We strayed from i, j, ..., that's fine. But now I'm scratching my head what g might possibly stand for. Either explain it, or stick with boring index names.

Consider extracting this for loop as a tiny helper, which would give you the opportunity to name what it does.

At least with h & w we get the clue to mentally pronounce them as "height" & "width" of some diagram that appears in the cited reference.

unit tests

There aren't any, partly due to the lack of any functional helpers. A function that is focused on displaying a result, rather than returning a result object, tends to be one that isn't a great fit for unit tests.

Writing automated tests can save on edit-debug cycle time. But it can also save time spent developing the larger project, by encouraging us to break big problems into smaller ones that are easy to describe to colleagues.


This code appears to achieve its design goal.

I would not be willing to delegate or accept maintenance tasks on this code in its current form.

answered Feb 11, 2024 at 17:56
\$\endgroup\$
3
\$\begingroup\$

Code reviewed from a non-math perspective with just code comments.

Efficiency and software development principles (e.g. single responsibility principle a.k.a SRP)

//Pascals Triangle
package logicBuilding;
import java.util.*;
public class Triangle {
 static void print(int r) { // pass just the number of lines to print method anything else is internal logic of print method
 int[] arr = { 1, 1 }; // use inline array initialization
 int size = 2, t = 1, n = 0;
 int[] arr1 = null;
 int temp1 = 1, temp2 = 1, sizeArr1 = 2;
 for (int i = 0; i < r; i++) {
 for (int k = r - i - 1; k > 0; k--) {
 System.out.print(" ");
 }
 for (int j = 0; j <= i; j++) {
 if (i == 0) { // efficiency wise 
 System.out.print(arr[i]);
 break; 
 } else if (i == 1) {
 System.out.print(arr[0] + " " + arr[1]); // i = 1 use static values for array index, 0 and 1
 break;
 }
 System.out.print(arr[j] + " ");
 }
 if (i > 2) {
 arr1 = new int[sizeArr1];
 for (int a = 1; a < size - 1; a++) {
 arr1[n] = arr[a];
 n++;
 }
 n = 0;
 }
 if (i > 0) {
 arr = new int[++size];
 arr[1] = ++temp1;
 arr[size - 2] = ++temp2;
 arr[size - 1] = 1;
 arr[0] = 1;
 }
 if (i > 2) {
 for (int g = 0; g < sizeArr1; g++) {
 if (t < sizeArr1) {
 arr1[g] += arr1[t];
 t++;
 } else {
 break;
 }
 }
 int w = 0;
 while (w < sizeArr1) {
 for (int h = 2; h < size - 2; h++) {
 arr[h] = arr1[w];
 w++;
 }
 break;
 }
 t = 1;
 sizeArr1++;
 }
 System.out.println("");
 }
 }
 public static void main(String[] args) {
 Scanner s1 = new Scanner(System.in);
 System.out.println("Enter the number of rows you want : ");
 int r = s1.nextInt(); // use inline variable declaration initialization
 s1.close(); // close the scanner
 print(r);
 }
}

For improved readability use meaningful variable names

//Pascals Triangle
package logicBuilding;
import java.util.*;
public class Triangle {
 static void print(int rowsNumber) { // rowsNumber than r 
 int[] row = { 1, 1 }; // row than arr
 int rowLength = 2, t = 1, n = 0;
 int[] arr1 = null;
 int temp1 = 1, temp2 = 1, sizeArr1 = 2;
 for (int rowNumber = 0; rowNumber < rowsNumber; rowNumber++) { // rowNumber than i
 for (int column = rowsNumber - rowNumber - 1; column > 0; column--) { // column than k
 System.out.print(" ");
 }
 for (int j = 0; j <= rowNumber; j++) {
 if (rowNumber == 0) { 
 System.out.print(row[rowNumber]);
 break; 
 } else if (rowNumber == 1) {
 System.out.print(row[0] + " " + row[1]);
 break;
 }
 System.out.print(row[j] + " ");
 }
 if (rowNumber > 2) {
 arr1 = new int[sizeArr1];
 for (int a = 1; a < rowLength - 1; a++) {
 arr1[n] = row[a];
 n++;
 }
 n = 0;
 }
 if (rowNumber > 0) {
 row = new int[++rowLength];
 row[1] = ++temp1;
 row[rowLength - 2] = ++temp2;
 row[rowLength - 1] = 1;
 row[0] = 1;
 }
 if (rowNumber > 2) {
 for (int g = 0; g < sizeArr1; g++) {
 if (t < sizeArr1) {
 arr1[g] += arr1[t];
 t++;
 } else {
 break;
 }
 }
 int w = 0;
 while (w < sizeArr1) {
 for (int h = 2; h < rowLength - 2; h++) {
 row[h] = arr1[w];
 w++;
 }
 break;
 }
 t = 1;
 sizeArr1++;
 }
 System.out.println("");
 }
 }
 public static void main(String[] args) {
 Scanner input = new Scanner(System.in); // input than s1
 System.out.println("Enter the number of rows you want: ");
 int rowsNumber = input.nextInt(); // rowsNumber than r
 input.close();
 print(rowsNumber);
 }
}

This review is a hint that in case is helpful it should be scaled to cover the entire code.

answered Feb 14, 2024 at 10:29
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.