8
\$\begingroup\$

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<Person.Attrib, Object> 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 deal with a String or an Integer or Whatever, type cast it.

\$\endgroup\$
  • 10
    \$\begingroup\$ public int phone; is, IMO, a Very Bad Thing. If you're not doing math with it, use a string, Full Stop. \$\endgroup\$ – MikeTheLiar Nov 6 '13 at 14:28
  • 5
    \$\begingroup\$ Sometimes you will need a leading 0 or sometimes double 0 (00) to dial some numbers, which will be lost with an integer representation. In some (rarer) case you may need "," to temporize between some numbers, too. And an integer will be less human-readable (and there are several ways to group numbers, depending on the country and if it's a local or inter-country call, so you can't 'guess' it from the integer value). Finally, as pointed out by 200_success, the numbers would be limited to the integer range, and this could be not enough. All good reasons that you can't use integer representation. \$\endgroup\$ – Olivier Dulac Nov 6 '13 at 15:13
  • 2
    \$\begingroup\$ @olivier a leading 0 also gets turned into octal, so you've got that to deal with as well. \$\endgroup\$ – MikeTheLiar Nov 6 '13 at 15:20
  • \$\begingroup\$ @mikeTheLiar: yes, another (of possibly many more) reason not to use integer ^^ \$\endgroup\$ – Olivier Dulac Nov 6 '13 at 17:12
  • \$\begingroup\$ what's bugging me more than anything else is name.toLowerCase() for the enum Attrib toString() method. Horribly inefficient. \$\endgroup\$ – MadConan Nov 7 '13 at 13:25
19
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ @paveljurca: as 200_success points out, integer to represent numbers is bad. Sometimes you will need a leading 0 or sometimes double 0 (00) to dial some numbers, which will be lost with an integer representation... \$\endgroup\$ – Olivier Dulac Nov 6 '13 at 15:11
  • 2
    \$\begingroup\$ @OlivierDulac I do hope you mean phonenumbers. "Integer to represent numbers is bad" is a bit overkill. ;) \$\endgroup\$ – Simon Forsberg Nov 6 '13 at 20:43
  • \$\begingroup\$ Good answer. I would add that this approach makes unit testing extremely difficult as setAttribute and getAttribute will have potentially hundreds of code paths that cannot be broken down. \$\endgroup\$ – John Wu Nov 7 '13 at 1:47
  • 2
    \$\begingroup\$ @SimonAndréForsberg: you did made me laugh ^^ ... However the context (answer + my following sentence stating "to dial some numbers" made it clear ;) I also didn't precise "in your program" "at the current technology level and on this planet", etc... So many things could change the whole thing! It's mind boggling. In any sentence we have to accept that there always will be assumtions about a "common context" that goes uncited ^^ [that's what makes citations so different depending on who uses them and where they cut the sentences ^^] \$\endgroup\$ – Olivier Dulac Nov 7 '13 at 8:12
  • \$\begingroup\$ Good answer. Apart from all point mentioned; Regular getters/setters will be understandable by other developers looking at the code, whereas the code of the OP will be confusing and take time to understand, which will make the code confusing and may lead to bugs. \$\endgroup\$ – thaJeztah Nov 8 '13 at 23:01
13
\$\begingroup\$

You are right, this is not recommended practice. In your case, you need to know how the internals of the Person class works in order to use it effectively. As much as getters and setters appear to be clutter, they actually create a protocol, standard, and convenient system that you can take for granted. It is helps for everything from code readability (not necessarily this Person class, but the code that uses the Person class), to code maintainability, and even code generation.

Because getters and setters are such a common, simple, and reliable pattern the Java IDE's often make things really simple. For example, if I code your class in Eclipse, I would do the following:

public class Person {
    private String name;
    private String email;
    private int phone;
    // I create an empty line here....
}

Then I would type Alt-Shift-S and select 'Create Constructor Using Fields...' and hit enter, then I would type Ctrl-Shift-S again and select 'Generate Getters and Setters...' and I would hit 'Select All' and then enter. This is the result:

public class Person {
    private String name;
    private String email;
    private int phone;
    public Person(String name, String email, int phone) {
        super();
        this.name = name;
        this.email = email;
        this.phone = phone;
    }
    public String getName() {
        return name;
    }
    public void setName(String name) {
        this.name = name;
    }
    public String getEmail() {
        return email;
    }
    public void setEmail(String email) {
        this.email = email;
    }
    public int getPhone() {
        return phone;
    }
    public void setPhone(int phone) {
        this.phone = phone;
    }

}

It took 20 seconds or less.

Saving time and convenience is not an excuse.

\$\endgroup\$
3
\$\begingroup\$

I think, you don't save any work to users of your class Person: The setAttrib method needs a further literal argument, which is only a pseudo-argument, and each time calling the getAttrib method, a cast is needed with all its downsides.

Obviously, you also don't save any work for maintainers of this class, because its structure is much more complicated than the every-day getter and setter approach.

Maybe the following would be an option (this is how I always imagined POJOs):

public class Person {
    public String name;
    public String email;
    public int phone;
    public Person() {}
}

maybe not:

Is it necessary to have getters and setters in POJO's

I'm no java programmer (but I'd discourage such "mutli-methods" even from a C/C++ POV)

\$\endgroup\$
  • \$\begingroup\$ Making all fields public is very bad idea. You can't make any validation when someone is changing the value of any field. Actually setters-getters are not only for encapsulation but for also data-safety. Further reading and get over it C/C++ != Java \$\endgroup\$ – Anirban Nag 'tintinmj' Nov 7 '13 at 12:46
2
\$\begingroup\$

As other people have pointed out, this is probably a bad idea. There are some instances where attribute specifiers have a legitimate role, and in such cases, you can get type-checked attributes using generics like this:

public class Person {

  public static class Attrib<U>{
    private Attrib() {}
  }
  public static final Attrib<String> NAME = new Attrib<>();
  public static final Attrib<String> EMAIL = new Attrib<>();
  public static final Attrib<Integer> PHONE = new Attrib<>();

  private final Map<Attrib, Object> attribs;

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

  public <U> U getAttrib(Attrib<U> attribute) {
    return (U) attribs.get(attribute);
  }

  public <U> void setAttrib(Attrib<U> attribute, U value) {
    attribs.put(attribute, value);
  }


}
\$\endgroup\$
2
\$\begingroup\$

Let's take your idea to the extreme. You want to get rid of specific getters and setters? Heck, why not get rid of specific classes too? Everything generic.

So instead of

Person person = new Person();
person.setAttribute(Person.Attribute.EMAIL, "john.doe@test.com");

why not have

Thing thing = new Thing(Thing.Type.PERSON);
thing.setAttribute(Person.Attribute.EMAIL, "john.doe@test.com");

Hey look I just got rid of all those useless classes and replaced them with one generic class, Thing. WOW! Why didn't anyone think of this before? Maybe I can replace ALL of my classes like this!!!

Obviously this is ridiculous. At some point, someone has to declare that certain things that have certain behavior, can only have certain attributes, and that the attributes conform to certain rules. That is how we define the system in code.

The way we do this is by defining classes for the different things and properties for the attributes. And when we do that, we benefit from things like inheritance, composition, type safety, etc.

If you get rid of all that, you... well... get rid of all that.

\$\endgroup\$

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.