0
\$\begingroup\$

I'm roughly trying to follow the guidelines in "Effective Java", "Item 15: Minimize mutability", hence the class is not "final" on purpose.

Information is gathered from a number of temperature meters.
Some may fail to report, so the size of the map is not known.

The serializing bothers me. Is there a simpler way to make this class immutable ?

public class Result<T extends Number> {
    private final Map<String, T> result;

    private Result(final Map<String, T> result) {
        this.result = result;
    }

    public static final Result<Number> of() {
        return new Result<>(new LinkedHashMap<>());
    }

    /** Add or replace value. */
    public Result<T> put(final String key, final T value) {
        final Map<String, T> copy = new LinkedHashMap<>(result);
        copy.put(key, value);
        return new Result<>(copy);
    }

    @SuppressWarnings("unchecked")
    public T get(final String key) throws Exception {
        return ((T) convertFromBytes(convertToBytes(result.get(key))));
    }

    @Override
    public String toString() {
        return result.toString();
    }

    private byte[] convertToBytes(Object object) throws IOException {
        try (ByteArrayOutputStream bos = new ByteArrayOutputStream(); ObjectOutput out = new ObjectOutputStream(bos)) {
            out.writeObject(object);
            return bos.toByteArray();
        }
    }

    private Object convertFromBytes(byte[] bytes) throws IOException, ClassNotFoundException {
        try (ObjectInput in = new ObjectInputStream(new ByteArrayInputStream(bytes))) {
            return in.readObject();
        }
    }
}
\$\endgroup\$
0
1
\$\begingroup\$

At the moment your class is not immutable because the Map given in the constructor is not copied (and Map itself isn't immutable). You should instead have a constructor like

public Result(final Map<String, T> result) {
    this.result = new LinkedHashMap<>(result);
}

What's the goal of the of() method ? Shouldn't it be named empty() instead ?

public static final Result<Number> empty() {
    return new Result<>(new LinkedHashMap<>());
}

Or having two parameters (1 key, 1 value) ?

public static final Result<Number> of(final String key, final T value) {
    Map<String, T> base = new LinkedHashMap<>();
    base.put(key, value);
    return new Result<>(base);
}

The fact that get() throws an IOException or a ClassNotFoundException bothers me because it makes the current underlying implementation "leaking" out of your class. Who wants (and expects) to have to handle an IOException when getting a value from a Map ? Since these exceptions are not supposed to happen in this case, I would rethrow them as unchecked exceptions

@SuppressWarnings("unchecked")
public T get(final String key) throws Exception {
    try {
        return ((T) convertFromBytes(convertToBytes(result.get(key))));
    } 
    catch(IOException | ClassNotFoundException e) {
        throw new RuntimeException(e);
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ changed constructor to private. \$\endgroup\$
    – user77115
    Aug 15 '16 at 5:59
  • \$\begingroup\$ of() method was supposed to replace the constructor (since it is now private) \$\endgroup\$
    – user77115
    Aug 15 '16 at 6:00
  • \$\begingroup\$ @user77115 So why not naming this method empty() ? I've always encountered of(something) methods. See Optional for example. \$\endgroup\$
    – Spotted
    Aug 15 '16 at 6:04
  • \$\begingroup\$ changed to Exception. \$\endgroup\$
    – user77115
    Aug 15 '16 at 6:05
  • \$\begingroup\$ this of() business is takenfrom Effective Java AFAICR. just following that template. \$\endgroup\$
    – user77115
    Aug 15 '16 at 6:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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