7

Which of the following is better? Is it even opinion-based or are there any relevant differences? Can one or the other be favored in some scenarios?

public class MyClass {
    private Integer myField;

    public void setMyField(Integer myField) {
        this.myField = myField;
    }

    public Integer getMyField() {
        return myField;
    }

}

I need a method to check wether something is allowed or not. Please, let's not talk about the sense of this code example. It's just a minimal example.

Implementation 1

public boolean isAllowed() {
    MyEnum.ALLOWED.getInt().equals(getMyField());
}

Implementation 2

public boolean isAllowed() {
    MyEnum.ALLOWED.getInt().equals(myField);
}

Edit: This post does not have an answer in the linked question (see comments to the initial post)

progyammer
  • 1,498
  • 3
  • 17
  • 29
Chris311
  • 3,794
  • 9
  • 46
  • 80
  • If you go by Java Encapsulation and Abstraction then I think first one is more preferable. As you are not directly exposing the fields. – SachinSarawgi Dec 06 '16 at 12:40
  • 1
    There are no difference, as far as meaning is concerned. It is more a matter of point of view. I'd choose the **implementation 1** as you'll be sure to get the value you wanna test, if you have, by example, any kind of transcodification – DamCx Dec 06 '16 at 12:44
  • 2
    You should give this a thorough read: http://stackoverflow.com/questions/1568091/why-use-getters-and-setters – denvercoder9 Dec 06 '16 at 12:44
  • 1
    @RafiduzzamanSonnet The accepted and most voted answer there is about using getters and setters *from other classes*. Here we have a case of using the value in the implementing class itself, which *is* already aware by definition of its existence and implementation. – RealSkeptic Dec 06 '16 at 12:48
  • 1
    @RealSkeptic It's the same concept. Otherwise there is no need for a getter/setter or public modifier – Murat Karagöz Dec 06 '16 at 12:52
  • 3
    @MuratK. The need for a getter/setter is for other classes, and that need is irrelevant. The question here is whether to use that getter/setter also for *internal* uses or use the field directly. – RealSkeptic Dec 06 '16 at 13:03

6 Answers6

12

Which of the following is better? Is it even opinion-based or are there any relevant differences? Can one or the other be favored in some scenarios?

It is question of good practice I think. The difference is in the readability of the code.

As a general rule, you should avoid indirection if not required. The current instance of MyClass has the information in one of these fields to implement the operation. It doesn't need to hide its internal state to itself.
So in internal, MyClass has no valuable reason to favor the use of the getMyField() over the direct use of the myField field.
The getMyField() accessor is more suitable to be used by clients of the class.
So I think that it is better in any case in your example code :

public boolean isAllowed() {
    MyEnum.ALLOWED.getInt().equals(myField);
}

Edit :
Beyond the readability, here is an example why you have no interest to couple the internal state to a public getter.
Suppose during the development phase you remove from the class the public getMyField() method because not need or not needed any longer for clients of the class, if isAllowed() relies on getMyField() in its implementation, it will be broken and you should replace it by myField.

