Creating a random item generator in java,to use in a libgdx project

Question

I'm trying to create random items in a libgdx project.I'm relatively new to Java,but here is the code I've come up with for the method. I've been at this for a week now,and figured I'd ask here for an answer. I've been trying to come up with something that works first.So please do forgive the shabby code. The number parameter of the method is the number of items that will be created. The item just needs to have a random x positon,which is generated within the constraints of the width of the container. The game is as bottom up scroller,with different platforms being generated.

private Item[] generateRandomItems(int number){
    Money[] items=new Money[number];

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

        Random r=new Random();

        int x =r.nextInt(120)+3;//136 is the width of the container to which the item is to be generated



        Money tempitem=generateMoney(x);//generateMoney() just returns a new instance of the Money class with the created x passed in as a param.


        if(i!=0) {
            for (int j=0;j<i;j++) {
                boolean failed=true;

                while (failed) {
                    //getItem() returns the bounding rectangle/circle f the item
                    if (!Intersector.overlaps(tempitem.getItem(), items[j].getItem())) {

                        failed = false;

                        items[i] = tempitem;

                    }else{

                        Random random= new Random();
                        int newX=random.nextInt(120)+3;


                        tempitem=generateMoney(newX);
                    }

                }

            }

        }else{

            items[i]=tempitem;

        }



    }

    return items;

}

I don't know if this is a correct way to do it or not,but the created Items do collide sometimes.I've been trying to find what's wrong with the code for sometime now.Any suggestions to improve the code are also appreciated.

Edit::I Know that the code is unnecessarily complicated.This is my first attempt at procedural generation.So please do forgive me.

Answer 1

Thanks for the answers.I now know that checking each generated item for collision without saving the previously generated item is bad. But I got the previous code working after some help,and wanted to share it with anyone who would need it in the future. I moved the checking part into a new method,and added a new flag to see if the item was generated correctly,after checking for collision from all the items before it.

private Item[] generateRandomItems(int number){
Money[] items=new Money[number];

for(int i=0;i<number;i++){
    Random r=new Random();
    int x =r.nextInt(120)+3;
    Money tempitem=generateMoney(x);

    if(i>0) {
        boolean generated=false;

        while (!generated) {
            boolean f = checkIfItemOverlapsWithPrevious(items, tempitem);

            if (!f) {
                items[i] = tempitem;
                generated = true;
            } else {
                Random random = new Random();
                int newX = random.nextInt(120) + 3;
                System.out.println("Collided");
                tempitem = generateMoney(newX);
            }
        }

    }else{
        items[i]=tempitem;
    }
}
return items;
}

private boolean checkIfItemOverlapsWithPrevious(Money[] items, Money tempitem) {
for(Money item :items){

    if(item!=null) {
        if (Intersector.overlaps(tempitem.getItem(), item.getItem())) {
            return true;
        }
    }
}
return false;

}

Answer 2

Instead of generating a new random position if there is a collision, you should move it deliberately left or right until there is no collision. To illustrate the problem reusing random generation after each collision, if you have 10 slots and 9 slots are already filled, it could take a long time to find that open slot using random generation as you would be almost certain to hit the same object numerous times. However, if you keep track of where you’ve checked and deliberately move to a new location each time, then the worst case scenario is you’d hit each object one time before finding the empty slot.

You can check how much of an overlap there is and move the object by that amount to clear the object, then check to make sure it didn’t collide with another object next to it, if it did then keep moving it over until there is a free spot. If you hit the edge of the screen, move to the opposite side and keep moving until you find a free spot.

Also, as good coding practice you should avoid hard coding numbers (like 120+3) into method calls. Since you use the same value in multiple places, if you decide to change the width of your container to 500, then you have to change it in all those places...and if you forget to change one you’re opening yourself up for a nightmare of bug hunting. Instead you can either set an integer such as containerWidth=120 or set the container width directly using container.setWidth(120) and then use container.getWidth() each time you call your random method to get the width of the container the random value is being constrained too. Either way will work fine, whichever is better for your workflow. And I know you said this was quick and sloppy code just to get it going, so you may already be aware of this.