2
\$\begingroup\$

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?

\$\endgroup\$
2
\$\begingroup\$

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.

|improve this answer|||||
\$\endgroup\$
2
\$\begingroup\$

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);
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for suggestion. Quick question - does random have to be static in this scenario? \$\endgroup\$ – arsenal Dec 19 '14 at 9:04
  • \$\begingroup\$ As you will use the random variable yes. \$\endgroup\$ – Heslacher Dec 19 '14 at 9:08

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.