3
\$\begingroup\$

Puzzle description: input is dimensions (length, width, heigth) of various presents.

Example:

2x3x1  
5x9x10  
1x40x2

Part 1: determine the area of paper needed to wrap the gifts, wich is equal to 2*(l*w + w*h + l*h) + the smallest between l*w, w*h and l*h for each gift.

Part 2: determine the length of ribbon needed to wrap the gift, wich is equal to l*w*h + smallest perimeter for each gift.

I learned about aoc recently and decided to start from 2015. Below is my code from the day 2 (problem link). How can I improve it? Is it too long?

import java.io.*;
import java.util.*;

public class adventofcode2 {
    public static void main(String[] args){
        System.out.println("input: ");
        Scanner input = new Scanner(System.in);
        String line;
        String text = "";
        while (!(line = input.nextLine()).isEmpty()){
            text += line + "\n";
        }
        calcTotal(text);
        System.out.println("Paper area used -  " + TotalPaper);
        System.out.println("Ribbon length used -  " + TotalRibbon);
    }
    /**
     * 1 -  Calculates the area of paper necessary to make the presents wrapping
     * 2 -  Calculates the length of ribbon necessary to the wrapping
     * @param presentsText list of strings in the style numberxnumberxnumber
     */
    private static void calcTotal(String presentsText){
        String[] presents = presentsText.split("\n");
        for (String present : presents){
            calcEachPresent(present.split("x"));
        }
    }
    /**
     * Iteration inside the calcTotal method
     * @param presentText array of each present string written in 
     * the style numberxnumberxnumber
     */
    private static void calcEachPresent(String[] presentText){
        //Definition of the three dimensions of the present.
        int l = Integer.parseInt(presentText[0]), w = Integer.parseInt(presentText[1]), h = Integer.parseInt(presentText[2]);

        //Definition of the area of the sides in the present.
        int[] d = {l*w,w*h,h*l};

        int min = getMin(d);

        int area = 2*(d[0] + d[1] + d[2]) + min;

        int ribbon = (l*w*h) + smallestPerimeter(l,w,h);
        TotalPaper += area;
        TotalRibbon += ribbon;
    }
    /**Get the smallest area to use in the area calculation
     * 
     * @param num array of numbers
     * @return smallest number in an array
     */
    private static int getMin(int[] num){
        int min = num[0];
        for (int n : num){
            if(n < min) min = n;
        }
        return min;
    }
    /**Get smallest perimeter to use in ribbon length calculation
     * 
     * @param l length of present
     * @param w width of present
     * @param h heigth of present
     * @return smallest perimeter
     */
    private static int smallestPerimeter(int l,int w,int h){

        int[] num = {l,w,h};
        Arrays.sort(num);

        return 2*(num[0] + num[1]);
    }

    /**
    *   VARIABLES TO RETURN IN THE MAIN METHOD
    */
    private static int TotalPaper = 0;
    private static int TotalRibbon = 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Could you please improve the title of your question to make that more generic? \$\endgroup\$ – πάντα ῥεῖ Dec 30 '16 at 21:12
3
\$\begingroup\$

The structure of your code looks quite good so far but you could improve readability:

Naming Conventions

As with almost every other programming language java has a schema of how to name identifiers that's common consensus You can read the details here: http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-135099.html

The main line is: classes start with an uppercase letter, method and variable names start with lower case letter. And if the name consists of multiple words use CamelCase.

variable declarations

You declared 2 variables ab the end of your class file. The common consensus in Java is to write class (or object) scope variables at the top of the class file.

less comments ##

there should be 2 types of comments in your code:

  1. Description of the contract of an interface method
    this type of comment should guide the implementer of that method and tell what the caller of the method expects, especially what the properties of the parameters and the result are like.

  2. Explanations why the code is like it is
    this should tell the reader why you chose a certain approach.

There should not be any other comment in your code.

Whenever you feel the need to write a comment try to find better identifier names first.

Especially comments like this are not needed instead they clutter the code:

 /**Get smallest perimeter to use in ribbon length calculation
     * 
     * @param l length of present
     * @param w width of present
     * @param h heigth of present
     * @return smallest perimeter
     */
    private static int smallestPerimeter(int l,int w,int h){

Instead the code could be like this:

private static int getSmallestPerimeterToUseInRibbonLengthCalculation(int length, int width, int heigth){

You may argue that the name is to long and you're right: the method should only tell what it does and not restrict the possible callers, so it should better be:

private static int getSmallestPerimeter(int length, int width, int heigth){

And get is not correct since this method is not a getter in the sense that the content of an object (or class) variable is returned, it calculates the returned value and the name should reflect that:

private static int calculateSmallestPerimeter(int length, int width, int heigth){

You use comments to structure a method:

  //Definition of the three dimensions of the present.
        int l = Integer.parseInt(presentText[0]), w = Integer.parseInt(presentText[1]), h = Integer.parseInt(presentText[2]);

  //Definition of the area of the sides in the present.

In such cases you should better extract that code between the comments into separate methods or objects with names derived from the comments:

PresentDimensions.java

  class PresentDimensions{
    final int length, height, width;
    PresentDimensions(int length, int width, int height){
       this.length  =  length;
       this.width  =  width;
       this.height  =  height;
    }
    public int[] calculateSurfaceAreas(){
       return new int[]{length*width,width*height,height*length}
    }

in calcEachPresent

 PresentDimensions presentDimensions = new PresentDimensions(presentText[0], presentText[1], presentText[2]);
 int[] d = presentDimensions.calculateSurfaceAreas();
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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