How to generate valid number of combinations basis on the rules?

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.

Problem Statement:

Now I have got all the above logic working fine in my below code:

```
public static List> createDatacenterIdMappings(Map> input, boolean debug) {
// Output holder
ArrayList> output = new ArrayList>();

// Initialize variables
int szRaw = input.size();
int minReq = -1;
for (List dataCenters : input.values()) {
minReq = dataCenters.size() > minReq ? dataCenters.size() : minReq; // Find minimum size required
}
minReq = input.size() > minReq ? input.size() : minReq;

// Flip map from datacenters : id to id

Your code is very hard to understand. As I mentioned in a comment, I needed roundabout 2 hours to unwrap it, and that was just for understanding. In that process I renamed a big part of your variables, so: here goes the review, be prepared

Naming:

Overall the naming quality of this code is between acceptable and hellishly gruesome.

int minReq = -1;

This name is a drastic shortening, and missing too much context to describe exactly what it does. I renamed to requiredIterations, and suddenly the for-loop 20 LoC later was a little clearer. Everytime you read minReq, you need to remember: "minReq stands for the minimum required iterations". If you read requiredIterations then you know: "this stands for the minimum required iterations".

int k = 0;

Later you use k to do some very useful, but hard to understand stuff. Do you actually know what k is? I can tell you. It's an index, that's persistent through the different for-loops you have. I thusly named it persistentIndex.

And this is where it gets fishy. Your requirements don't match the currently used code! This totally boggled me.

Map map = new LinkedHashMap();
Map outputMap = new LinkedHashMap();

These are horrible too. What is in your Map map? You comment there "tracking status in one map, output in the other". This is not a comment, it's a name! your map is better off with statusTrackingMap.

Then there is that outputMap. I confused that one with the output fairly often, because they both start with output. What do you put in there? The current data-center:host combinations. Then name it, so it contains what it says on the tin!: currentCombinationsMap is definitely better.

List list = input.get(keys.get(j));

ouch. First Map map and now List list? Again rename it. I chose machines from the comment you wrote before flipped.

int sz = list.size();

or rather, now:

int sz = machines.size();

This one is the worst after k. Use machineCount or something along these lines instead!

int initK = k;

That one is also kinda evil. This is the break-marker for the following do-while loop, given the case persistentIndex rolls back to 0. I called it startingPersistentIndex, which still isn't good, but definitely better than initK.

int szRaw = input.size();

You never use that variable. That was the first thing I could remove.

Code-Smells:

You have a few not-very-nice code-smells in that code, one of them is this here:

if (!found) {
    continue;
}
//following code

This is in fact a guard clause, and a not even quite bad at that. But it's overly complicated to track back. What you actually want to say is:

if(found){
   //following code
}

IMO this is easier to understand.

While we're at it:

if (!map.values().contains(valueToUse)) {
    found = true;
    break;
}

is in fact nothing else but:

found = !map.containsValue(valueToUse);

for the following program. And here we see, that the name is wrong. It says found on the tin, but it tells you if the valueToUse was not found in the statusTrackingMap.

Aaand another rename:

if(!map.containsValue(valueToUse)){
    valueToUseNotFound = true;
    break;
}

And continuing from that, this could be refactored to sit somewhere different, but then your behavior changes. As I don't know how that would be seen, it might be difficult, but in fact, this is just a break-condition for your do-while loop.

It could be rewritten to:

do {
    //stuff
} while (persistentIndex != startingPersistentIndex && !valueToUseNotFound)

But this includes another increment of the persistentIndex and I was reluctant to change the behavior of the code.

Your innermost for-loop then ends with:

if (valueToUseNotFound) {
    statusTrackingMap.put(dataCenters.get(j), valueToUse);
    if(flipped) {
        currentCombinationsMap.put(valueToUse);
    } else {
        currentCombinationsMap.put(dataCenters.get(j), valueToUse);
    }
    machines.remove(persistentIndex);
}

And this is where I lost it, when I examined WHY IN GOD'S NAME THE HELL the expected output when calling your old method with the same input as my refactored method and asserting: assertEquals(oldOutput.toString(), newOutput.toString()) was [{}, {}, {}]

So here is your lesson for all eternity, and if it wasn't you who committed this crime, then slap that person with a boulder!

DON'T TINKER WITH INPUT!

Keep references intact, somebody outside may want to reuse them!

It's like you gave some person a reference to your bag of Keys, asking him to tell you how he would reorder them on different bunches, and when you check your keybag, suddenly all the keys have vanished.

What you do is: you copy the data, and you leave the original references intact. How? like this:

```
Map> clone = new LinkedHashMap<>();
for (String key : input.keySet()) {
clone.put(key, new ArrayList<>(input.get(key)))

StackExchange Code Review Q#56406, answer score: 6