From 77edba2e4e829c50dad6f743606385b235a7adc8 Mon Sep 17 00:00:00 2001 From: Jamie McKernan Date: Thu, 18 May 2023 17:54:58 +0100 Subject: [PATCH 1/5] Add extra detail to Liskov basic conclusion --- src/solid/liskov/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/solid/liskov/README.md b/src/solid/liskov/README.md index 4cfda45..29afdbc 100644 --- a/src/solid/liskov/README.md +++ b/src/solid/liskov/README.md @@ -238,7 +238,8 @@ immediately obvious to us as a developer, especially when the inheritance hierar makes sense when talking about objects with natural language. If you want to dive into this principle more deeply, take a look inside the -[`./detailed`](./detailed) directory. +[`./detailed`](./detailed) directory. It contains the specific rules which form the +basis of the Liskov substitution principle. ### Additional reading From f829d30a141d6a856d41e098987a5fa847b8a5e3 Mon Sep 17 00:00:00 2001 From: Jamie McKernan Date: Thu, 18 May 2023 18:26:08 +0100 Subject: [PATCH 2/5] Add anti-pattern and tests --- src/solid/liskov/detailed/parameter_types.py | 30 ++++++++++++++++ tests/liskov_parameter_types_test.py | 37 ++++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 src/solid/liskov/detailed/parameter_types.py create mode 100644 tests/liskov_parameter_types_test.py diff --git a/src/solid/liskov/detailed/parameter_types.py b/src/solid/liskov/detailed/parameter_types.py new file mode 100644 index 0000000..72154b4 --- /dev/null +++ b/src/solid/liskov/detailed/parameter_types.py @@ -0,0 +1,30 @@ +# anti-pattern +class VideoGame: + name: str + + +class PCGame(VideoGame): + operating_system: str + + +class Gamer: + def play_games(self, game: VideoGame) -> bool: + return True + + +class PCGamer(Gamer): + def play_games(self, game: PCGame) -> bool: # type: ignore[override] + if game.operating_system != "windows": + raise ValueError("Games only work on Windows.") + else: + return True + + +class LivingRoom: + def __init__(self, gamer: Gamer, video_game: VideoGame): + self.gamer = gamer + self.video_game = video_game + + def start_entertainment(self) -> bool: + result = self.gamer.play_games(self.video_game) + return result diff --git a/tests/liskov_parameter_types_test.py b/tests/liskov_parameter_types_test.py new file mode 100644 index 0000000..86eed1e --- /dev/null +++ b/tests/liskov_parameter_types_test.py @@ -0,0 +1,37 @@ +import pytest + +from src.solid.liskov.detailed.parameter_types import ( + Gamer, + LivingRoom, + PCGamer, + VideoGame, +) + + +def test_subclass_usage_works_fine() -> None: + # given + my_game = VideoGame() + gamer = Gamer() + + living_room = LivingRoom(gamer, my_game) + + # when + result = living_room.start_entertainment() + + # then + assert result + + +@pytest.mark.xfail(reason="This test demonstrates an anti-pattern.") +def test_liskov_violation_breaks_code_with_superclass() -> None: + # given + my_game = VideoGame() + gamer = PCGamer() + + living_room = LivingRoom(gamer, my_game) + + # when + result = living_room.start_entertainment() + + # then + assert result From 4fffa9f88a59d835373b4663d91c772d340155dc Mon Sep 17 00:00:00 2001 From: Jamie McKernan Date: Sat, 20 May 2023 22:06:04 +0100 Subject: [PATCH 3/5] Changed to ConsoleGamer --- src/solid/liskov/detailed/parameter_types.py | 20 ++++++++++++-------- tests/liskov_parameter_types_test.py | 4 ++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/solid/liskov/detailed/parameter_types.py b/src/solid/liskov/detailed/parameter_types.py index 72154b4..9cedd7b 100644 --- a/src/solid/liskov/detailed/parameter_types.py +++ b/src/solid/liskov/detailed/parameter_types.py @@ -1,10 +1,13 @@ -# anti-pattern -class VideoGame: +class Game: name: str -class PCGame(VideoGame): - operating_system: str +class VideoGame(Game): + ... + + +class ConsoleGame(VideoGame): + supported_console: str class Gamer: @@ -12,10 +15,11 @@ def play_games(self, game: VideoGame) -> bool: return True -class PCGamer(Gamer): - def play_games(self, game: PCGame) -> bool: # type: ignore[override] - if game.operating_system != "windows": - raise ValueError("Games only work on Windows.") +# anti-pattern +class ConsoleGamer(Gamer): + def play_games(self, game: ConsoleGame) -> bool: # type: ignore[override] + if game.supported_console != "xbox": + raise ValueError("Game only supported on xbox.") else: return True diff --git a/tests/liskov_parameter_types_test.py b/tests/liskov_parameter_types_test.py index 86eed1e..0e8bf55 100644 --- a/tests/liskov_parameter_types_test.py +++ b/tests/liskov_parameter_types_test.py @@ -1,9 +1,9 @@ import pytest from src.solid.liskov.detailed.parameter_types import ( + ConsoleGamer, Gamer, LivingRoom, - PCGamer, VideoGame, ) @@ -26,7 +26,7 @@ def test_subclass_usage_works_fine() -> None: def test_liskov_violation_breaks_code_with_superclass() -> None: # given my_game = VideoGame() - gamer = PCGamer() + gamer = ConsoleGamer() living_room = LivingRoom(gamer, my_game) From 099721c1d486f0fbfb38cd33715c2ca1d5fb73ee Mon Sep 17 00:00:00 2001 From: Jamie McKernan Date: Mon, 22 May 2023 19:32:32 +0100 Subject: [PATCH 4/5] Add tutorial --- src/solid/liskov/detailed/ParameterTypes.md | 166 ++++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 src/solid/liskov/detailed/ParameterTypes.md diff --git a/src/solid/liskov/detailed/ParameterTypes.md b/src/solid/liskov/detailed/ParameterTypes.md new file mode 100644 index 0000000..a15cc84 --- /dev/null +++ b/src/solid/liskov/detailed/ParameterTypes.md @@ -0,0 +1,166 @@ +# Detailed Liskov Substitution + +## Parameter Types + +When inheriting from a class, all method parameter types should match, or be _more_ +abstract than the superclass (parent) type. + +Put simply, if `B()` inherits from `A()` and `A()` has a `.do_something(some_parameter)` +where `some_parameter` is of type `C()`. Then if `B()` overrides `A.do_something(some_parameter)`, +it should not make the type of `some_parameter` more specific than +type `C()`. It can only be `C()`, or a parent class of `C()`. + +Yeah, that didn't make it clearer for me either - so let's take a look at the example. +Rather than walk through the full code in the usual order, I will present the code +in an order which helps highlight the problem first. + +### The Problem + +Let's say we have a class, `LivingRoom` which looks like the following: + +```python +class LivingRoom: + def __init__(self, gamer: Gamer, video_game: VideoGame): + self.gamer = gamer + self.video_game = video_game + + def start_entertainment(self) -> bool: + result = self.gamer.play_games(self.video_game) + return result +``` + +And our `Gamer` looks like the following (take note of the `game` parameter type): + +```python +class Gamer: + def play_games(self, game: VideoGame) -> bool: + return True +``` + +Now let's take a look at a simple test which passes: + +```python +def test_subclass_usage_works_fine() -> None: + # given + my_game = VideoGame() + gamer = Gamer() + + living_room = LivingRoom(gamer, my_game) + + # when + result = living_room.start_entertainment() + + # then + assert result +``` + +This test works fine because all the types match up, so we are using the class +as intended. + +But what about this test? It fails for some reason: + +```python +@pytest.mark.xfail(reason="This test demonstrates an anti-pattern.") +def test_liskov_violation_breaks_code_with_superclass() -> None: + # given + my_game = VideoGame() + gamer = ConsoleGamer() + + living_room = LivingRoom(gamer, my_game) + + # when + result = living_room.start_entertainment() + + # then + assert result +``` + +All we did was change `Gamer` to `ConsoleGamer`. And if we take a (high-level) look at +`ConsoleGamer` we can see that it's a subclass of `Gamer`: + +```python +class ConsoleGamer(Gamer): + ... +``` + +We are making the following assumption: the `LivingRoom` constructor +tells us that we can accept any gamer of type `Gamer`, and since `ConsoleGamer` +inherits from `Gamer`, it is a valid subtype, so it should work right? + +This is a _correct_ assumption and interpretation of the code from the type hints. So +why didn't it work? + +Well, if we had followed the Liskov substitution principle properly, it _would_ have +worked. But when we take a closer look at the implementation of `ConsoleGamer`: + +```python +# anti-pattern +class ConsoleGamer(Gamer): + def play_games(self, game: ConsoleGame) -> bool: # type: ignore[override] + if game.supported_console != "xbox": + raise ValueError("Game only supported on xbox.") + else: + return True +``` + +We can see that the signature of `.play_games(...)` has changed! It takes a `game` +parameter of type `ConsoleGame`, a subclass of `VideoGame`: + +```python +class ConsoleGame(VideoGame): + supported_console: str +``` + +Compare that with the original `.play_games(...)` method of a `Gamer`: + +```python +class Gamer: + def play_games(self, game: VideoGame) -> bool: + return True +``` + +And we can see it takes a `game` parameter of type `VideoGame`. + +The violation of the Liskov substitution principle occurred when `ConsoleGamer` +changed the type of the`game` parameter in `.play_games(...)` from `VideoGame` to +`ConsoleGame`. + +Because of this violation, although we'd expect that second test to work, it doesn't +because the more specific parameter type `ConsoleGame` makes use of a property +`supported_console` which doesn't exist in the more generic `VideoGame`. + +### The Solution + +Preventing this issue is simple. In our example, the `ConsoleGamer` should not have +changed the `game: VideoGame` parameter to the more specific `game: ConsoleGame` type. + +This is what we mean by: + +>...all method parameter types should match, or be _more_ +>abstract than the superclass (parent) type. + +If we kept the parameter as `game: VideoGame` it would have been fine. It is also +acceptable to make the type _more_ abstract. So in our case, we could have changed the +parameter to `game: Game` since this is the parent class of `VideoGame`: + +```python +class Game: + name: str + + +class VideoGame(Game): + ... +``` + +### Notes + +You may have noticed in the definition of the `ConsoleGamer` a comment with: + +`# type: ignore[override]` + +This is to tell `mypy`, a tool designed to look for +issues with the type-hints in your code, to ignore the error in my code. + +[`mypy`](https://mypy.readthedocs.io/en/stable/) is sophisticated enough that it can actually detect this violation of the +Liskov substitution principle, so I have to use a comment to suppress the warning so +that the project will still pass linting checks in GitHub Actions. \ No newline at end of file From 5ac39738bd2c83503f9a1b68af5db44ca072ab2c Mon Sep 17 00:00:00 2001 From: Jamie McKernan Date: Tue, 23 May 2023 22:27:27 +0100 Subject: [PATCH 5/5] Add code structure to guide --- src/solid/liskov/detailed/ParameterTypes.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/solid/liskov/detailed/ParameterTypes.md b/src/solid/liskov/detailed/ParameterTypes.md index a15cc84..23eb9be 100644 --- a/src/solid/liskov/detailed/ParameterTypes.md +++ b/src/solid/liskov/detailed/ParameterTypes.md @@ -1,5 +1,12 @@ # Detailed Liskov Substitution +## Structure + +| File | Description | +| ----------- | ----------- | +| [`./parameter_types.py`](parameter_types.py) | Code example containing an anti-pattern. | +| [`tests/liskov_parameter_types_test.py`](../../../../tests/liskov_parameter_types_test.py) | Unit tests to show code in action. | + ## Parameter Types When inheriting from a class, all method parameter types should match, or be _more_