Getters and setters in a Person class

I thought to do away with getters and setters behind the real world objects such as Person, Company or Address, i.e. getName and setName.

Each of these have public enum Attrib of states.

And the state is actually a pure Object mapped to a private HashMap.

package model;

import java.util.HashMap;
import java.util.Map;

public class Person {

    public enum Attrib {
        NAME, //String
        EMAIL, //String
        PHONE; //int

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

    private final Map attribs;

    public Person(String name, String email, int phone) {
        attribs = new HashMap<>();
        attribs.put(Person.Attrib.NAME, name);
        attribs.put(Person.Attrib.EMAIL, email);
        attribs.put(Person.Attrib.PHONE, phone);
    }

    public Object getAttrib(Person.Attrib attribute) {
        return attribs.get(attribute);
    }

    public boolean setAttrib(Person.Attrib attribute, Object value) {
        if (attribute == Person.Attrib.PHONE && !(value instanceof Integer)) {
            return false;
        }
        attribs.put(attribute, value);
        return true;
    }

    @Override
    public boolean equals(Object o) {
        if (o instanceof Person) {
            Person person = (Person) o;
            String _this = (String) this.getAttrib(Person.Attrib.NAME);
            String _that = (String) person.getAttrib(Person.Attrib.NAME);
            return _this.equalsIgnoreCase(_that);
        }
        return false;
    }

    @Override
    public int hashCode() {
        return attribs.get(Person.Attrib.NAME).hashCode();
    }

    @Override
    public String toString() {
        return (String) attribs.get(Person.Attrib.NAME);
    }
}

I did saved some getters and setters but I guess it can't justify that all attributes are objects. To check what instanceof the getAttrib(Person.Attrib attribute) is, i.e. skim the class source to know whether you d

As you observed, casting within your implementation of setAttribute() is ugly. Worse than that, returning Object from getAttribute() is horrible — you've given up all type safety. In a loosely typed language, that would just be business as usual. However, in Java, the caller would have to cast the result (cumbersome and a potential source of bugs) or just treat it as an opaque Object (spreading the nastiness throughout the program).

Are you sure that you want to represent phone numbers as integers? You would only get 9 digits reliably. That's not enough to hold, say, an American phone number including the area code, which would be 10 digits. Not to mention, you may need country codes, extension numbers, and spaces or punctuation for readability. A string may be more appropriate after all.

Now, suppose that you just store all three attributes as strings, which eliminates the type safety issue. What if you want to implement validation within setAttribute()? You would end up putting in a switch, which would defeat any code savings you have achieved by doing this instead of implementing a separate setter for each attribute.

In summary, this is a bad idea, unless you had a requirement to associate many obscure or arbitrary attributes with a person (e.g. pet, priest, etc.). In that case, you would implement something more elaborate, such as

public abstract class ModelEntity {
    public String getAttribute(String attrname);
    public void setAttribute(String attrname, String value) throws ValidationException;
    public ModelEntity getRelationship(String relname);
    public void setRelationship(String relname, ModelEntity obj) throws ValidationException;
}

public class Person extends ModelEntity {
    ...
}

public class Pet extends ModelEntity {
    ...
}

… but you would be doing that out of necessity, not out of laziness. If you really wanted to be sloppy and lazy, you might as well publicly expose the Person's instance variables (not that I'm recommending that at all!).

Returning false to indicate validation failure is poor design. I would throw an exception to force the caller to deal with it.

Your hashCode() and equals() methods need to be consistent. If your equals() compares names case insensitively, then your hashCode() should also squash the name to lowercase.

StackExchange Code Review Q#33914, answer score: 19