2

http://www.javaworld.com/article/2073723/core-java/why-getter-and-setter-methods-are-evil.html

If you need to change the way the object is implemented in such a way that the type of X changes, you're in deep trouble.

If X was an int, but now must be a long, you'll get 1,000 compile errors. If you incorrectly fix the problem by casting the return value to int, the code will compile cleanly, but it won't work.

If that bold statement is true, that means IMO that design was not well thought off! Why should the "type" of a variable change suddenly in a program?

Is the presented case really viable?

It follows then that in OO systems you should avoid getter and setter functions since they mostly provide access to implementation details.

We can set conditions in the setter method that will prevent writing nonsense data to the field? Can't we?

If we are talking about "they mostly provide access to implementation details." then aren't we talking about protected and public areas? If we are, then these functions are supposed to be accessed by external classes. So, what is the problem here?

Why should a private variable have public getter and setter functions? Should it? If not then above argument is not applicable there?

Aquarius_Girl
  • 1,601
  • 2
  • 18
  • 25
  • are you asking specifically about java, or about OO programming in general? – Bryan Oakley Apr 05 '16 at 12:55
  • That's an old article with a deliberately inflammatory title. Don't take it seriously. No one does. – duffymo Apr 05 '16 at 20:12
  • 1
    @duffymo: Googling for the article's title gives multiple results, including several that are articles taking it seriously. And the fact that it's old is irrelevant. – Michael Shaw Apr 06 '16 at 02:49
  • It has not changed the way serious people write Java. Getters and setters in Spring. Q.E.D. The article had little or no effect. – duffymo Apr 06 '16 at 02:50
  • @BryanOakley I work with C++, but the question was asked w.r.t general OOP. – Aquarius_Girl Apr 06 '16 at 04:52
  • @duffymo I disagree. It has changed how I and many people write Java, and it (possibly) also pushed people to better languages. – Andres F. Apr 06 '16 at 23:18
  • I do not believe that anybody decided to use another language because of getters and setters. "Many people" - no data supporting that. Here's some data for you: Nobody is running away from Java. The JVM is more vital than ever: http://www.tiobe.com/tiobe_index – duffymo Apr 06 '16 at 23:23

4 Answers4

5

Getters and setters are not evil per se, they're evil when they're used for things they shouldn't be used for.

As there are cases when using a getter is inevitable, there are also cases when using the Tell Don't Ask principle is much better suited as a solution for the problem rather than a get* method.

Why are getters exactly frown upon then?

It's quite simple. They break encapsulation by giving you access to protected and private variables of a class, and encapsulation is one of the main benefits of OO design.

And when encapsulation is broken and getters are abused, you are very likely to switch from the object oriented design, where components are wired together, to the concept, where you tear an entire object apart by getting its values and doing operations based on these, otherwise private, values - you're back to procedural programming.

But as with everything, there are cases where getters are not only a good solution but actually the only one.

For example an entire ORM layer: abstracting pure database to objects, which simply need to have getters. They are simple data transfer objects whose sole responsibility is to deliver data, not to provide any logic. They provide data through getters and setters while usually also map their properties to concrete database attributes (columns if you wish). You use these very simple structures to build domain entities (domain models).

It is not recommended for domain models to have getters/setters

You should layer your application in a way where there is a layer of objects, which even though have getters, only use the getters to provide data for the presentation or persistence layers (that is if you want to display or persist a domain model, you extract its attributes using getters and forward them to the presentation or persisting layer respectively). In no way should the getter be used to pull a value out of the object and do an operation on it.

Take a look at this example (written in PHP, the language I am the most comfortable with):

class HarmlessMonster
{
    /** @var int */
    private $health;

    /** @var int */
    private $armor;

    /**
     * HarmlessMonster constructor.
     * @param int $health
     * @param int $armor
     */
    public function __construct($health, $armor)
    {
        $this->health = $health;
        $this->armor = $armor;
    }

    /**
     * @param int $health
     */
    public function setHealth($health)
    {
        $this->health = $health;
    }

    /**
     * @return int
     */
    public function getHealth()
    {
        return $this->health;
    }

    /**
     * @return int
     */
    public function getArmor()
    {
        return $this->armor;
    }
}

class Player
{
    /** @var int */
    private $damage;

    /** @var int */
    private $weaponWearPercentage;

    /**
     * Player constructor.
     * @param int $damage
     * @param int $weaponWearPercentage
     */
    public function __construct($damage, $weaponWearPercentage = 0)
    {
        $this->damage = $damage;
        $this->weaponWearPercentage = $weaponWearPercentage;
    }