davidxxx
  • 125,838
  • 23
  • 214
  • 215
  • This answer is slightly misleading because the example is overly simplistic and does not make semantic sense. The getMyField name does not convey any intention or meaning. Of the method were named "getSecurityLevel" it would better convey its intention. In reality you would never remove this method as it would change the semantics of the program. Instead you would alter the implementation of the getter to change the behavior of isAllowed. The principal to consider here is OCP. – Cliff Dec 06 '16 at 14:24
  • @Cliff Why do you want to make the question more complex as it is ? The question is: should I use the public getter or the private field from a method of the same class ? I didn't be specific but my example where I refer to removing a getter method refers to a removal during the development phase. Sometimes you provide getters in a automatically way for many fields (IDE helps that) of your class because you need or you think having need it at the time where you generate them and when you review the class at the end of your development, you notice that the getter should not be provided. – davidxxx Dec 06 '16 at 14:37
  • Anyway. the example may be indeed misleading. So, I updated it to specify the context. – davidxxx Dec 06 '16 at 14:48
  • I added an answer of my own explaining the issue from a use case. The problem is that it's difficult to judge or determine what code works best without meaning and context. I attempted to explain based on my experience rather than attempt to say which is better. In short, direct access leads to certain side effects that should not be ignored whereas practicing encapsulation leads to different experiences while adding slightly more effort up front. – Cliff Dec 06 '16 at 19:55
  • 1
    Also your original explanation looks at code for its implementation rather than its intention. Eg. removing the accessor method breaks the caller means you are focused on how its pieced together rather than focusing on the behavior you wish to elicit. It ignores the benefit of encapsulation and instead runs counter to this practice suggesting that encapsulation is counter-productive. – Cliff Dec 06 '16 at 20:01
  • 2
    I agree with you. In some cases, the OCP and more generally, a level of abstraction have to be used as we need to extend the initial behavior. But in a case where the variation is not foreseen and almost not possible, I think it's simply overkill. It's the same thing than creating interfaces in front of any class while the class has zero value to have an interface. A meaningful code is a code which reveals its intention. So, we should create abstraction only when it is required. – davidxxx Dec 06 '16 at 20:25
  • 1
    Otherwise, the intention is misleading since we wonder why we have this abstraction in the code while we notice that is not required. I am very pragmatic. A code lives and refactoring makes part of the life of the application. If a requirements changes totally and we should handle now a level of abstraction, refactoring is the way I follow and unit tests is my shield – davidxxx Dec 06 '16 at 20:25
  • 1
    Two things. I address the oversimplification vs overkill in my answer below while pointing out that it is difficult to know when decoupling is important without personal experience. Also, your explanation mentions "an example why you have no interest to couple the internal state to a public getter." and goes onto suggest direct access as an advantage. Decoupling should never be described as a detriment to design. That whole section is misleading and obscures any benefit of decoupling. While I understand your point it reads like good cohesion is a bad thing. – Cliff Dec 06 '16 at 20:36
  • I'm seriously not trying to be a jerk but I feel your answer would be better if it addressed both the pros and cons and steered clear of presenting either approach as inappropriate. The OP asks, "Can one or the other be favored in some scenarios?" Maybe explain why it is good. Also, your observation about readability could include an example to back it. Also I agree that you should avoid indirection but only up to the last step in red/green/refactor but that's an entirely different topic. – Cliff Dec 06 '16 at 20:45
  • We don't see the things with the same eyes. You try to address beyond the simple example. And it is an interesting point. I am stopped on the example : **"So I think that it is better in any case in your example code"** A jerk ? Don't worry. It is is a interesting discussion and you are respectful :) I don't need any longer to show why it is good because your answer address is very well. See you maybe in another discussion. – davidxxx Dec 06 '16 at 21:09
  • I removed my down vote. (I hardly ever down vote unless something really stands out.) Indeed we see things different. I'm usually the "glass is half clear, not empty or full" kind of guy. But then someone usually comes along and argues that it's half opaque. However I can't up vote with the answer sounding like decoupling is a disadvantage. – Cliff Dec 06 '16 at 21:09
7

My answer won't be the most informative however it will come from direct experience of dealing with this pattern. When designing an object it is often tempting to directly access member fields rather than rely on accessors. The desire stems from wanting to simplify the object and avoid adding clutter from methods that simple return a value. Taking your example a step further to add context & meaning:

public class MyClassmate {
    private Integer age;

    public MyClassmate(Integer age) {
        this.age = age;
    }

    public void setAge(Integer age) {
        this.age = age;
    }

    public Integer getAge() {
        return age;
    }

}

Here age is a simple number and it appears unnecessary to add getters/setters around it. If we add the following method you would be tempted to directly access the field since there is no change in behavior:

public Integer calculateScore() {
    if(age > 21) return 100 - getNumberOfIncorrectAnswers();
    //Add 10% before for younger students
    else return (100 - getNumberOfIncorrectAnswers()) + 10;
}

Your object may then grow new features with methods relying on the age field where you continue to use it directly. Later, you might alter the way age is originated and pull the value from across a network. You might not want to incorporate the networking logic in the constructor because it is an expensive operation which should only be triggered as needed. The calculateScore() method could make the network connection and discover the age but then too would all of the other methods that rely on age. But what if calculateScore looked as follows?:

public Integer calculateScore() {
    if(getAge() > 21) return 100 - getNumberOfIncorrectAnswers();
    //Add 10% before for younger students
    else return (100 - getNumberOfIncorrectAnswers()) + 10;
}

You could then enhance the object changing the way it derives age without touching the calculateScore() method. This means your method follows Open Closed Principle (OCP). It is open for enhancement but closed to change, or you didn't have to change the method source in order to change where it gets the age.

