2
\$\begingroup\$

First time doing program documentation, how is it? Any thing that was done good or poorly? What can I work on to make it better if need be?

/** primary class for MultiplicationTable */
public class MultiplicationTable {

/** This method is used to execute the program */
public static void main(String[] args) {

    /** variables to store our row(rowNum) and column(colNum) numbers */
    int rowNum, colNum;

    /** intializing variables with their respected values, in this case its 12 */
    rowNum = 12;
    colNum = 12;

    /** displays the top left blank space, so we don't have to have a '0' there */
    System.out.print("     |");

    /** the loop will go 12 times, horizontal and then end once at 12 the top row */
    for( rowNum = 1; rowNum <= 12; rowNum++) {

        /** displays our numbers on the top row (going horizontal) with format(spacing) in between each number */
        System.out.printf("%4d |", rowNum);
    
    } /** end of first for loop */

    /** carriage return so theres a line between row 1 and row 2 so it is seperated equally throughout the program */
    System.out.print("\n");
    
    /** the loop will go for 12 times, vertical and then end once at 12 on the leftmost column */
    for( rowNum = 1; rowNum <= 12; rowNum++) {

        /** displays out numbers on the leftmost column, (going vertical) with format(spacing) in between each number */
        System.out.println();
        System.out.printf("%4d |", rowNum);

        /**  the loop will go for 12 times, horizontal and then ending once at 12 for each row*/
        for( colNum = 1; colNum <= 12; colNum++) {

            /** displays our calculations of colNum*rowNum and with format(spacing) */
            System.out.printf("%4d |", colNum*rowNum);
        
        } /** ends the third for loop, (inside the second for loop) */

        /** carriage return after the rightmost column, allowing us to go to the next row */
        System.out.println();

    } /** ends the second for loop*/

} /** end of public static void main(String[] args) */

} /** end of public class TempChart */

Just want to know if I can change anything!

\$\endgroup\$
3
  • 11
    \$\begingroup\$ It seems you've confused documentation for the program with an explanation of how the programming language works. Why is there no description of the purpose of the program itself? \$\endgroup\$
    – Mast
    Commented Nov 2, 2022 at 6:08
  • 8
    \$\begingroup\$ Documentation is not meant to tell users what every line does. That should be evident from the code itself. As it is now, there's waaay too much documentation. I can't read the actual code because it all blends together. I would remove every single comment except the one that documents the class. This one should describe the purpose of the class, maybe what users should/shouldn't expect from it, how to use it etc... What you can do is to use descriptive variable names, extract functionality into methods with descriptive names. Methods are another case where documentation is useful. \$\endgroup\$
    – QBrute
    Commented Nov 2, 2022 at 9:38
  • 5
    \$\begingroup\$ @QBrute you can post this comment as an answer. \$\endgroup\$ Commented Nov 2, 2022 at 10:28

3 Answers 3

9
\$\begingroup\$

First of all, read this: Code Conventions for the Java Programming Language: Comments

Code already tells what it does. You should not repeat that in the comments. Comments should tell why the code does something when it is not evident from the code itself. For example, you should document why you chose number 12. Most of your comments are unnecessary and clutter the code, making reading it very hard. That's why I will not bother looking at the code at all now.

Comments like /** ends the second for loop*/ are an indication that your method is too long and/or your code is formatted badly. If you need to remind yourself what the corresponding block opener is, you should fix the issue that is causing the problem. Code blocks should be so short that you can easily see the matching opening and closing brackets. So either fix the formatting and indentation or split the code block into methods. Removing the unnecessary comments probably would be enough here.

The /** format makes sense only for fields, methods and classes. It is used to format standard JavaDoc documentation. Using it for commenting code is useless. In code you should use /* Comment. */ or // End of line comment.

\$\endgroup\$
6
\$\begingroup\$

For now, let's not worry about efficiency and start by addressing your main question.

As TorbenPutkonen points out, you have missed the point of documenting your code - you should document what you are doing and why you are doing it, not how you are doing it, as the code itself shows how.

Much of the clarity of your code is lost by the plethora of unnecessary comments - and as already noted, these should generally not be introduced with "/**" as they are not helpful for Javadoc.

Communicating what the code is doing is often most helpfully done by breaking the code down into methods with meaningful names. Here's how I might restructure your code on that basis.