    /**
     * @return int
     */
    public function getDamage()
    {
        return $this->damage;
    }

    /**
     * @return int
     */
    public function getWeaponWearPercentage()
    {
        return $this->weaponWearPercentage;
    }

    /**
     * @param int $weaponWearPercentage
     */
    public function setWeaponWearPercentage($weaponWearPercentage)
    {
        $this->weaponWearPercentage = $weaponWearPercentage;

        if ($this->getWeaponWearPercentage() > 100) {
            $this->setWeaponWearPercentage(100);
        }
    }
}

To make these classes somehow work, you need another class to provide the logic, a class like this:

class PlayerMonsterService
{
    public function attackMonsterByPlayer(Player $player, HarmlessMonster $monster)
    {
        if (($player->getDamage() * (100 - $player->getWeaponWearPercentage())) <= $monster->getArmor()) {
            $player->setWeaponWearPercentage($player->getWeaponWearPercentage() + 2);
            return;
        }

        $monster->setHealth($monster->getHealth() - ($player->getDamage() * (100 - $player->getWeaponWearPercentage()) - $monster->getArmor()));
        $player->setWeaponWearPercentage($player->getWeaponWearPercentage() + 1);
    }
}

You are completely tearing up the domain models, getting values of private attributes to use them in the setters, because there's a clear connection between them. And that's really ugly. As opposed to a Tell Don't Ask models might look like this:

class GameException extends RuntimeException { }
class ArmorTooHighException extends GameException { }

class HarmlessMonster
{
    /** @var int */
    private $health;

    /** @var int */
    private $armor;

    /**
     * HarmlessMonster constructor.
     * @param int $health
     * @param int $armor
     */
    public function __construct($health, $armor)
    {
        $this->health = $health;
        $this->armor = $armor;
    }

    public function reduceHealth($damage)
    {
        if ($damage <= $this->armor) {
            throw new ArmorTooHighException('Health was not recuded. Insufficient damage.');
        }

        $this->health -= ($damage - $this->armor);
    }
}

class Player
{
    /** @var int */
    private $damage;

    /** @var int */
    private $weaponWearPercentage;

    /**
     * Player constructor.
     * @param int $damage
     * @param int $weaponWearPercentage
     */
    public function __construct($damage, $weaponWearPercentage = 0)
    {
        $this->damage = $damage;
        $this->weaponWearPercentage = $weaponWearPercentage;
    }

    public function attack(HarmlessMonster $monsterToBeAttacked)
    {
        try {
            $monsterToBeAttacked->reduceHealth($this->damage * (100 - $this->weaponWearPercentage));
        } catch (ArmorTooHighException $e) {
            $this->increaseWeaponWearBy(2);
            return;
        }

        $this->increaseWeaponWearBy(1);
    }

    public function increaseWeaponWearBy($amountToBeIncreased)
    {
        $this->weaponWearPercentage += $amountToBeIncreased;

        if ($this->weaponWearPercentage > 100) {
            $this->weaponWearPercentage = 100;
        }
    }
}

You can see, that even though I am using private variables of both classes, there are no getters what so ever. Because you don't need them.

But even here remember, both the Player and HarmlessMonster classes would probably need getters, for the reasons said before, presenting the data to the user or persisting it. The point of this is not to use the getters to calculate values and based on these values do operations.

Domain layer is the layer which in an ideal case should follow quite strictly the Tell Don't Ask principle, having the tell methods, which throw exceptions on an invalid action. This puts all the logic related to the class an operation is run on to a very specific place and exactly where it belongs (usualy the class itself), rather than it being scattered across many places in your codebase.

I have found out, one of the reasons why some people prefer the getter/setter combination over the other approach is laziness, because coming up with the tell solution usually takes a few minutes longer than writing a simple get and doing with the value whatever they want. And it's not necessarily bad, if you don't mind the procedural approach, but if what you're after is OO design, avoiding getters unless you really need them is definitely a good way to do so.

