6
\$\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);
    }
}
\$\endgroup\$

3 Answers 3

3
\$\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
\$\endgroup\$
3
\$\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.

\$\endgroup\$
2
\$\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.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.