5

Let's take this class as an example:

public class Student{
    private String name;
    private String id;

    public Student(String name, String id){
        this.name = name;
        this.id = id;
    }

    ... getters and setters for both fields

and compare it to this:

public class Student{
    public final String name;
    public final String id;

    public Student(String name, String id){
        this.name = name;
        this.id = id;
    }
 }

There is in my opinion no need for the accessors.
Would this be considered bad OO design?

rtheunissen
  • 7,347
  • 5
  • 34
  • 65
  • 1
    imho, it's not bad design. though it doesn't follow convention. what if, in the you need to provide pre/post processing in the getter and/or setter. – Alex Aug 08 '12 at 00:11
  • Why would you have a setter if you say they will never change? – Alex Fischer Aug 08 '12 at 00:14
  • 1
    However you do it, be consistent. As far as using OO design as your gauge - POJOs alone aren't exactly the paragon of OO. – Matt Ball Aug 08 '12 at 00:15
  • Premature optimization is the root of all evil. -- Dr. Donald Knuth. Although Dr. Knuth was referring to performance optimizations, the same can be said of coding practices that add clutter without adding value. – Peter Gluck Aug 08 '12 at 00:23
  • @PeterGluck I read that as: a) limit scope until you need to increase it - b) don't put setters, until you need them - c) make fields final, until you need to change them? – assylias Aug 08 '12 at 00:26
  • @MattBall What are you using as a definition of a POJO. Martin Fowler talks specifically about putting business logic in them http://www.martinfowler.com/bliki/POJO.html , but you appear to be talking about something similar to C++ PODS. – Tom Hawtin - tackline Aug 08 '12 at 02:33
  • @TomHawtin-tackline I used "POJO" in the "bag-of-data-fields-with-getters-and-setters" sense, as frequently seen in [anemic domain models](http://martinfowler.com/bliki/AnemicDomainModel.html). – Matt Ball Aug 08 '12 at 03:08
  • @MattBall That link. He says "The fundamental horror of this anti-pattern [Anemic Domain Model] is that it's so contrary to the basic idea of object-oriented design; which is to combine data and process together." He goes on "Some technologies encourage it [the Anemic Domain Model]; such as J2EE's Entity Beans which is one of the reasons I prefer POJO domain models." Fowler is saying the opposite to you. – Tom Hawtin - tackline Aug 08 '12 at 04:11
  • @assylias Yes, I would say that's correct, and in keeping with the [SOLID OOD principles](http://en.wikipedia.org/wiki/SOLID_%28object-oriented_design%29). – Peter Gluck Aug 08 '12 at 16:51

8 Answers8

7

This is a loaded question.

Unfortunately, good (or bad) design is 100% dependent on how it is going to be used.

As a rule of thumb, it is a good idea to keep member variables private, just so that the Object model is in control of their access. This implies the first approach is better.

BUT

if the values never change, what's the point? Why bother writing setters if they will never be used?

So, which one is better? As I mentioned above, that depends on what you are doing this for. If it's for an assignment for class, I would go with the first one. Your teacher will like that more, as it is more "textbook".

So, if this is a personal project or for work where you can take advantage of future releases, I would go with a cross between the two:

public class Student{
    private final String name;
    private final String id;

    public Student(String name, String id){
        this.name = name;
        this.id = id;
    }

  ... getters ONLY for both fields

This approach is safe, because the member fields are private, and there isn't the "code smell" of unused methods. This is also fairly extensible, as you can quite easily add the setters if your requirements ever change and you need to modify the fields.

edthethird
  • 6,263
  • 2
  • 24
  • 34
6

You should always aim at limiting mutability and scope to their strict minimum. In your case: make the fields private, final, with getters, no setters.

Bonus: your class then becomes immutable and thread safe.

See also this post.

Community
  • 1
  • 1
assylias
  • 321,522
  • 82
  • 660
  • 783
2

Getter and Setter make API more stable. For instance, consider a field public in a class which is accessed by other classes. Now later on, you want to add any extra logic while getting and setting the variable. This will impact the existing client that uses the API. So any changes to this public field will require change to each class that refers it. On the contrary, with accessor methods, one can easily add some logic like cache some data, lazily initialize it later. Moreover, one can fire a property changed event if the new value is different from the previous value. All this will be seamless to the class that gets value using accessor method.

ThePragmaticProgrammer recommends always using getters and setters too, because it is the more general interface. Check this article http://c2.com/cgi/wiki?AccessorsAreEvil it's very nice and gives deep understanding on why we should use them.

Manisha Mahawar
  • 627
  • 6
  • 9
0

Of course if you know they'll never change, then you don't need accessors.

But if you told me that you were certain they'd never change, I wouldn't believe you.

Sean
  • 1,668
  • 1
  • 18
  • 28
0

If the fields are final, then you really don't need to use getters/setters. A setter would also be pointless since they will never change.

Alex Fischer
  • 545
  • 3
  • 11
0

Consistency rules. If you have some classes/properties with public access to variables whilst others have "getters", it isn't going to be a nice place to be.

Say you take your Student class and add a java.util.Date fields.

public final Date dateOfBirth;

Argh, now you Student is mutable in an uncontrolled manner. We need.

private final Date dateOfBirth;
...
public Date dateOfBirth() {
     return new Date(dateOfBirth.getTime()); // Awful API.
}

In general, just make every (instance) field private (exception for Data Transfer Objects). If you have setters, then you've probably abandoned all hope for encapsulation. Getters are fine for immutable value objects. ("get" prefix is a convention (mostly) for Java, but it's largely noise, particularly in code that favours immutable value objects and tell-don't-ask interfaces. If you remove the "get" the client code only has the extra "()" as verbosity over direct field access.)

Tom Hawtin - tackline
  • 145,806
  • 30
  • 211
  • 305
0

There's some good answers out there, and I agree with making fields private and adding appropriate constructors/getters/setters to limit mutability where needed.

I will point to another drawback of using field based assignment. Any proxy based AOP will have trouble adding pointcuts to field assignments, so it would make it difficult to do logging (and other operations) via proxy based AOP constructs, as they are limited to method interception. You'd be forced most likely into actually doing weaving, which becomes more complicated. From Spring's own documentation:

Spring AOP currently supports only method execution join points (advising the execution of methods on Spring beans). Field interception is not implemented, although support for field interception could be added without breaking the core Spring AOP APIs. If you need to advise field access and update join points, consider a language such as AspectJ.

Matt
  • 11,523
  • 2
  • 23
  • 33
-1

I don't think this is a bad idea.

If they aren't going to change then make sure you mark the fields with the appropriate keywords eg Final.

It also a good idea to make non changing values appear differently. eg Constants in Java are typically done in all caps.

mikek3332002
  • 3,546
  • 4
  • 37
  • 47