Andy
  • 10,238
  • 4
  • 25
  • 50
  • 1
    +"They break encapsulation by giving you access to protected and private variables of a class,"_ - I don't think that's necessarily true. A getter gives you access to a public attribute. Whether that public attribute is directly related to a private or protected attribute is an implementation detail which exactly what the getter is hiding from the caller. This is precisely what getters are for: to provide a public interface to some data. _where_ or _how_ that data comes from is of no concern to the caller. – Bryan Oakley Apr 05 '16 at 12:14
  • @BryanOakley In practice, however, getters/setters in languages such as Java directly map to private fields almost 99% of the time. Which is why they break encapsulation and stray from the spirit of OOP. Now, you may argue "but that's bad programming", but I don't know if I'm convinced... are you going to argue almost all Java programs are badly written? – Andres F. Apr 05 '16 at 12:45
  • @AndresF.: it's not clear if the question is strictly about java. It starts by referencing an article that talks about java, but the question appears to be about general OO principles. To specifically address your question, if a program uses getters solely to access a private or protected variable, and that practice runs throughout the program, then yes, I would say it's badly written, – Bryan Oakley Apr 05 '16 at 12:54
  • @BryanOakley Agreed about the question not being about Java. The referenced article is, however (and do purer OO languages such as Smalltalk use getters/setters this way, anyway? Honest question here) If you work with Java, you'll agree with me this is how getters & setters are used in practice, common enough that we cannot argue it's just "bad programmers" that do it ;) Frameworks like Hibernate make this problem even worse. – Andres F. Apr 05 '16 at 13:17
  • @BryanOakley I provided an actual (sadly written in PHP but still an) example how the pure `get/set` approach differs from the tell don't ask principle I mentioned in my answer. – Andy Apr 05 '16 at 15:00
  • @AndresF.: The point of encapsulation is that the client does not know or care how a method is implemented. Even if a setter happen to map directly to a private field, the client does not know that. So it does *not* break encapsulation. – JacquesB Apr 07 '16 at 12:43
  • @JacquesB True, I phrased it poorly. It doesn't exactly break encapsulation in *theory* (though it does in *practice*, since the vast majority of getters directly expose mutable fields in Java. This is so common, novices may not even know other uses. The client may not care, but if they accidentally change the field, behavior may be unexpectedly broken. An example of accidentally breaking behavior is changing fields used to compute a hash code for an instance stored in a HashMap). And regardless of encapsulation, it definitely breaks principles from the OO paradigm (again, "Tell, Don't Ask"). – Andres F. Apr 07 '16 at 17:35
  • @AndresF.: In that case the error would be changing a field used for computing a hash code. This would be as bad whether it happened through a setter or any other reason. – JacquesB Apr 07 '16 at 17:56
  • @JacquesB In this case, yes, but I don't want you to focus on this example alone. Java is littered with this kind of pitfalls, in many cases encouraged by a proliferation of direct-access getters & setters. It's common enough that the overall lesson is: direct access to internal fields, which is almost always the case in Java, is an anti-pattern that leads to errors. This, of course, aside from the fact I've mentioned many times already, that it's against OOP principles. So getters/setters are evil for multiple reasons, some practical, some theoretical :) – Andres F. Apr 07 '16 at 19:48
  • @JacquesB A different, unrelated problem with getters/setters *as used in Java* is that they break the "atomicity" of the construction of valid objects. When you build (or update) an object by using setters, you often break its invariant. In principle it's better to only build valid objects using constructors, not by changing them using setters. In *practice*, this almost never happens with Java (and intrusive frameworks like Hibernate are partly to blame). The overall pattern that emerges is that getters/setters are evil for multiple reasons! – Andres F. Apr 07 '16 at 19:51
  • @JacquesB Finally, a nitpick: it's not true that in my example "it would be as bad whether [the change] happened through any other reason". The fact is that setters *as used in Java* provide uncontrolled access. *Without* the setter, access is limited to the instance -- which the class author knows how to handle responsibly. *With* the setter, access is granted to the caller, who may or may not know about sensitive details... and often doesn't!... which is how Java pitfalls are born :) – Andres F. Apr 07 '16 at 19:55
  • @AndresF.: Can you mention any feature in any language which cannot be misused? – JacquesB Apr 08 '16 at 09:10
  • @JacquesB Yes, many, but more importantly, that's not a good argument. Some features are prone to misuse and are considered bad design choices (as an example, see the infamous Billion Dollar Mistake). The widespread use of getters/setters in Java is one of them. Note that it's not that they "can be misused", but that they *often are*; when an error is widespread, it points to a design error in the language itself. And regardless, like it has been mentioned several times already, they are theoretically at odds with OO design (which is what this question was about). – Andres F. Apr 08 '16 at 11:59
3

Protecting a class from mis-use is only one reason for avoiding inspectors/mutators (getters/setters).

