Avoiding getters and setters: what is wrong with my design?

Question

I have a class that wraps a map UI element: Map.

The map can have overlays that appear on top of it, in both side panels and modal windows. When these UI elements appear, I want to temporarily disable map interaction and prevent multiple overlays appearing on top of each other.

Therefore, I inject an interface, IMap, of Map into Modal and SidePanel classes, like this:

class Map {
 private _isDisplayingOverlay : boolean = false; 
 public get isDisplayingOverlay() {
 return this._isDisplayingOverlay;
 }
 
 public set isDisplayingOverlay(newValue:boolean) {
 this._isDisplayingOverlay = newValue;
 }
 //Other methods here ...
}
class ExampleModalWindow {
 
 constructor(IMap) {}
 public display() {
 if (this.IMap.isDisplayingOverlay) {
 return; // exit early to prevent overlapping overlays.
 }
 this.IMap.isDisplayingOverlay = true;
 //UI logic here
 }
 
 public hide() {
 this.IMap.isDisplayingOverlay = false;
 //UI logic here
 }
 
 
}

I've read that getters and setters are evil and break OOP, so I assume that there is something wrong with my design. What am I doing wrong? How can I avoid using getters and setters in this example?

Edit: Added images to clarify the layout. This image shows the different components and their relationships on the UI. The blue rectangle is the size of the map when the sidepanel is hidden. The red rectangles are drawn around overlays (which are both sidepanels and modal windows). The map is not displayed within a modal itself, but has modals drawn on top of it.

Image showing different components and their relationships on the UI

This second image shows the approximate size of modals, if they were allowed to be shown at the same time as the sidepanel. Second image showing approximate size of modals

• Eoin: Hi @DocBrown, I've added a couple of images to clarify. I cannot show screenshots of the application itself as it doesn't belong to me, but the UI is very similar to Google Maps. Hopefully the images above clarify the question.
• Filip Milovanović: "getters and setters are evil" sounds a bit too dogmatic; the problem they indicate is that you're grabbing data from other components on other to do something yourself (procedural programming), instead of telling those components (via method calls) what you want them to do in a higher level way (OOP). This means that the relevant code that should be extracted out of the ExampleModalWindow into some other component (Map, or a new one) is precisely the detail-manipulating code that you omitted. That said, some objects are really just data structures, so sometimes getters & setters are OK.
• Filip Milovanović: @RobertBräutigam "data structures are the norm today" - the context was someone wanting to write good OOP code; "sometimes" wasn't meant as a statement about the frequency plain data structures in most code today. "Sometimes == almost never in applications" - I disagree, there's plenty of opportunity to use behaviorless data structures at the boundaries of applications, or you can mix paradigms if it fits the problem better. The way to go is not to be dogmatic, it's to develop a deep understanding of design, of the pros and cons of certain design choices.
• IMSoP: @RobertBräutigam You missed the second half of that sentence: "develop a deep understanding of ... the pros and cons". So, absolutely, if the best argument anyone has against your design is "it doesn't follow this dogma", then you can argue back; but if they say "that mixes responsibilities, which makes these scenarios harder...", they're applying the principles reasonably. The same applies to getters and setters: they might be a "code smell" to check if the design is appropriate, but you might look at the design and realise that a plain data structure is actually appropriate.
• IMSoP: @RobertBräutigam I wasn't attacking your understanding, just pointing out where your straw man argument fell down, in a hypothetical conversation between two developers. Maybe in an ideal world even a basic data type like Address would encapsulate behaviour, but creating a plain data object with getters and setters will be better than falling back on a built-in type like string or list<string>. Discussing why the rule is there might help someone build a better Address type; enforcing it blindly might lead them to not create one at all.

Answer 1

Depending on the context, there may be several solutions. I will list 2:

1. Keep the boolean flag, but have the map wrap any overlays. Like this:

private _isDisplayingOverlay : boolean = false; 
public overlay(UIComponent component) : UIComponent {
 // Wrap "component" with another one that sets/resets flag
 // on display()/hide()
}

Then create all UI Components that are overlayed through the Map.

2. Extract the positioning info (the flag whether something is overlayed) into it's own thing. For example a Screen, or Page, etc. Have this object supplied to the display() and hide() methods. Something like this:

// In Map
public display(screen: Screen) {
 if (!screen.isThereAnOverlay()) {
 ...
 }
}

Again, it all depends on your context, there are probably other solutions too.

