13

I am programming a game in java, and as the question title suggestions i am using public fields in my classes. (for the time being)

From what i have seen public fields are bad and i have some understanding why. (but if someone could clarify why you should not use them, that would be appreciated)

The thing is that also from what i have seen, (and it seems logical) is that using private fields, but using getters and setters to access them is also not good as it defeats the point of using private fields in the first place.

So, my question is, what are the alternatives? or do i really have to use private fields with getters and setters?

For reference here is one of my classes, and some of its methods.

I will elaborate more if needs be.

public double health;
//The player's fields.
public String name;
public double goldCount;
public double maxWeight;
public double currentWeight;
public double maxBackPckSlts;
public double usedBackPckSlts; // The current back pack slots in use
public double maxHealth; // Maximum amount of health
public ArrayList<String> backPack = new ArrayList<String>();

//This method happens when ever the player dynamically takes damage(i.e. when it is not scripted for the player to take damage.
//Parameters will be added to make it dynamic so the player can take any spread of damage.
public void beDamaged(double damage)
{
    this.health -= damage;
    if (this.health < 0)
    {
        this.health = 0;
    }
}

EDIT: For checking purposes, this is what my Weapon class looks like now: (Code sample is not working for some reason, so it does not look right.)

private final double DAMAGE;
private final double SPEED;

public Weapon(double initialDmg,double initialSpd,String startName,double initialWg)
{
    DAMAGE = initialDmg;
    SPEED = initialSpd;
    setItemName(startName);
    setItemWeight(initialWg);
}

public double getSpeed() 
{
    return SPEED;
}


public double getDamage()
{
    return DAMAGE;
}

As you can see, as the Weapon's DAMAGE and SPEED do not need to be changed, they can be final's for the time being. (if, later in the game, i decided these values can be "Upgraded" so to speak, i may add setters then , with validation, or just make a new weapon with the upgraded values) They get set in the Weapon's constructor.

Conclusion: getters and setters are fine, as long as they are used smartly, and only used when needed. (however)

reevesy
  • 3,452
  • 1
  • 26
  • 23
James
  • 215
  • 1
  • 2
  • 7
  • Ok thanks for the help everyone, getters and setters it is. (which means quite a bit of refactoring for me, but ah well, my fault) – James Jan 01 '11 at 15:45
  • 7
    Setters and getters are only a very little improvements over public fields. They are a sure sign of missing abstraction and [quasi classes](http://www.idinews.com/quasiClass.pdf). – sbi Jan 01 '11 at 20:42
  • That is the question i asked, if they are little improvement over public fields, what is the alternative? , everyone here says getters and setters are the way forward but not all the members have to have them if those getters and setters are never used or if quite a few of them only get and set a value. (as apposed to setting value and validating it.) – James Jan 02 '11 at 13:28
  • On another note, i plan to add many more things to this game (it is certainly not finished) which may combat weapon and armour being nothing more then a data store at the moment (e.g. Some weapons could jam, armour could become damaged and less effective for every hit until reparied etc) – James Jan 02 '11 at 14:58
  • 2
    @James: (You need to properly @address comment replies, so they show up in our Responses tab.) What can I say that isn't said so much more eloquently in that article? The alternative is to raise the abstraction level, so that access to the members isn't needed anymore. A class is some internal (aka `private`) state plus `public` methods to manipulate that state. If users of a class need to directly access the class' state, this is actually _Structured Programming_ in disguise. Sadly, the Java/C# crowd, feeling so much superior over C++ for being pure OO, seemed to have completely missed this. – sbi Jan 02 '11 at 21:33
  • @sbi (thanks for that, i did not know this was needed) on another note, i have looked at that article, and it was somewhat not so helpful, i get what it means but i am improving my code to make sure my classes are needed. Thanks for your comments, but it seems most everyone else disaggrees with you. – James Jan 02 '11 at 21:57
  • 1
    @James: That "most everyone" is most everyone in this `java` tag you posted this in. (Which means I only ran into it by accident, BTW.) As I said, this is something the Java crowd invented and which, sadly, spread to the point where, in C#, people believe they're doing pure OO when they add public properties instead of public data members. Had you posted this in the `c++` tag "most everyone" would have agreed with me. (Well, I hope so, anyway.) – sbi Jan 03 '11 at 06:39
  • @sbi Very well, its just that i wanted you to explain, not some article to do it for you. Everyone else that said I should use getters and setters has explained why I should use getters and setters in their own words, but not to just blindly use them, and I should be smart in how I use them. I am sorry, I too now just seem to disagree with you now, but i respect your point of view. – James Jan 03 '11 at 14:50
  • @sbi I was going to edit the comment above but it won't let me now, so let me reiterate what i would actually like from you. (if you don't mind and have the time) Please explain why i should not use getters and setters in words that i can understand,can understand why i should use an alternative to getters and setters, (Even if it is public classes, as long as you can qualify it) beacause simply put at the moment more people say use getters and setters then not, and yes, i will put in the C++ tag so that we may get some other people that agree with you and can help qualify your side of this. – James Jan 03 '11 at 15:00
  • on another note it seems that this also says what people here are saying quite nicely: (The top answer) http://stackoverflow.com/questions/565095/java-are-getters-and-setters-evil – James Jan 03 '11 at 15:30
  • On a final note,it was bad of me to use only a java tag, but then it would have been equally bad of me to use only a c++ tag, i have tried to rectify this and i will be aware of this for the future, or use more general tags, as this applies not just to java i presume. – James Jan 03 '11 at 16:07
  • 2
    @James: If you have algorithms that grab into objects' innards to read and write state from/to those objects, then that, basically, is _Structured Programming_. _SP_ organizes code into __data structures__ and __algorithms__ operating on them. The algorithms operate directly on the values stored in the data structures. _OOP_, OTOH, has _encapsulation_ as one of its cornerstones. That means that you aren't supposed to even _know_ the internal representation of an object's state, let alone fiddle with it. Instead you invoke methods on the object, which change its state in a well-defined manner. – sbi Jan 03 '11 at 20:08
  • @sbi Well that makes sense, thanks for clearing that up, that's what i endeavour to do as apposed to blindly using getx and setx methods, thank-you for your input. – James Jan 03 '11 at 21:31
  • @sbi, I also apologize for any offence caused and I am sorry if I came across as hostile, this was not intended. – James Jan 03 '11 at 21:44
  • @James: I took no offense. I was just as a loss as to what to answer without keeping repeating what was already said. Anyway, I'm glad you think what I said now makes sense. – sbi Jan 04 '11 at 10:49
  • 1
    I've just seen that coobird gave an excellent answer saying pretty much the same I did in these comments. – sbi Jan 04 '11 at 10:52
  • Indeed, both coobird's answer and Paul Tomblin's answer are good answers. – James Jan 04 '11 at 14:16

