0
\$\begingroup\$

This is a follow up question to 3D bin packing in Java

I finished testing my code's result and started cleaning it up.

Could anyone please tell me what I should do or fix to clean up this code more?

I'm required to put a comment for every instruction line I write. English is not my first language, so maybe some of the comments are not clear enough.

private int ID;
private double length;
private double height;
private double breadth;
private ArrayList<ItemsUnit> packedItems;
private double x, y, z = 0;
private double[] remainingLength;
private double[] remainingHeight;
private double[] remainingBreadth;


/**
 * initialize the remaining dimensions' arrays
 * @param n
 */
public void init(final int n) {

    remainingLength = new double[n];
    remainingHeight = new double[n];
    remainingBreadth = new double[n];

    for (int i = 0; i < n; i++) {

        /*
         * initialize the remainingLength array to the length of the container
         */
        remainingLength[i] = length;

        /*
         * initialize the remainingHeight array to the height of the container
         */
        remainingHeight[i] = height;

        /*
         * initialize the remainingBreadth array to the breadth of the container
         */
        remainingBreadth[i] = breadth;
    }
}


/**
 * put3D method
 * @param item
 * @param p
 * @param n
 * @return
 */
public boolean put3D(final ItemsUnit item, final int p, final int n) {

    /*
     * if x still did not exceed the container's length
     */
    if (x < length) {

        z = remainingHeight[p];     // update z to the position of the new item added
        y = remainingBreadth[p];    // update y to the position of the new item added

        /*
         * if we can put the new item next to the item packed in the extreme point of length
         */
        if (putL(item, p)) {

            packedItems.add(item);  // if item fits add it to the packedItems into the container
            return true;
        }
    }

    /*
     * if x still did not exceed the container's breadth
     */
    if (y < breadth) {

        x = remainingLength[p];     // update x to the position of the new item added
        z = remainingHeight[p];     // update z to the position of the new item added

        /*
         * if we can put the new item next to the item packed in the extreme point of breadth
         */
        if (putB(item, p)) {

            packedItems.add(item);  // if item fits add it to the packedItems into the container
            return true;
        }
    }

    /*
     * if z still did not exceed the container's height
     */
    if (z < height) {

        x = remainingLength[p];     // update x to the position of the new item added
        y = remainingBreadth[p];    // update y to the position of the new item added

        /*
         * if we can put the new item next to the item packed in the extreme point of height
         */
        if (putH(item, p)) {

            packedItems.add(item);  // if item fits add it to the packedItems into the container
            return true;
        }
    }

    return false;   //return false if item cannot be packed in neither extreme point
}


/**
 * adding the new item to the extreme point in length
 * @param item
 * @param p
 * @return
 */
private boolean putL(final ItemsUnit item, final int p) {

    double minRemL = remainingLength[0];     //the minimum remaining length of all already packed items
    int i = 0;  //to store the index of the item next to which we should put the new item

    /*
     * choosing the point (position) where to put the new item
     */
    for (int j = 0; j < remainingLength.length; j++) {

        if ((remainingLength[j] != 0) && (minRemL >= remainingLength[j])
                && (remainingLength[j] >= item.getLength())) {

            i = j;  //storing the item next to which we should put the new packed item
            minRemL = remainingLength[j];   //minimum length left

        }
    }

    remainingLength[p] = remainingLength[i] - item.getLength();   //update the remaining length of the new item added 
    remainingBreadth[p] -= item.getBreadth();   //update the remaining breadth of the new item added
    remainingHeight[p] -= item.getHeight();     //update the remaining height of the new item added
    remainingLength[i] = 0;      //insert 0 to the remainingLength of the item next to which we put the new item (so that we don't consider its remaining length anymore)

    x += item.getLength();  //increment x by the length of the new packed item in the extreme point of length

    return true;
}

/**
 * adding the new item to the extreme point in breadth
 * @param item
 * @param p
 * @return
 */
private boolean putB(final ItemsUnit item, final int p) {

    double minRemB = remainingBreadth[0];    //the minimum remaining breadth of all already packed items
    int i = 0;  //to store the index of the item next to which we should put the new item

    /*
     * choosing the point (position) where to put the new item----
     */
    for (int j = 0; j < remainingBreadth.length; j++) {

        if ((remainingBreadth[j] != 0) && (minRemB >= remainingBreadth[j])
                && (remainingBreadth[j] >= item.getBreadth())) {

            i = j;  //choosing the item to which we should put the new packed item next to
            minRemB = remainingBreadth[j];   //minimum length left

        }
    }

    remainingBreadth[p] = remainingBreadth[i] - item.getBreadth();   //update the remaining breadth of the new item added 
    remainingHeight[p] -= item.getHeight(); //update the remaining height of the new item added
    remainingLength[p] -= item.getLength();     //update the remaining length of the new item added
    remainingBreadth[i] = 0;    //insert 0 to the remainingBreadth of the item next to which we put the new item (so that we don't consider its remaining breadth anymore)

    y += item.getBreadth();     //increment y by the breadth of the new packed item in the extreme point of breadth

    return true;
}


/**
 * adding the new item to the extreme point in height
 * @param item
 * @param p
 * @return
 */