The other reason is to enforce cohesion and separation of concerns by making sure that behaviour which logically belongs to a class is defined in that class, and not elsewhere.

For example, consider the relationship between a Driver and their Vehicle; you would expect a DriveToTheShops() method to be in terms of things the Driver understands:

class Driver
{
    private Vehicle _vehicle;
    public void DriveToTheShops()
    {
        _vehicle.StartEngine();
        _vehicle.Accelerate();
        _vehicle.RotateSteeringWheel(90.0);
        _vehicle.RotateSteeringWheel(0.0);

        if(trafficLights.State == TrafficLight.Red)
        {
            _vehicle.ApplyBrake();
        }
    }
}

But if everything in Vehicle was exposed publicly, the DriveToTheShops() method might end up looking like:

class Driver
{
    private Vehicle _vehicle;
    public void DriveToTheShops()
    {
        // Start Engine
        do {
            _vehicle.StarterMotor.Enabled = true;
            _vehicle.CrankShaft.Turn();

            if (_vehicle.Engine.State == Engine.Off)
            {
                _vehicle.StarterMotor.Enabled = false;
            }
        } while(_vehicle.Engine.State == Engine.Off);

        // Accelerate
        var throttleAmout = _vehicle.Accelerator.throttleAmount
        _vehicle.Engine.IncreaseFuelFrom(_vehicle.FuelTank, throttleAmount);

        // Turn Right
        _vehicle.FrontLeftWheel.Angle = 90.0;
        _vehicle.FrontRightWheel.Angle = 90.0; 

        // Straighten up
        _vehicle.FrontLeftWheel.Angle = 0.0;
        _vehicle.FrontRightWheel.Angle = 0.0; 

        if(trafficLights.State == TrafficLight.Red)
        {
            // Apply Brake
            var brakeAmount = _vehicle.BrakePedal.BrakeAmount;
            _vehicle.FrontLeftWheel.Brake = brakeAmount;
            _vehicle.FrontRightWheel.Brake = brakeAmount; 
        }
    }
}

Firstly, that makes the job of the Driver a lot more complex - the Driver now needs to know how the vehicle's Engines, Wheels, Brakes, Fuel tank and Starter motor work. But a driver shouldn't need to know any of those things in order just to drive a vehicle.

Secondly, the lack of abstraction behind a clean interface means that if the Driver wants to drive a different type of Vehicle, the driver needs to know about the inner-workings of that particular type of Vehicle aswell.

Creating a different kind of Vehicle would involve some refactoring to move its behaviour into another class, because the logic is tied up in the Driver.

Ben Cottrell
  • 11,656
  • 4
  • 30
  • 41
2

They are not. The article you link is based on a major misunderstanding. What is frowned upon is exposing public fields, because this ties the clients to implementation details. Getters and setters are the solution for this problem, since you encapsulate the underlying implementation, and can change it without changing the client. The article uses the argument against public fields and applies it equally to getters/setter, but this misses that the whole point, that getters/setters encapsulates the implementation, and you can change the underlying representation without changing the interface.

Obviously you shouldn't use getters and setters to expose implementation details - but this is the same for any method. Getters and setters can be used for exposing data which should really be encapsulated, but you can do the same with any ordinary method., We don't say methods are evil because they can be misused.

JacquesB
  • 57,310
  • 21
  • 127
  • 176
  • I think the article has a very good point. In practice, almost all uses of getters/setters in Java directly expose private fields. They allow you to modify the internals of a class in indirect ways, and also allow you to break invariants by constructing invalid objects. Special care *may* be taken to avoid these situations (defensive copying, verification of invariants before each operation), but *in practice* this is *never* done, which makes the widespread use of getters/setters evil. Some frameworks such as Hibernate need this to work, which makes the situation inescapable in Java... – Andres F. Apr 05 '16 at 12:50
  • The other argument is that **get**ting things is not the right design in OOP (the famous "Tell, don't ask" principle). But I won't press this, since almost nothing in Java is true OOP, but a hybrid of procedural+OO which was born with C++ and popularized with Java, I guess. – Andres F. Apr 05 '16 at 12:53
  • it's not a misunderstanding. the issue isn't necessarily that a getter corresponds to a certain field, the issue is that you design classes that you need to query for their state in order to use. this is the opposite of OOP. in OOP you define behavior inside the class and expose some operations that object can do. you don't poke a bunch of holes that let you peek into it's state and then do stuff yourself depending on what you see. – sara Apr 05 '16 at 13:00
  • 1
    @AndresF.: So what you actually mean is Java is evil? – JacquesB Apr 05 '16 at 13:01
  • @JacquesB That could be argued, yes :P But more seriously, I think many of its features and the style of programming they suggest (if not directly enforce) go against OOP principles. Then again, I'm not married to OOP, but regardless of the paradigm I don't think Java as usually written leads to good, clean code. Of course, good programmers will use Java in clean ways, but what about the average programmer? – Andres F. Apr 05 '16 at 13:15
  • @kai: Data transfer objects are a totally legitimate pattern. Say you have a GUI form which displays and updates an underlying model. Getters and setters are a totally fine to get/update data here. But yeah, you shouldn't blindly use getters/and setters where another approach is more appropriate, but this is true for any feature or pattern. – JacquesB Apr 05 '16 at 13:18
  • 1
    DTOs aren't objects in the strict OOP sense though. they are data structures (similar to classical C structs). they don't model an entity in the domain, they are just a way to group data with no behavior. – sara Apr 05 '16 at 14:11
  • 1
    of course that isn't to say that they are evil or shouldn't be used – sara Apr 05 '16 at 14:11