12 Answers12

21

It's common to use getters and setters instead of giving other objects permission to change your fields directly. That might not make any sense when you see that 99.99% of your getters and setters don't do anything except what you could have done with direct access to the fields. But what happens when you decide that when a player is damaged beyond a point, he drops half his inventory? Or you want to restrict how many backpack slots can be used by magical items? You either have to hunt down all the places in your code where you modify the fields, or, if you used getters and setters, you make the changes entirely in the class. That's the heart of object oriented programming - that you've encapsulated "knowledge" of what an object does within the object itself, not spread it out among all the objects that interact with that object.

Paul Tomblin
  • 179,021
  • 58
  • 319
  • 408
  • I would say that half of getters and setters written/generated are pointless (often not even called) However, its better to have too many that too few. IMHO. – Peter Lawrey Jan 01 '11 at 18:43
  • 1
    @Peter, I consider myself guilty of using the "Generate getters and setters" option in Eclipse far too often. I agree with you completely. – Paul Tomblin Jan 01 '11 at 19:20
  • Me too, i have tried to avoid using the auto-generate in Eclipse, but where i have i have tried to remove all the get and set methods i don't need, and modify the ones i do need to make them smarter. (See my edit above) – James Jan 02 '11 at 13:57
  • 1
    @Peter: I consider this wrong. See my comments to the question. – sbi Jan 04 '11 at 10:50
  • 1
    "But what happens when you decide that when a player is damaged beyond a point, he drops half his inventory?" Then the method that sets the `health` member (which shouldn't be a `setHealth()` method) triggers a check for that. – sbi Jan 04 '11 at 10:55
  • 2
    A getter/setter (or adjuster in this case) is essential if you cannot trust the accessing code to do the right thing. e.g. if the code is used by another developer or team this is a good idea. If you are the developer in both cases, but you don't trust yourself to get it right, or to be able to fix it, use a getter/setter. ;) Personally I prefer to drop the getter if the field is immutable. Mutable fields accessed in another module are more likely to need getter/setters. If you have decent unit tests, you should pick up the sort of bugs which might occur by directly changing a field. – Peter Lawrey Jan 04 '11 at 17:04
18

One of the core concepts of object-oriented programming is encapsulation -- that is, hiding an object's state (for example, the data in the object) from the outside, and letting the object handle it's own state.

When encapsulation is done well, the object's state can only be affected from the outside world through the interfaces provided by the object, such as methods the object has.

I think your code is already starting to use encapsulation.

Let's take a look at the code

Let's take a look at the beDamaged method.

public void beDamaged(double damage)
{
    this.health -= damage;

    if (this.health < 0)
    {
        this.health = 0;
    }
}

Here's we can see that this method will be called by the outside world, and the player's health will be affected. It also contains logic, so the health cannot be a negative number. The player's beDamaged method that you wrote is keeping the state of the object within the parameters that you defined as being the valid state.

Let's infer something about the player

Now, from the above, I think I can infer the following about the player object:

A player's health cannot be a negative number.

Is what we inferred always true?

Let's see if this can always be true from the code you've provided.

Aha! We have a little problem here:

public double health;

With the health field being public, the outside world can directly manipulate the field in order to place the player object's state into one that is probably not desired, by some code like the following:

Player player = new Player();
player.health = -100

I'm going to guess that the player shouldn't be in a state where the health is a negative number.

What can we do about it?

How could that have been avoided? -- by having the health field private.

Now, the only way to affect the player's health would be through the beDamaged and gainHealth methods, and that's probably the right way for the outside world to affect your player's health.

Which also means this -- when you make a field private, that does not automatically mean that you should make getters and setters for the field.

Private fields does not necessitate getters and setters

Getters and setters are usually a way to directly affect a field that an object has, maybe with some validation to prevent bad input from making your object have a state that it shouldn't, but there are going to be times where the object itself should be in charge of affecting the data, rather than an outside entity.

coobird
  • 159,216
  • 35
  • 211
  • 226
  • Thanks for your answer and explaination too, i never thought about it like that, but can't you just use the set method to set it to a negative number, or should i validate the set method so you can't? (And yes you are right that the player's health can not be below 100) – James Jan 01 '11 at 15:38
  • 2
    "should i validate the set method" --> Bingo! That's an advantage you can get from using setters rather than public fields; an opportunity to validate your inputs and reject bad data to keep your object in an acceptable state. – coobird Jan 01 '11 at 15:40
  • Ah many thanks, i think i have not stated this but for reference i have programmed in VB6 for a few years now but have only just really started with java, so thank you for your logical explaination :) – James Jan 01 '11 at 15:42
  • Just wanted to say that this is a very thorough and good answer. Good job :) – Accatyyc Jan 01 '11 at 18:23
  • That's pretty much what I've been saying in comments to the question. Except for the very last paragraph (why would you need a setter for `health` if you have `beDamaged` and `gainHealth`?), I wholeheartedly agree. `+1` from me. – sbi Jan 04 '11 at 10:54