private boolean putH(final ItemsUnit item, final int p) {

    double minRemH = remainingHeight[0];    //the minimum remaining height of all already packed items
    int i = 0;  //to store the index of the item next to which we should put the new item

    /*
     * choosing the point (position) where to put the new item----
     */
    for (int j = 0; j < remainingHeight.length; j++) {

        if ((remainingHeight[j] != 0) && (minRemH >= remainingHeight[j])
                && (remainingHeight[j] >= item.getHeight())) {

            i = j;   //choosing the item to which we should put the new packed item next to
            minRemH = remainingHeight[j]; //minimum length left

        }
    }

    remainingHeight[p] = remainingHeight[i] - item.getHeight();  //update the remaining height of the new item added 
    remainingBreadth[p] -= item.getBreadth();   //update the remaining breadth of the new item added
    remainingLength[p] -= item.getLength();     //update the remaining length of the new item added
    remainingHeight[i] = 0;     //insert 0 to the remainingHeight of the item next to which we put the new item (so that we don't consider its remaining height anymore)

    z += item.getHeight();  //increment z by the height of the new packed item in the extreme point of height

    return true;
}
Improve this question
\$\endgroup\$
8
  • \$\begingroup\$ Could you include the definitions of global/class variables (like x, y, z, height...)? makes it easier to read the code \$\endgroup\$ – Pimgd Apr 6 '16 at 11:36
  • \$\begingroup\$ @Pimgd : done ! I edited the question \$\endgroup\$ – Rii933 Apr 6 '16 at 12:04
  • \$\begingroup\$ (Consider unifying the dimensions: class Dimension { double range, value, remaining[];…} class Whatever3D { int ID; Dimension length, height, breadth; …}. Or at least mapping all those putXYZ() method members to one implementation.) \$\endgroup\$ – greybeard Apr 6 '16 at 12:18
  • 4
    \$\begingroup\$ I have to put a comment for every instruction line I make If every line needs a comment, then it's a sure-fire sign that your code is quite the opposite of understandable/clean. Please ask the person that requested this to kindly bugger off :D \$\endgroup\$ – Arnab Datta Apr 6 '16 at 12:42
  • 1
    \$\begingroup\$ Please state what your code does in the title. \$\endgroup\$ – user34073 Apr 6 '16 at 17:48
6
\$\begingroup\$

Broken Code

Your code is broken right now.

If I make a 2 by 2 by 2 cube.

And I put in it a 3 by 1 by 1 stick.

Then you say "sure, okay!"

/*
 * if x still did not exceed the container's length
 */
if (x < length) {

    z = remainingHeight[p];     // update z to the position of the new item added
    y = remainingBreadth[p];    // update y to the position of the new item added

    /*
     * if we can put the new item next to the item packed in the extreme point of length
     */
    if (putL(item, p)) {

        packedItems.add(item);  // if item fits add it to the packedItems into the container
        return true;
    }

x < length, well, length is 2, x is 0. So that's true.

z and y are set to 0, because that's the spot you're putting the item in.

Then we call putL, which ALWAYS returns true.

So even if the item doesn't fit, you can always put it in.

Taking a closer look at your previous questions, I think you should go back to the code you had in the first question you asked, and make sure that sticking a 3x1x1 stick in a 2x2x2 cube gives you "false".

So when your boss or your teacher asks you to comment every line, there's not much to do about it.

That doesn't take away that comments are usually used to explain "why" something was done in the code. Your teacher asks you to add a comment for each line, so explain "why & how", to show both your thought process and your solution.

Specifically...

/**
 * initialize the remaining dimensions' arrays
 * @param n
 */
public void init(final int n) {

What does n do?

/**
 * put3D method
 * @param item
 * @param p
 * @param n
 * @return
 */
public boolean put3D(final ItemsUnit item, final int p, final int n) {

Or item? p? n again? Is that n the same n or a different one? What does the boolean I get back mean?

Local variable declarations too:

int i = 0;  //to store the index of the item next to which we should put the new item

Don't use i here; write a descriptive variable name!

init in general is flawed. What you should use is a constructor, taking a width, height and length of the container. Then you can set the arrays and such based on those values. Right now, init only allows creating cubes that are the same size in all dimensions... if I'm reading your code correctly.

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ I have a constructor in the class (I forgot to copy it in my question's code) no , cubes (items) are created in another class with different dimensions. I only made the init() function to initialize the remaining dimensions' arrays. for example, what I mean by the remainingLength array is: what remains of the length , from item i till the length of the container. etc... (I don't know if you understand what I mean) \$\endgroup\$ – Rii933 Apr 6 '16 at 12:42
  • \$\begingroup\$ I will do what you said and explain more in the comments. \$\endgroup\$ – Rii933 Apr 6 '16 at 12:43
  • \$\begingroup\$ about the broken code remark, I realized it when I tested so I am fixing it now. Thank you alot. \$\endgroup\$ – Rii933 Apr 6 '16 at 13:35
  • \$\begingroup\$ @Rii933 once it is fixed you should post another followup so that we can actually review it =) \$\endgroup\$ – Pimgd Apr 6 '16 at 19:06

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