Depending on the complexity of your app and object model there may still be times when encapsulated access is overkill but even in these scenarios it is good to understand the tradeoffs of direct field access and these more simplistic scenarios are mostly rare.

In general you should understand that the need for encapsulation is almost never immediate. It appears over time as the object grows and if the object is not setup with encapsulation from its onset it is more expensive to phase it in. That's what makes this approach so difficult to appreciate. It takes experience (i.e. making the typical oversimplification and suffering several times over several years) to feel why encapsulation is necessary. It is not something you can eyeball and detect.

That said, this used to be a much bigger problem than it is today when IDEs were not as full featured. Today you can use the built in encapsulate fields refactoring in certain IDEs like IntelliJ to introduce the pattern as you need it. Even with modern IDEs it is still favorable to practice encapsulation from the onset.

Cliff
  • 10,586
  • 7
  • 61
  • 102
  • Interesting counter example. Age is maybe a too simple property for the example but good example all the same. – davidxxx Dec 06 '16 at 21:02
  • Yeah I cooked up the example as I was typing. Not the best to explain but crude way to introduce meaning and context to the original code otherwise it all starts to read like a series of mechanical steps rather than tell a story. – Cliff Dec 06 '16 at 21:12
  • @Cliff: In your example you are exploiting the getter. If the age is about to be retrieved from a network, one rather should utilize an assembler or a factory to create a `MyClassmate`. I don't think the logic should go straight into `MyClassmate`. Else it is no getter anymore but rather a `computeAge()`-method. – Chris311 Dec 08 '16 at 10:49
  • @Chris311 this example was not intended to be a full-fledged demonstration on how to do proper networking. I do understand it is contrived and not the best illustration of network code. Instead, the goal here is to highlight the advantages of proper encapsulation with regards introducing change. The point is that you can augment the behavior of a method without changing its code. This is something that becomes impractical when you use direct field access. – Cliff Dec 08 '16 at 16:35
  • True, and I really like your answer. I vote it up, although I still went with the "use members directly"-approach. – Chris311 Dec 09 '16 at 13:21
3

I would recommend using the getter because in certain scenarios, it can have some additional logic (like formatting, checking for nulls and so on). So you may be losing some logic when using the field itself.

Vaidas
  • 1,494
  • 14
  • 21
  • 3
    Isn't adding logic to the getter-setter methods considered a bad practice? – Aleksandr Erokhin Dec 06 '16 at 12:47
  • 2
    Getters should have no logic. – Chris311 Dec 06 '16 at 13:09
  • 5
    @AlexErohin and Chris311 - of course logic is allowed in getters. Otherwise the whole point of "hiding changes of implementation" would be lost. Suppose a field is now `int`. And you decide to change the implementation to an enum. The contract for the getter stays the same because that's published and used by other classes. So instead of having `return x;` in it, you have something like `return x.getIntValue()` - that's logic in the getter, and it's perfectly fine. – RealSkeptic Dec 06 '16 at 14:41
0

To keep a good encapsulation, the first think you need to think is which methods are you going to expose outside your class, if here, for example you are going to use only the is allowed method, I would make public only that method, and define the field in the constructor, if the field is suitable to change then you will need getter/setters but always depends on what do you want to offer from your class. And keep as much encapsulated as you can.

public class MyClass {
    private Integer myField;

    MyClass(Integer myField){
        this.myField = myField;
    }

    //Only this method is offered nobody need to know you use myField to say true/false
    public boolean isAllowed() {
        MyEnum.ALLOWED.equals(myField);
    }

}
cralfaro
  • 5,822
  • 3
  • 20
  • 30
-1

In my Software Engineering courses I was told to realize the "principle of secret". Thus, you should always use getter- and setter-routines. This way you can be sure that nobody accesses the member variable by accident.

Strange functions or objects may never see or even change member variables except you explicitly tell them to do so by setter and getters.

Kapa11
  • 311
  • 2
  • 18
-1

Due to your attribute being private, you can only securely access it within other class using getter or setter methods. So I would say that the best implementation is the one following the encapsulating principle, i.e., the one using the getter instead of accessing directly. This will prevent data leaks as well.

https://www.tutorialspoint.com/java/java_encapsulation.htm

  • 1
    My fields are encapsulated. The article does not speak of accessing the variables inside the same class. – Chris311 Dec 06 '16 at 12:50