8

In Java, using private fields with getters/setters is the recommend practice, provided external clients of your class really need access to those fields.

Otherwise keep them as private fields and simply don't provide a getter/setter.

There are various reasons why this is a best practice:

  1. If clients are using your field directly and later something needs to change regarding that, you're stuck. With a getter you can do a whole lot of things before the field is accessed.
  2. There is something called the JavaBeans specification that requires you to use getter/setters. Without them your class (then called bean) won't interoperate with that. JSP and JSF's EL is one example of something that required your class to comply with JavaBeans standards.

(p.s. unrelated to your question, but you'd better not declare backPack as an ArrayList. Declare as List; code to interface, not to implementation)

Arjan Tijms
  • 37,782
  • 12
  • 108
  • 140
  • Thanks for you answer too, a bit more in depth but i do wonder what you meant by your p.s. , i.e. why not? – James Jan 01 '11 at 15:28
  • 1
    Your code only needs to know it's dealing with a List. It doesn't matter that it's an array list, tree list, linked list, whatever list. If your code doesn't depend on a a specific type of list, you can later very easily swap it for another list implementation. This makes your code easier to maintain and more robust in general. – Arjan Tijms Jan 01 '11 at 15:46
  • Ok thanks for that, so if i make it just a list, would i not need to refactor my code so that it can access the list correctly? (EDIT: i just checked it and it seems to be fine, thanks for that ) – James Jan 01 '11 at 15:53
  • Indeed, in 99.9999999% of the cases you'll see that nothing needs to be refactored after the change. This is thus the 'prove' that you didn't need the ArrayList at all, but only the List ;) – Arjan Tijms Jan 01 '11 at 15:56
  • Actually it seems i need to change a check as the list type does not have an "isEmpty()" method, which i require for a certain check. hmmm – James Jan 01 '11 at 16:01
  • 1
    You're mistaken, java.util.List definitely has an isEmpty method. See http://download.oracle.com/javase/6/docs/api/java/util/List.html#isEmpty() – Arjan Tijms Jan 01 '11 at 16:14
  • Oh, im using java.awt.list, should i be using that one? – James Jan 01 '11 at 16:38
  • No, that's something else entirely. java.util.List is the one. Check the ArrayList Javadoc, it implements this interface. – Arjan Tijms Jan 01 '11 at 16:43
  • Right, sorry ,was a bit confused as to which one it was :S – James Jan 01 '11 at 16:47
