Randomly pick a host from a list, subject to an uptime requirement

Question

I have a POJO as shown below which will have hostname and other attributes.

public class MachineInfo {

    private String hostname;

    // getters and setters here
}

I have my below method from which I need to randomly pick a hostname which is up for more than 10 minutes from machines list. The checkHostUptime method will return true if a particular machine is up for more than 10 minutes.

public static String pickRandomMachine(List<MachineInfo> machines) {
    String hostname = null;
    Collections.shuffle(machines, new Random(System.nanoTime()));
    for (MachineInfo info : machines) {
        if (!checkHostUptime(info.getHostName())) {
            continue;
        }

        hostname = info.getHostName();
        break;
    }

    return hostname;

}

I want to keep on trying finding the hostname until I find the hostname which is up for more than 10 minutes from machines list. If I am not able to find anything, then I would simply return null.

Is there any better way of doing the same thing?

Answer 1

Three main issues:

• It's generally impolite to alter the list that was passed to you, especially if such behaviour is not clearly documented.
• Collections.shuffle() seems to be overkill.
• Performance could be bad if the list that is passed to you is not suitable for random access (such as a linked list).

My suggestion:

public static String pickRandomMachine(List<MachineInfo> machines) {
    String[] candidates = new String[machines.size()];
    int n = 0;
    for (MachineInfo info : machines) {
        if (checkHostUptime(info.getHostname())) {
            candidates[n++] = info.getHostname();
        }
    }
    return n == 0 ? null : candidates[(new Random()).nextInt(n)];
}

The default Random() constructor should be fine. It would be better to reuse an instance of Random… though you should use ThreadLocalRandom if you need thread safety.

Answer 2

Naming

• by calling pickRandomMachine() where I pass a List<MachineInfo> I would assume that I will get a MachineInfo. Better name it to getRandomMachineHostName()

• if you name the parameter holding the List machines you should rename info to machine.

• hostname should be hostName, because it is camelCase casing for variable names.

General

• A Random should be created once and be reused.

• You should place and use the String hostname inside the loop, to remove the need to accessing machine two times.

• You should extract the shuffeling of the machines to a separate method.

• By returning out of the loop we can reverse the if condition

Refactoring

Implementing the above will lead to

public static String getRandomMachineHostName(List<MachineInfo> machines) {

    shuffleMachines(machines);

    for (MachineInfo machine : machines) {
        String hostName = machine.getHostName();
        if (checkHostUptime(hostName)) {
            return hostName;
        }
    }

    return null;
}

private static Random random = new Random(System.nanoTime());
private static void shuffleMachines(List<MachineInfo> machines) {
    Collections.shuffle(machines, random);
}