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.
ExampleModalWindowinto 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.Addresswould encapsulate behaviour, but creating a plain data object with getters and setters will be better than falling back on a built-in type likestringorlist<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.