How to refactor code with varargs - put to map

Question

I have four methods with differents number of parameters. How to refactor these methods (is it possible to make single universal method?)

public Map<String, Object> addToMap(String name, Object value){
        Map<String, Object> map = new HashMap<String, Object>();
        map.put(name, value);
        return map;
    }
    
    public Map<String, Object> addToMap(String name, Object value, String name2, Object value2){
        Map<String, Object> map = new HashMap<String, Object>();
        map.put(name, value);
        map.put(name2, value2);
        return map;
    }
    
    public Map<String, Object> addToMap(String name, Object value, String name2, Object value2, String name3, Object value3){
        Map<String, Object> map = new HashMap<String, Object>();
        map.put(name, value);
        map.put(name2, value2);
        map.put(name3, value3);
        return map;
    }
    
    public Map<String, Object> addToMap(String name, Object value, String name2, Object value2, String name3, Object value3, String name4, Object value4){
        Map<String, Object> map = new HashMap<String, Object>();
        map.put(name, value);
        map.put(name2, value2);
        map.put(name3, value3);
        map.put(name4, value4);
        return map;
    }

How to do this?

Possibly related: [builder for HashMap](https://stackoverflow.com/q/7345241) — Pshemo, Jun 17 '21 at 20:21
in case you can only edit this code, you can't... the only way to do this is to accept an array of object, and when you call `put` cast them back to the type you need.. obviously something you don't want since someone can pass a odd number of params, and those params can be any type — Alberto Sinigaglia, Jun 17 '21 at 20:22

score 5 · Accepted Answer · answered Jun 17 '21 at 21:11

Simple answer

You're doing it wrong. This stuff already exists; use e.g. guava's ImmutableMap.builder, or use the existing Map.of(). It's not quite the same: Your stack of method 'hardcodes' that it's a Map<String, Object>, but presumably it's not worth rewriting the ImmutableMap or Map.of API for just that benefit.

Complicated answers

Note that your naming is off. addToMap is not a good method name, particularly not for a method that creates an entirely new map. addToMap just screams 'this adds the provided arguments to an existing map', which these methods don't do. So I took the liberty of fixing your names for you. These methods also have no state so should be static.

Option #1: Passthrough to a private method.

public static Map<String, Object> newMap(String name, Object value) {
    return newMap0(name, value);
}

public static Map<String, Object> newMap(String name1, Object value2, String name1, Object value2) {
    return newMap0(name1, value1, name2, value2);
}
    
public static Map<String, Object> newMap(String name1, Object value1, String name2, Object value2, String name3, Object value3) {
    return newMap0(name1, value1, name2, value2, name3, value3);
}
// and so on

private static Map<String, Object> newMap0(Object... v) {
    assert v.length % 2 == 0;
    var out = new HashMap<String, Object>();
    for (int i = 0; i < v.length; i +=2) out.put((String) v[i], v[i + 1]);
    return out;
}

That last method is type-wise a disaster (freely allows passing in an odd number of arguments or non-string keys), which is why it has no business being a part of your API. That's why it's private.

option 2: a builder

Check out ImmutableMap.builder for the full treatise. This is a whole bunch of complicated code, but it makes for a decently nice API. Your goal is that a caller can do this:

YourUtilityClassThing.newMap()
  .put("a", 5)
  .put("b", 10)
  .build();

This has the considerable advantage of being 'typesafe' (you can only pass in String keys and any object for a value), as well as allowing an infinite amount of k/v pairs. The downside is, your API needs quite a bit of code to make this work.

I cannot stress strongly enough how you should not be reinventing this wheel and using e.g. guava's ImmutableMap.Builder instead, if you like this approach.

option 3: a tuple type

Make an explicit tuple type and write your API docs to indicate that it is not usable without aggressive use of static imports, but with static imports, it's nice. The callers end up writing:


import static com.foo.YourThingie.*;

....

newMap(p("a", 5), p("b", 5));

You need quite a bit of code in YourThingie to tie this together, but it's typesafe and supports infinite arguments:

class YourThingie {
    public static final class KeyValuePair {
        String key; Object value;
    }

    public static KeyValuePair p(String key, Object value) {
        KeyValuePair kvp = new KeyValuePair();
        kvp.key = key; kvp.value = value;
        return kvp;
    }

    public static Map<String, Object> newMap(KeyValuePair... pairs) {
        var out = new HashMap<String, Object>();
        for (var pair : pairs) out.put(pair.key, pair.value);
        return out;
    }
}

You may want to make KeyValuePair a proper class: Immutable, with a 2-arg constructor, and a proper toString/equals/hashCode impl. That either involves a pageful of boilerplate, or use JDK16's record, or use Project Lombok's @Value to get this (DISCLAIMER: I'm known to contribute to lombok quite a bit).

score 0 · Answer 2 · answered Jun 17 '21 at 20:30

You can do this using two approaches: the compile-time and the run-time one.

The compile-time approach is just like you're doing right now: a lot of overloads to guarantee that the parameters will be in pairs.

Pros:

Anyone calling this method using an odd number of arguments or using a non-String argument as names will not be able to compile the code.

Cons:

A lot of typing.
Refactoring will be awful.

You can reduce the refactoring con by method redirection:

public Map<String, Object> addToMap(String name, Object value){
    Map<String, Object> map = new HashMap<String, Object>();
    map.put(name, value);
    return map;
}
    
public Map<String, Object> addToMap(String name, Object value, String name2, Object value2){
    Map<String, Object> map = new HashMap<String, Object>();
    map.addAll(addToMap(name, value));
    map.addAll(addToMap(name2, value2));
    return map;
}
    
public Map<String, Object> addToMap(String name, Object value, String name2, Object value2, String name3, Object value3){
    Map<String, Object> map = new HashMap<String, Object>();
    map.addAll(addToMap(name, value, name2, value2));
    map.addAll(addToMap(name3, value3));
    return map;
}

_{Don't forget you're a programmer, write code to do that for you!}

This approach is also used in Map.of method.

The run-time approach is something like this:

public Map<String, Object> addToMap(Object... args) {
    if (args.length % 2 != 0)
        throw new IllegalArgumentException("number of arguments must be even");

    for (int i = 0; i < args.length; i++) {
        final Object arg = args[i];
        if (i % 2 == 0 && !(arg instanceof String)) 
            throw new IllegalArgumentException("odd arguments must be of type String");
    }

    Map<String, Object> map = new HashMap<String, Object>();
    for (int i = 0; i < args.length - 2; i += 2) {
        final String name = args[i] as String;
        final Object value = args[i+1];
        map.put(name, value);
    }
    return map;
}

Pros:

Reduced typing

Cons:

Anyone calling this method using an odd number of arguments or using a non-String argument as names will be able to compile the code, only raising an error when executing the method.

Usually making a clearly vastly worse API in order to safe a few characters worth of typing when making the API is a clear bad move. I cannot stress how bad that second snippet is. The first snippet is impressive in how much memory and allocations it wastes, but given that we're talking about 4-tuple maps, nobody's going to notice. For what its worth, the maps can be pre-sized with the right constructor. `Map.of` and guava's ImmutableMap already take care of all of this. — rzwitserloot, Jun 17 '21 at 21:13
Of course it is a bad move, that's why I put a "cons" section in that snippet. About the first one, that's based on OP code. I don't know if that's a MRE and there's more code besides creating a map, adding entries to it and returning it, so that's why I left as it was. If all OP want is to create a new map, I added an `Map.of` documentation link in case you didn't notice it. — enzo, Jun 17 '21 at 21:22
`addToMap("one", 1)` returns an empty map. `args [i] as String` will result in a compilation error. — , Jun 17 '21 at 22:02

How to refactor code with varargs - put to map

2 Answers2

Simple answer

Complicated answers