(You'd probably find some of Robert Martin's ("Uncle Bob's") writings helpful, notably his book on "Clean Code" - the approach I've taken below is very much in line with his approach to restructuring.)

Note also that your handling of rowNum and colNum (bad names in Java) was messy and inconsistent (you initialise them to 12, but immediately reassign them), so I've taken a different approach.

/**
 * Class to print a multiplication table
 */
public class MultiplicationTable {

  /**
   * Print a multiplication table, up to 12 * 12 (defined by MAX_NUMBER)
   * @param args Currently unused
   */
  public static void main(String[] args) {

    /*
     * Table size - table is square
     * with same number of rows as columns
     *
     * TODO:
     * 1) take this as a command line parameter
     * 2) allow for differing numbers of rows versus columns
     *
     */
    final int MAX_NUMBER = 12;

    printHeader(MAX_NUMBER);

    printBody(MAX_NUMBER);

  }

  /**
   * Print header for multiplication table
   *
   * @param numberOfColumns
   */
  private static void printHeader(int numberOfColumns) {
    System.out.print("     |");
    for (int column = 1; column <= numberOfColumns; column++) {
      System.out.printf("%4d |", column);
    }
    System.out.print("\n");
  }

  /**
   * print body for multiplication table
   * (assumed square - i.e. same number of rows and columns)
   * @param maxNumber - highest row or column number
   */
  private static void printBody(int maxNumber) {
    for (int row = 1; row <= maxNumber; row++) {
      printRowSeparator(maxNumber);

      printRowLabel(row);

      printProducts(row, maxNumber);

      System.out.println();

    }
  }

  /**
   * Print a separating line between rows of the table
   * @param numberOfColumns
   */
  private static void printRowSeparator(int numberOfColumns) {
    for (int column = 0; column <= numberOfColumns; column++) {
      System.out.print("-----+");
    }
    System.out.println();
  }

  /**
   * Print the products of row and column numbers for this row
   * @param row
   * @param numberOfColumns
   */
  private static void printProducts(int row, int numberOfColumns) {
    for (int column = 1; column <= numberOfColumns; column++) {
      System.out.printf("%4d |", column * row);
    }
  }

  /**
   * Print a label for the row
   * @param row
   */
  private static void printRowLabel(int row) {
    System.out.printf("%4d |", row);
  }

}
\$\endgroup\$
2
  • \$\begingroup\$ Would tableSize be a more descriptive name than maxNumber? \$\endgroup\$ Commented Nov 3, 2022 at 5:31
  • \$\begingroup\$ Probably, until we allow for non-square tables... \$\endgroup\$ Commented Nov 3, 2022 at 7:12
1
\$\begingroup\$

Efficiency

You keep calling System.out.print all over again and again, which is not so efficient. Why not create a class that builds the entire output string and override the toString(), putting the object into a System.out.println() once. Like so:


public class MultiplicationTable {
    
    private static final String CELL_FORMAT = "%4d |";
    
    private final int bound;
    private String textualRepresentation;
    
    public MultiplicationTable(int bound) {
        checkBound(bound);
        this.bound = bound;
    }
    
    @Override
    public String toString() {
        if (textualRepresentation == null) {
            textualRepresentation = createTextualRepresentation(bound);
        }
        
        return textualRepresentation;
    }
    
    private static void checkBound(int bound) {
        if (bound < 1) {
            throw new IllegalArgumentException(
                    "Bound is too small: " + bound + ". Must be at least 1.");
        }
    }
    
    private static String createTextualRepresentation(int bound) {
        StringBuilder sb = new StringBuilder();
        loadHeader(sb, bound);
        boolean first = true;
        
        for (int y = 1; y <= bound; y++) {
            if (first) {
                first = false;
            } else {
                sb.append('\n');
            }
            
            loadRowY(sb, y, bound);
        }
        
        return sb.toString();
    }
    
    private static void loadHeader(StringBuilder sb, int bound) {
        sb.append("     |");
        
        for (int x = 1; x <= bound; x++) {
            sb.append(String.format(CELL_FORMAT, x));
        }
        
        sb.append('\n');
    }
    
    private static void loadRowY(StringBuilder sb, int y, int bound) {
        sb.append(String.format(CELL_FORMAT, y));
        
        for (int x = 1; x <= bound; x++) {
            sb.append(String.format(CELL_FORMAT, x * y));
        }
    }
}

class Demo {
    
    public static void main(String[] args) {
        System.out.println(new MultiplicationTable(12));
    }
}
\$\endgroup\$
4
  • 4
    \$\begingroup\$ Some of this I like, notably breaking it down into small methods with reasonable names. However, I'm not convinced the lazy loading of the table is of value - the only point of the table is to be populated and printed, after all. Also, for the specific issue we're looking to address here - documentation was what was asked about - fussing about efficiency and adding the use of a StringBuilder (though it makes some sense) is probably muddying the waters. \$\endgroup\$ Commented Nov 2, 2022 at 12:41
  • 3
    \$\begingroup\$ @MarkBluemel ish. CodeReview does not expect or require single-issue answers. Any meaningful observation is on-topic. So yes to the questionable lazy loading, but no to muddy waters. \$\endgroup\$
    – Reinderien
    Commented Nov 2, 2022 at 12:44
  • \$\begingroup\$ @Reinderien Fair enough - thank you for the clarification. \$\endgroup\$ Commented Nov 2, 2022 at 13:24
  • 4
    \$\begingroup\$ In addition to Mark's remarks, I would say all of these static methods should be instance methods, sharing the bound field (and perhaps sb should be a field as well of some short-lived object, though I could understand some argument) instead of pointlessly passing the same values everywhere. And I think it's pretty silly to override toString - that's for debugging. Just define a differently-named method that returns a String. \$\endgroup\$
    – amalloy
    Commented Nov 2, 2022 at 15:28

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.