• Eoin: Thank you @robert. In my case, option 1 is more practical.
• Doc Brown: I think your solution #2 has a good start (moving the responsibility for the overlay visibility away from the Map into something like a Screen), but then the proposed solution is introducing a unnecessary dependency from Map to Screen. Why not simply give display a boolean parameter and let the caller pass screen.isThereAnOverlay() into it?
• IMSoP: The second option here is still basically using a getter, with the problems that brings: why should other components know this specific thing about the Screen's state? That should be internal knowledge, and the component should call screen.canDisplay(this), or even screen.tryDisplay(this), leaving the logic of what components can co-exist encapsulated in the Screen class.
• Robert Bräutigam: @IMSoP Yes, the second solution is not my favorite, it's just an illustration of what's possible. Since components are displayed on screens there is an argument to be made that they should know about the screen. They will probably have to know all sorts of things about it to display correctly. That's all an assumption, I didn't even know it's a web-based app at that time. And also, you don't necessarily need to pass this, as the Screen received could be a specific instance for this very component.
• IMSoP: @RobertBräutigam The problem is not with the component knowing about the Screen itself, it's with it knowing, and having responsibility for, the specific states that Screen can be in. The responsibility for "only showing one overlay at a time" needs to be entirely in the Screen class. See my answer for one way this can work (although I kept with Map as the owner rather than introducing a new Screen class, for simplicity).

Answer 2

I'd say you're thinking about this all wrong. The map doesn't need to be aware that there's an overlay preventing it from being interacted with. The modality of the overlay is a characteristic of your UI as a whole, not of the map component.

One way to keep the map component agnostic of the overlay would be to display your overlay on top of another transparent full-screen overlay that intercepts any interactions with the map.

As for getters and setters being evil, I don't know where you heard that. Getters and setters are the bread and butter of encapsulation.

• Eoin: This ties in with @gnasher729 's points about strange logic. In this case, the getters and setters were a symptom of something being wrong. But it does seem to be a controversial subject. Searching google for "getters and setters are evil" returns articles like this: infoworld.com/article/2073723/…
• Kevin Krumwiede: @Eoin That article's argument seems to be a lot more subtle than its title suggests. It's not that getters and setters are evil; getters and setters are the correct way to expose properties. It's that having a lot of properties is evil, or at least a code smell, since OOP is really about modeling behavior, not just passing around bags of properties.
• IMSoP: The argument goes something like this: Good encapsulation hides state, and makes it a side-effect of behaviour. Getters and setters expose state, and make behaviour a side-effect of modifying it.
• Eoin: @KevinKrumwiede My takeaway from the article was that the ideal program would have absolutely 0 getters and setters, with the caveat that in some cases they are unavoidable. In my example above, it turns out they were indeed avoidable and a code smell.

Answer 3

The idea that getters and setters are "evil" or at least a "code smell" is based on the principle that an object should hide its state as an implementation detail of its behaviour.

Let's look at some problems with your current code, and how thinking about behaviour rather than state would help us resolve them:

• If we wanted to change the logic to allow, say, a sidebar on each side, every component that currently reads or writes the isDisplayingOverlay property would need updating.
• Each time we add a new type of overlay, we have to remember to include the isDisplayingOverlay logic, because there's nothing in the design to stop us missing it out.

We can solve the first point by replacing the direct get and set with a "lock" and "unlock" method. If we pass the component we're trying to display, we can use that to make more complex decisions in future:

class Map {
 private _isDisplayingOverlay : boolean = false; 
 public bool tryToClaimOverlayLock(IOverlay claimant) {
 if this._isDisplayingOverlay {
 return false;
 }
 else {
 this._isDisplayingOverlay = true;
 return true;
 }
 }
 
 public void releaseOverlayLock(IOverlay claimant) {
 this._isDisplayingOverlay = false;
 }
}
class ExampleModalWindow implements IOverlay { 
 constructor(IMap) {}
 public display() {
 if (! this.IMap.tryToClaimOverlayLock(this)) {
 return; // exit early to prevent overlapping overlays.
 }
 //UI logic here
 }
 
 public hide() {
 this.IMap.releaseOverlayLock(this);
 //UI logic here
 } 
}

This is better - we now only have one class that needs to manage the detailed state, and can change the implementation without changing everywhere that calls it.

But our second problem remains: the design lets us forget to claim the lock, or worse, forget to release it, because "claiming a lock" is still just a way of managing state, and is divorced from the behaviour of displaying the overlay.

The underlying problem is that we've given the display method too many responsibilities:

