-1

So currently I looking for some hints from more experienced programmers. I am in the point where, when looking into my code it seems not readable, what is probably big flaw. So lets go to the code itself:

In one the classes I have method:

    protected void createModelsCharacterisitcListGroup(Map<CharacterType, Map<String, String>> characters) {

        for (Entry<CharacterType, Map<String, String>> outterEntry : characters.entrySet()) {
            for (Entry<String, String> innerEntry : outterEntry.getValue().entrySet()) {
                outterEntry.getKey().create(characters.get(outterEntry.getKey()).get(innerEntry.getKey()), source.getStringified(innerEntry.getKey()));
            }
        }

}

And CharacterType is the enum like this:

public enum CharacteristicType {
    TYPE1 {

        @Override
        public void create(String name, String value){

        }
    },
    TYPE2{

        @Override
        public void create(String name, String value) {

        }
    };

    public abstract void create(String name, String value);
}

So what I want to do is:

Call method create for given CharacterType in Map with Strings from inner map

3
  • 2
    I think you should start by figuring proper names for your variables. outerEntry and innreEntry are terrible and useless names. After you've done that, maybe you realize that your types are wrong.
    – Euphoric
    Oct 2, 2019 at 7:42
  • 3
    I would start by fixing the obvious spelling errors. Then, I would introduce some explaining variables to split up the inner calls to create into understandable pieces.
    – Doc Brown
    Oct 2, 2019 at 9:06
  • 4
    Surely characters.get(outterEntry.getKey()).get(innerEntry.getKey()) is just innerEntry.getValue()?
    – Caleth
    Oct 2, 2019 at 10:42

1 Answer 1

1

Using temporary variables can help a lot (note I'm not a Java programmer, so syntax might be wrong):

for (Entry<CharacterType, Map<String, String>> outterEntry : characters.entrySet()) {
    for (Entry<String, String> innerEntry : outterEntry.getValue().entrySet()) {
        var arg1 = characters.get(outterEntry.getKey()).get(innerEntry.getKey());
        var arg2 = source.getStringified(innerEntry.getKey());
        outterEntry.getKey().create(arg1, arg2);
    }
}

arg1 and arg2 should be given meaningful names, of course. The expression assigned to arg1 looks still fairly unreadable, which might be due to both its length and the generic method names get() and getKey() which convey technical function but not meaning.

Not the answer you're looking for? Browse other questions tagged or ask your own question.