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.

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

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.

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.

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.

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.

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.