1

I remember some pretty heated arguments back when this came out. This article was always very controversial in the Java community. Personally, I think the article is one of a large set of articles about design where the author tells you not to do things a certain way but provides very little detail on how to do it differently. I would recommend taking such articles with a grain of salt. It's not always clear that these authors have actually built real systems of significant complexity.

Having said that, getters are setters are not 'evil' per se (for some reason there was a number of why X is evil articles around that time) but there is one problem with their use that I think is very problematic. To explain this, we have to go back to the way things are done in a pure procedural language. In that world, you create data records and then you write methods that operate on this data. Essentially, you pass pointers/references to those data records to the methods where you write the logic.

One of the main concepts of OO is to turn this on it's head. Instead of passing the data to the method, the method is conceptually bound to a specific record. This combination of methods and data is called an object. The big advantage of this is that be keeping the data encapsulated you can control how the data is managed and understand that without looking through out the entire program for anything that might be touching it.

The problem with getters and setters that I've seen is that often developers will create a class, declare a bunch of variables, generate getters and setters for every variable in the object and then put all the interesting logic elsewhere. Fundamentally, this kind of program structure looks just like the pure procedural approach. The getters and setters are simply a facade over a behavior-free record.

It's definitely better to have the getters and setters than to just expose the variables. It at least provides a layer of abstraction that you can use if you need to change things later but you won't really get the value of OO without taking the logic specific to that data and moving it to the class.

I'll provide an example that should hopefully provide some clarity here. NOTE: This is terrible design. Don't do this:

class ImExposed
{
   private int[] values = {};

   public int[] getValues()
   {
       return values;
   }

   public void setValues(int[] values)
   {
       this.values = values;
   }
}

class Calculator
{
    public static long sum(ImExposed data)
    {
        long total = 0;

        for (int value : data.getValues()) {
            total += value;
        }

        return total;
    }
}

class ValueUpdater
{
    public static void add(int value, ImExposed data)
    {
        int[] oldValues = data.getValues();
        int[] newValues = new int[oldValues.length + 1]

        for (int i = 0; i < oldValues.length; i++) {
          newValues[i] = oldValues[i];
        }

        newValues[oldValues.length] = value;

        data.setValues(newValues);
    }
}

Hopefully the problems with this are apparent. Even though you've 'encapsulated' the data with getters and setters, it's a facile abstraction. In order to understand how the data values are being updated and accessed, you need to potentially search through the entire codebase. And because the updates and reads to this array are not being coordinated by the owner class, there are innumerable ways this code could fail and it's very hard to know if your code will work properly. Every class that interacts with the data array needs to do it the right way. Trying to make the above work properly with concurrency is beyond hopeless.

class NotExposed
{
   private List<Integer> values = new ArrayList<>();

   public int[] getValues()
   {
       return list.toArray(new int[list.size()]);
   }

   public void add(int value)
   {
       values.add(value);
   }

   public long sum()
   {
       long total = 0;

       for (Integer value : list) {
           total += value;
       }

       return total;
   }

}

Don't get hung up on the implementation here. The point is that I've moved all the logic to the class. I keep the list private so that I can now control how it's modified and accessed. Now, if I need to add concurrency support, I can do it in one spot. I can change the implementation of how the data is stored too and none of the other classes in the application need to change.

JimmyJames
  • 24,682
  • 2
  • 50
  • 92