Generating a valid number of combinations of datacenter and its host ID

I have a list of datacenters, which are dc1, dc2 and dc3. And each datacenter has six IDs as of now (can be 11 in each datacenter as well or uneven number for each datacenter).

dc1 - h1, h2, h3, h4, h5, h6
dc2 - h1, h2, h3, h4, h5, h6
dc3 - h1, h2, h3, h4, h5, h6

Now I am trying to generate combinations of datacenter and its ID so the rules are:

• In each pass, we will have each datacenter and alternate IDs.

For example:

dc1=h1, dc2=h2, dc3=h3

If you see above, each datacenter has unique ID, meaning dc1 has h1, dc2 has h2 and dc3 has h3. We cannot have same ID for each datacenter in any of the passes, like we cannot have like this:

dc1=h1, dc2=h1, dc3=h3

If you see above, h1 appears twice in dc1 and dc2 which is wrong.

-
In the next pass, we should not use same ID for that datacenter. Meaning if below is the first pass:

dc1=h1, dc2=h2, dc3=h3

then second pass can be as below. If you see below, dc1 has h4 in the second pass, which has some other ID apart from h1, as h1 was used in the first pass for dc1 datacenter and same with dc2 and dc3.

dc1=h4, dc2=h5, dc3=h6

We cannot have same ID for same datacenter in the next pass if it was used earlier. So we cannot have this in the second pass:

dc1=h4, dc2=h2, dc3=h6

as h2 was used earlier for dc2 in the first pass.

Now I have got all the above logic working fine in my below code. I'd like to see whether it will work fine in all the input cases and if it will follow my two above rules or not. I have tested on some of the input cases and it works fine.

```
public static class ConfigurationGenerator {
private Map> lists;

public ConfigurationGenerator(Map> lists) {
this.lists = new LinkedHashMap>();
// Make copies of the lists so we can modify them without touching the original lists.
for (K key : lists.keySet())
this.lists.put(key, new ArrayList<>(lists.get(key)));
}

Unit testing

Your code does the job. But right now there's no good way to verify that. Running the main method with various inputs and reading the output is not a good way to verify. This is relatively easy to fix by converting your manual tests to unit tests that are easy to re-run and re-verify, for example:

public class ConfigGeneratorTest {
    private String resultToString(Map map) {
        return new TreeMap<>(map).toString();
    }

    @Test
    public void testBasicExampleWithSameHosts() {
        Map> lists = new LinkedHashMap<>();
        lists.put("dc1", Arrays.asList("h1", "h2", "h3", "h4"));
        lists.put("dc2", Arrays.asList("h1", "h2", "h3", "h4"));
        lists.put("dc3", Arrays.asList("h1", "h2", "h3", "h4"));

        ConfigGenerator generator = new ConfigGenerator<>(lists);
        assertEquals("{dc1=h1, dc2=h2, dc3=h3}", resultToString(generator.next()));
        assertEquals("{dc1=h2, dc2=h1, dc3=h4}", resultToString(generator.next()));
        assertEquals("{dc1=h3, dc2=h4, dc3=h1}", resultToString(generator.next()));
        assertEquals("{dc1=h4, dc2=h3, dc3=h2}", resultToString(generator.next()));
        assertTrue(generator.next().isEmpty());
    }

    @Test
    public void testExampleWithEmptyHosts() {
        Map> lists = new LinkedHashMap<>();
        lists.put("dc1", Arrays.asList("h1", "h2", "h3"));
        lists.put("dc2", Arrays.asList());

        ConfigGenerator generator = new ConfigGenerator<>(lists);
        assertEquals("{dc1=h1}", resultToString(generator.next()));
        assertEquals("{dc1=h2}", resultToString(generator.next()));
        assertEquals("{dc1=h3}", resultToString(generator.next()));
        assertTrue(generator.next().isEmpty());
    }

    @Test
    public void testExampleWithVariableHosts() {
        Map> lists = new LinkedHashMap<>();
        lists.put("dc1", Arrays.asList("h1", "h2"));
        lists.put("dc2", Arrays.asList("h1", "h2", "h3"));
        lists.put("dc3", Arrays.asList("h3", "h4", "h5"));

        ConfigGenerator generator = new ConfigGenerator<>(lists);
        assertEquals("{dc1=h1, dc2=h2, dc3=h3}", resultToString(generator.next()));
        assertEquals("{dc1=h2, dc2=h1, dc3=h4}", resultToString(generator.next()));
        assertEquals("{dc2=h3, dc3=h5}", resultToString(generator.next()));
        assertTrue(generator.next().isEmpty());
    }
}

I wrote these test cases before improving your code. When writing the assertions, you don't even have to think too much. You can start by adding failing assertions like this:

assertEquals("blah", resultToString(generator.next()));

When you run this test case, it will fail, but it will tell you what was the expected result. If that result seems to make sense, you can copy-paste it into the assertion. You can continue this until all tests pass. Converting your examples took me only a few minutes this way.

Until this point, I only made trivial changes and some style changes for my taste:

• Renamed the ConfigurationGenerator class to ConfigGenerator, which is just shorter, more comfortable, without losing meaning

• Renamed the extractConfiguration method to next, which is short and sweet

• Return an empty map instead of null when there are no more possible configurations. Empty collections are more ergonomic in general, because checking null values is often tedious, ugly, and an unnecessary additional check. It's good to avoid using null values whenever possible.

These unit tests are easy to rerun, so now I can boldly go and make changes, with the comfort of knowing that the unit tests will scream at me if I break something.

Improvements

The variable storing the mapping of data centers to hostnames can be improved:

private Map> lists;

This can be final, as later in your code you initialize it once, and you don't need to reassign it. final elements make the code more robust by preventing unintended changes by accident.

Since duplicate hostnames won't make sense for your use case, so a Set will be better. Later in your code where you remove hostnames from the list, a Set will be more efficient.

private final Map> lists;

There are several efficiency and coding style issues here:

public ConfigurationGenerator(Map> lists) {
    this.lists = new LinkedHashMap>();
    for (K key : lists.keySet()) 
        this.lists.put(key, new ArrayList<>(lists.get(key)));
}

It's strongly recommended to use braces around for and if statements.

You use Java 7 style diamond operator in new ArrayList<>, but not for the LinkedHashMap. Why not just use the diamond operator everywhere.

In the loop you iterate over the keys, and then lookup elements in the input map using that key. It's more efficient to iterate over the entries, so that you don't need to perform an additional lookup by key. Like this:

```
this.lists = new LinkedHashMap<>();
for (Map.Entry> entry : lists.entrySet()) {
K key = entry.getKey();
List l

StackExchange Code Review Q#56817, answer score: 4