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?

7
  • are you asking specifically about java, or about OO programming in general? Apr 5, 2016 at 12:55
  • That's an old article with a deliberately inflammatory title. Don't take it seriously. No one does.
    – duffymo
    Apr 5, 2016 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. Apr 6, 2016 at 2: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 6, 2016 at 2:50
  • @BryanOakley I work with C++, but the question was asked w.r.t general OOP. Apr 6, 2016 at 4:52

4 Answers 4

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.

13
  • 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. Apr 5, 2016 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 5, 2016 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, Apr 5, 2016 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 5, 2016 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 5, 2016 at 15:00
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.

0
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.

8
  • 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 5, 2016 at 12:50
  • The other argument is that getting 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 5, 2016 at 12:53
  • 1
    @AndresF.: So what you actually mean is Java is evil?
    – JacquesB
    Apr 5, 2016 at 13:01
  • 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 5, 2016 at 14:11
  • 1
    of course that isn't to say that they are evil or shouldn't be used
    – sara
    Apr 5, 2016 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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.