Refactoring: removal of duplication in constructor

user_13963086 0
Viewed 50

I seem to be lacking enough coffee to make me see the following problem clearly.

Imagine I have a class with two constructors and several fields. One constructor is a no-arg constructor and one field depends on another. The other constructor takes an injected value for one of its fields. Example:

public class Practice {
    private final int n;
    private final char c;
    private final Map<String, String> m;
    private final Set<String> s;

    public Practice() {
        this.n = 0;
        this.c = 'a';
        this.m = new HashMap<>();
        this.s = m.keySet();
    }
    
    public Practice(Set<String> s) {
        this.n = 0;
        this.c = 'a';
        this.m = new HashMap<>();
        this.s = s;
    }
}

My question: how do I remove the duplication of code between the two constructors?

First failed attempt:

public Practice() {
    this(new HashMap<>(), new HashMap<>().keySet());
}

public Practice(Set<String> s) {
    this(new HashMap<>(), s);
}

private Practice(int n, char c, Map<String, String> m, Set<String> s) {
    this.n = 0;
    this.c = 'a';
    this.m = m;
    this.s = s;
}

Of course, this fails because the no-args constructor creates two separate Maps instead of one.

refactoring java
3 Answers

In cases where one argument depends on another argument you can solve the problem by adding an additional constructor. In this case private Practice(Map<String,String> map)

public Practice() {
    this(new HashMap<>());
}

public Practice(Set<String> s) {
    this(new HashMap<>(), s);
}

private Practice(Map<String,String> map) {
    this(map, map.keySet());
}

private Practice(Map<String,String> map, Set<String> s) {
    this.n = 0;
    this.c = 'a';
    this.m = map;
    this.s = s;
}
user_100941770

You can just initialize variables as you define them as members. In your constructor you can stick to initializing strictly those variables that can hold a different value after initialization.

Also note: Since n and c are primitives they are suitable candidates to be static, since they are also tagged final. This does not hold true for m.

public class Practice {
    private static final int n = 0;
    private static final char c = 'a';
    private final Map<String, String> m = new HashMap<>();
    private final Set<String> s;

    public Practice() {
        this.s = m.keySet();
    }
    
    public Practice(Set<String> s) {
        this.s = s;
    }
}

Lastly - it is not necessary to kill each and any duplication of code. The rules on this vary, though I usually follow the rule-of-three or when the duplicated chunks are large.

user_74476600

For the initial version, you could pass a null from the default constructor and then check for null when setting s:

public class Practice {
    private final int n;
    private final char c;
    private final Map<String, String> m;
    private final Set<String> s;

    public Practice() {
        this(null);
    }
    
    public Practice(Set<String> s) {
        this.n = 0;
        this.c = 'a';
        this.m = new HashMap<>();
        this.s = null == s ? m.keySet() : s;
    }
}

Similarly, the version with 3 constructors can be updated:

public Practice() {
    this(null);
}

public Practice(Set<String> s) {
    this(0, 'a', new HashMap<>(), s); // as all args constructor is private
}

private Practice(int n, char c, Map<String, String> m, Set<String> s) {
    this.n = n;
    this.c = c;
    this.m = m;
    this.s = null == s ? m.keySet() : s;
}
user_132798310
Related