Law of demeter - using only one dot, could I improve this logic?

Question

I have the following method:

    private boolean reserveSeat(int selectedRow, int selectedSeat) {
    if (show.getRows().get(selectedRow).getSeats().get(selectedSeat).getReservationStatus()) {
        return false;
    } else {
        show.getRows().get(selectedRow).getSeats().get(selectedSeat).reserve();
        setRowNumber(selectedRow);
        setSeatNumber(selectedSeat);

        return true;
    }
}

which resides in a Reservation class. This class has a Show Object (show), A show has Rows (another object), Rows have Seats (another object).

My question is could this method be improved? I have read about LoD and worried that my dot signals a bad design though I think it is logical. It is the Seat object that knows if it is reserved or not. However is going from Show to Seat talking to strangers? or is it ok because of the way each object contains the next object?

Apologies if my questing is not clear. What seems to happen with me (probably because I am self taught) is I design stuff that works then I read some OOP design principles and think crap, it works but it is not good design!

Any advice appreciated.

You could create a `getRow` method that does `getRows().get(row)` in your Show class. and similarly a `getSeat(seat)` method in your Row class. You could even add a helper method in your show class: `show.getSeat(row, seat)`. — assylias, May 13 '16 at 09:33
@assylias thanks for responding. getRows is currently in my Show class (and getSeat is currently in my Row class), but I guess you are saying it should do the getRows().get(row) bit so that this is not done in my Reservation class, which makes sense. A helper method would be a good idea — Urban Gemz, May 13 '16 at 10:17

score 3 · Accepted Answer · answered May 13 '16 at 09:37

3

Yes, that chain of calls is way too long. If show is in charge of the seats, then it would be better if it's fully in charge. Right now it's not fully in charge, because seats can be reserved without the show's knowing. This fragmentation of responsibilities is strange.

You can put show fully in charge by not exposing Seat to the Reservation, by hiding the seat status manipulations behind helper methods:

private boolean reserveSeat(int selectedRow, int selectedSeat) {
    if (show.isSeatReserved(selectedRow, selectedSeat)) {
        return false;
    } else {
        show.reserveSeat(selectedRow, selectedSeat);
        setRowNumber(selectedRow);
        setSeatNumber(selectedSeat);

        return true;
    }
}

Or, if you don't want show to be in charge of the seats, then it should not be aware of the seats at all, so then you would access seats not through show, but another class that's in charge of that.

answered May 13 '16 at 09:37

janos

120,954
29
226
236

thank you for this very useful response. When you have the reserveSeat method in show class, is it ok for show class to call its Row object method which in turn calls the Seat objects method? i.e this part .get(selectedRow).getSeats().get(selectedSeat).getReservationStatus() – Urban Gemz May 13 '16 at 10:10
1

That's still not great. For example `Row` should not expose `getSeats` like that, but provide `getSeat(int)` accessor (replacing `getSeats().get(selectedSeat)`. You really want to reduce the long chained calls as much as possible, as explained on https://en.wikipedia.org/wiki/Law_of_Demeter – janos May 13 '16 at 10:22
thanks @janos, so to support your example I now have in show class `public boolean isSeatReserved(int selectedRow, int selectedSeat) { if (getRow(selectedRow).getSeat(selectedSeat).getReservationStatus()) { return true; } else return false; } public void reserveSeat(int selectedRow, int selectedSeat) { getRow(selectedRow).getSeat(selectedSeat).reserve(); }` and in Row class I have `public Seat getSeat(int selectedSeat) { return seats.get(selectedSeat); }` Is this what you mean? sorry having trouble formatting my code in the comment!! – Urban Gemz May 13 '16 at 15:37
Yes, that's better. Btw, instead of `if (x) return true; else return false;` you can write simply `return x;` – janos May 13 '16 at 15:47

Jorn Vernee · Answer 2 · 2016-05-13T10:10:43.363

You're using show as a data object, and putting all the logic for handling that data in the class that contains it. This makes Show a data class and the enclosing class a god class.

The logic for handling data inside of show should really be inside the Show class itself (data is smart).

You could make a method in the Show class for reserving a seat. And equally, you could make a method in the Row class for reserving a seat.

Then one just passes on the message to the next until you get to Seat.

What if you changed the implementation of Show to use a 2D array for instance? That would break the code in your reservation class.

By doing these long chained calls, and not letting classes handle their own data. You are making the user classes dependent on the implementation the used data structures.

If you wanted to change one, you would have to update all the user classes, instead of just the one class that contains the data structure.

Thanks for this, could you just explain to me what you mean when you say show is a data object? — Urban Gemz, May 13 '16 at 10:04
@UrbanGemz ```show``` is perfectly capable of doing the reservations itself, but you're not letting it, you're only using it for it's data. That's what I mean by _data object_. — Jorn Vernee, May 13 '16 at 10:15

score 0 · Answer 3 · answered May 15 '16 at 15:51

So thanks for the suggestions, the feedback really helped with my learning. So what I went on to do was the following, based on the relationsip--> A Reservation (now called Booking) has a Show, A Show has a Row, A Row has a Seat(s).

In the Booking class I now have this: Thanks @janos

private boolean reserveSeat(int selectedRow, int selectedSeat) {
    if (show.isSeatReserved(selectedRow, selectedSeat)) {
        System.out.println("Sorry, that seat has already been booked");
        return false;
    } else {
        show.reserveSeat(selectedRow, selectedSeat);
        setRowNumber(selectedRow);
        setSeatNumber(selectedSeat);
        System.out.println("This seat has now been booked.");
        return true;
    }
}

In the Show class I have this:

public boolean isSeatReserved(int selectedRow, int selectedSeat) {
    if (getRow(selectedRow).getSeatStatus(selectedSeat)) {
        return true;
    } else
        return false;
}

and in the Row class I have

public boolean getSeatStatus(int selectedSeat) {
    return getSeat(selectedSeat).getReservationStatus();
}

I thought it may be useful to other people just starting out (like me) to show this graphically using before and after diagrams taken from the jarchitect tool which shows what a mess my code was in! I used the same logic to tidy up some other classes that "knew too much".

Law of demeter - using only one dot, could I improve this logic?

3 Answers3