CODEWITHSUNDEEP
java
Sean C
Sean C

Reputation: 43

Checking for duplicates and moving to the next element

I have a simple program consisting of a String array and a String arraylist. The program randomly chooses a String from the array and checks if that String also exists in the arraylist. If it does, then a different String is chosen. Right now the program runs fine, but I'm not sure how efficient my code is, since I have the same line of code running twice - which I think is bad practice.

`import java.util.ArrayList;
import java.util.List;
import java.util.Random;

public class Main {

public static String[] rooms1 = new String[] {
        "Library",
        "Kitchen",
        "Study",
        "Conservatory",
        "Ballroom",
        "Lounge",
        "Hall",
        "Billiard Room",
        "Dining Room"
    };

public static List<String> rooms2 = new ArrayList<String>();

public static void addRooms() {
    rooms2.add("Garage");
    rooms2.add("Bar");
    rooms2.add("Library");
    rooms2.add("Bathroom");
    rooms2.add("Lounge");
    rooms2.add("Dining Room");
}

public static void main(String[] args) {
    addRooms();
    Random random = new Random();
    String roomChosen = rooms1[random.nextInt(rooms1.length)];
    while (rooms2.contains(roomChosen))
        roomChosen = rooms1[random.nextInt(rooms1.length)];
    System.out.println("Player has entered the " + roomChosen);
}
}

`

The line of code that repeats itself is roomChosen = rooms1[random.nextInt(rooms1.length)];

Upvotes: 1

Views: 51

Answers (2)

Adam Bak
Adam Bak

Reputation: 1279

Having the same line of code running twice is not always a matter of efficiency, but a matter of being able to avoid entering bugs into your code. You've captured the logic of how to choose a random string from an array in this line of code:

rooms1[random.nextInt(rooms1.length)]

The question becomes what happens when your logic on how to accomplish this changes. In your current situation you will have to be extra careful to find every instance of this code and properly change it.

One way to avoid code duplication is through the use of methods. You can capture your logic on how to find random strings in an array inside of a method and then you will only be required to maintain this code inside of a method and nowhere else.

The following code can be used as an example:

public static String getRandomStringFromRooms1() {
        Random random = new Random();
        return rooms1[random.nextInt(rooms1.length)];
    }

public static void main(String[] args) {
        addRooms();        
        String roomChosen = getRandomStringFromRooms1();
        while (rooms2.contains(roomChosen))            
            roomChosen = getRandomStringFromRooms1();
        System.out.println("Player has entered the " + roomChosen);
}

This example is; however, less efficient than your code, since each time you call the getRandomStringFromRooms1() method, you are creating a brand new Random object. If you do not like the idea of creating a Random object with each method call, since you might be calling this method potentially hundreds of times, you can create the Random object once and pass it into your method as follows:

public static String getRandomStringFromRooms1(Random random) {

        return rooms1[random.nextInt(rooms1.length)];
}

public static void main(String[] args) {
        addRooms();     
        Random random = new Random();
        String roomChosen = getRandomStringFromRooms1(random);
        while (rooms2.contains(roomChosen))            
            roomChosen = getRandomStringFromRooms1(random);
        System.out.println("Player has entered the " + roomChosen);
}

Best of luck.

Upvotes: 1

Sesame
Sesame

Reputation: 183

This doesn't look too bad to me. You could use a do while loop if you think repeating the same line of code looks ugly. In terms of efficiency, I don't think repeating lines of code is a real sign that your code is less efficient. I agree that it can look uglier though and be harder to understand.

For efficiency in this case, it might make sense to just drop the while loop and only select from the rooms that aren't in rooms2. This will be one line of code then, instead of an ambiguously long while loop.

String roomChosen = rooms1[random.nextInt(rooms1.length)];
while (rooms2.contains(roomChosen))
    roomChosen = rooms1[random.nextInt(rooms1.length)];

Will become:

String roomChosen;
do {
    roomChosen = rooms1[random.nextInt(rooms1.length)];
} while (rooms2.contains(roomChosen));

Upvotes: 0

Related Questions