4

If you have a private field with a method get() and a method set() that don't do anything other than retrieve and assign the value, you should just make the field public, as the field isn't really private, and the getters and setters only hurt performance. If the getters and setters check the value being set or if the value is allowed to retrieve, then go ahead and use getters and setters. e.g. If you have a variable private int width; and someone tries to put in -1 with a setter, and the setter makes sure it isn't negative, then that is a good use. For example:

private int width;
public int get(){
    return width;
}
public void set(int w){
    if (w < 0) throw new RuntimeException();
    else width = w;
}

This would be a good use of getters and setters. Otherwise, they hurt your performance if the only thing they do is assign or get the value without anything else.

So to make a long story short:

Use getters and setters when doing anything other than retrieving or assigning a value. Else, just use public fields.

i.e.

BAD:

private int width;
public int get(){
    return width;
}
public void set(int w){
    width = w;
}

GOOD:

private int width;
public int get(){
    return width;
}
public void set(int w){
    if (w < 0) throw new RuntimeException();
    else width = w;
}

GOOD if you don't want anything other than getting or setting:

public int width;
Leo Izen
  • 4,165
  • 7
  • 37
  • 56
  • hmm, this somewhat contradicts what everyone else has said, but it makes sense, thanks for your answer too. – James Jan 01 '11 at 15:34
  • This practice works well only on programs which will be executed on a platform with *very limited* resources, like perhaps small devices. Otherwise, method call has a so small cost, that it isn't even worth considering in 99% of cases. So, if James is creating a game for something like a Java enabled phone, this answer is very useful. – Goran Jovic Jan 01 '11 at 15:39
  • 1
    Also, +1 for opposing the widely accepted practice that getters and setters shouldn't do anything other that setting and retrieving a field value. – Goran Jovic Jan 01 '11 at 15:40
  • Ok thanks for the explaination other people, you are most kind and helpful in your explainations. – James Jan 01 '11 at 15:43
  • @Leo I agree that getters and setters should *sometimes* be used for more than just reading and writing field values. But I disagree strongly with the statement that public fields should be used otherwise – Sean Patrick Floyd Jan 01 '11 at 15:47
3

About this:

The thing is that also from what i have seen, (and it seems logical) is that using private fields, but using getters and setters to access them is also not good as it defeats the point of using private fields in the first place.

The main problem is that many developers automatically generate getters and setters for all private fields. And if you're going to do that, I agree, you might as well keep the field public (no, public fields are even worse).

For every field that you have, you should check:

a) does it need a Getter (do other classes need to know the value of this field)
b) does it need a Setter (do other classes need to be able to change the value of this field)
c) or does the field need to be immutable (final), if so it must be initialized during definition or in the constructor (and it can obviously have no setter)

But you should hardly ever (exception: value objects) assume that all private fields will have getters and setters and let your IDE generate them all.

Sean Patrick Floyd
  • 292,901
  • 67
  • 465
  • 588
  • That seems to be the logical way of going about it, i will make sure i think before using getters and setters. – James Jan 01 '11 at 15:48
2

An advantage of using getters and especially setters is, that it is much easier to debug write access to the fields.

Mot
  • 28,248
  • 23
  • 84
  • 121
1

private fields and setters and getters is indeed your best way to go.