• Deciding whether to display something
• Knowing where to display it
• Knowing how to display it

What we really want is for the decision of whether to display the component to be the responsibility of the Map class, but at the moment it's spread across both classes. The same might be true for where to display it (again, imagine having both left and right sidebars).

So the behaviour we want to model is:

• The Map decides whether to display the component
• The Map decides where to display it
• The Map tells the component where to display itself
• The component decides how to display itself in that position

The result looks something like this:

class Map implements IRenderingTarget {
 private _isDisplayingOverlay : boolean = false; 
 public bool displayOverlay(IOverlay overlay) {
 if this._isDisplayingOverlay {
 return false;
 }
 else {
 this._isDisplayingOverlay = true;
 overlay.renderAt(this);
 return true;
 }
 }
 
 public void hideOverlay(IOverlay overlay) {
 overlay.removeFrom(this);
 this._isDisplayingOverlay = false;
 }
}
class ExampleModalWindow implements IOverlay { 
 public renderAt(IRenderingTarget target) {
 //UI logic here
 }
 
 public removeFrom(IRenderingTarget target) {
 //UI logic here
 } 
}

Where previously we would have written this:

myModal = new ExampleModalWindow(myMap); 
myModal.display();

We would now write this:

myModal = new ExampleModalWindow(); 
myMap.displayOverlay(myModal);

We now have code that:

• Is shorter and less repetitive (but may seem more complex at first glance)
• Has natural places to change each part of the behaviour
• Doesn't need us to remember to re-implement the same checks in multiple places, because we've modelled the purpose of those checks in one place
• De-couples the components into parts that can individually be tested, linked by purpose-specific interfaces.

Note: As others have pointed out, it may be that the Map should be a separate component, with responsibility for displaying the actual map data, panning and zooming, etc. In that case, the logic here would belong in a Screen or Layout class, and the map would communicate with it in the same way as the overlays.

• Caleth: I'd be tempted to change bool _isDisplayingOverlay to IOverlay _overlayTarget, so that hideOverlay can't do the wrong thing
• IMSoP: @Caleth Yes, that's actually a good example of why this re-factoring was worthwhile: to do that in the original code, we'd need to find everywhere that was managing the boolean flag, and update to the new behaviour, making sure we didn't miss anywhere or make a mistake. Now that we've encapsulated the state properly, that change can be made in the Map class without touching any of the other classes at all, and we can easily test that it works as expected (e.g. with a unit test).

Answer 4

Some people say: Getters and setters are evil. The most common reason why they say that is because they heard someone state this with a lot of conviction. But as we all learnt, stating something with a lot of conviction doesn’t mean you’re right.

There’s a real argument in Java, where instead of using a getter and setter you might as well have used a public member variable. There’s no difference really in you example.

Your logic seems a bit weird, I assume you pass the same map to multiple overlay windows? And it shouldn’t be the modal window’s job to decide whether to be displayed or not, that should be someone else’s. Either you have a map object that shows countries and overlays, or some other object showing a map and overlays, and they should control showing/hiding the overlay windows. But your setter/getter are not the problem.

• Eoin: It does seem to be a dividing issue. But I do agree with the sentiment that it probably means there is a problem with the design somewhere. I agree that the logic is quite strange, I will fix this. Both of @Robert's solutions above also eliminate this strange logic!
• Doc Brown: @Eoin: you already got two good answers, so I don't write my own, but let me add, I would probably replace Map._isDisplayingOverlay by a boolean flag like _isInteractionActive. This way, a Map only knows that it should allow interaction or not, but not that something like an "Overlay" even exists. Then something like a Screen object could control the activation of overlay and coupling to the interactivity of the map.
• IMSoP: It's best to understand an argument before disagreeing with it. The argument against getters and setters has nothing to do with the availability of public properties in a language, but to do with the idea that objects should expose behaviour rather than state. So in this view, explicitly reading or writing the isDisplayingOverlay property is indeed a sign that the objects are not interacting in a "good" way - which is a conclusion you came to as well, via different reasoning.

Answer 5

FYI – I calmly disagree with the notion that "getters and setters are evil." They are just procedure and function calls ... conveniently packaged. So, as long as you are aware that "this is actually what's taking place whenever you use them," they surely are convenient.

• IMSoP: As with other answers and comments, this seems to be based on a misunderstanding of what is being considered "evil" (probably too strong a word): it is not about the technical details of methods Vs properties, it's about the design of working with state rather than more abstract behaviour.