Further note that this is in general good code in any language as it keeps your security nice and tight while also giving you a structure that is far easier to debug and maintain. (Don't forget to document btw!)

All in all, go with setters and getters, it's just good practice even if you find options.

Mantar
  • 2,710
  • 5
  • 23
  • 30
  • by document, do you mean making a javadoc? – James Jan 01 '11 at 15:21
  • 1
    That and just plain commenting your code. You'll learn it yourself but I still want to stress the importance of commenting as much as possible. If you have codemonkey friends - Comment all your code so that they should be able to just get the code and understand it without talking to you. In a few years when you go back to your code without remembering it at all you'll thank yourself – Mantar Jan 01 '11 at 15:23
  • Ah, and don't worry, i know the importance of commenting, i am quite proficent in vb6 but have only really just got into java – James Jan 01 '11 at 15:26
  • Great. :) I just got a punch in the jewels by my own code yesterday for being lazy last year, as such, I try to inform everyone of the dangers. :P (Now back to finding out where the hell a[f].q goes) – Mantar Jan 01 '11 at 15:28
1

Getters and setters are part of the public interface of your class. It's a contract between the class designer/developer and the users of that class. When you define getters and setters, you should be committed to maintain them in future versions.

Attributes should only correspond the implementation of a given version of the class. In this way, the class developer may unilaterally change the implementation, hence the field, without breaking his/her commitment to maintain the interfaces.

Here is an example. Consider a class called Point. If you decide that a Point has x and y public attributes, then you may never change this. In contrast, if you have get/set X/Y methods, subsequent versions of the class may use various internal representations: rectangular coordinates (x, y), but also polar (r, theta), etc. All this without modifying the public interface.

ChrisJ
  • 5,161
  • 25
  • 20
1

A shorter version of your methods...

public void beDamaged(double damage) {
    health = Math.max(0, health-damage);
}

public void gainHealth(double gainedHp) {
    health = Math.min(maxHealth, health + gainedHp);
}

or even the following which can be called with +1 to gain, -1 to lose 1 hp.

public void adjustHealth(double adjustHp) {
    health = Math.max(0, Math.min(maxHealth, health + adjustHp));
}
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • 1
    er.. thanks for your comments, i do like having readability though :S – James Jan 02 '11 at 13:20
  • 1
    @James, once you get used to using functions like these, you might find this clearer. However, I suggest you stick with what you feel is clearest. – Peter Lawrey Jan 02 '11 at 17:37
  • Ok, thanks for that, and yea I may very well have to get used to functions like those, and i will look back and go "Wow, what I did in that old project was about as clear as dirt" :P but for now I will leave it as is. I say though, what is your opinion on getters and setters? – James Jan 03 '11 at 15:57
  • 2
    @James, getters/setters are useful and needed in cases where your object should/could contain some logic. They are less useful for data value objects. Some people over use them or insist you have to have them everywhere because one day you might use them. However, I believe that unused and untested code is worse than not having it. – Peter Lawrey Jan 03 '11 at 16:44
  • ok,Thank-you for your thoughts on the matter, this seems to be logical reasoning. – James Jan 03 '11 at 21:38
1

If you're not maintaining any invariants, then public fields are the way to go. If you do need an invariant across multiple members, then you need private fields and encapsulation.

But if you can't come up with any better names than GetFoo and SetFoo for the methods, it's a good clue that your getters and setters are probably worthless.

me22
  • 651
  • 3
  • 8
0

.... pathetic content omitted....

EDIT

sorry for beeing a little too pathetic -must be the pills... The other answers are quite relevant and good

mtraut
  • 4,720
  • 3
  • 24
  • 33
0

One advantage not yet mentioned for avoiding public fields: if there aren't any public fields, one may define an interface that includes all the public features of the class, have the class implement that interface, and then have everyplace that uses the class use the interface instead. If that is done, one may later design a class which has completely different methods and fields, but which implements the same interface, and use that class interchangeably with the original. If this is done, it may be useful to have the class implement a static factory method in addition to the constructor, and have the factory return an object of the interface type. Doing that would allow later versions of the factory to return an object of some other type. For example, one may come up with a low-cost version of the object in which many properties return constants; the factory could see if such an object would be suitable, and if so return one instead of the normal object.

Incidentally, the concept of using a mixture of constant and mutable objects in an adventure goes back at least to 1980. In Warren Robinett's "Adventure" cartridge for the 2600, each object has a number of pointers stored in ROM for things like position and state, so objects which aren't going to move (such as the castle gates or the "signature") don't need to have their position stored in RAM, and most grabbable objects (which don't have any state other than their position) won't need to store a state in RAM, but animated objects like the dragons and bat can store both state and position in RAM. On a machine with 128 bytes of RAM total, such savings were critical.

supercat
  • 77,689
  • 9
  • 166
  • 211