Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix example LSP #107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix example LSP #107

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2083,7 +2083,7 @@ Drawable RenderLargeRectangles(Rectangle rectangles)
{
rectangle.SetWidth(4);
rectangle.SetHeight(5);
var area = rectangle.GetArea(); // BAD: Will return 25 for Square. Should be 20.
var area = rectangle.GetArea(); // BAD: Will return 20 for Square. Should be 25.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your are wrong. BAD example - correct.

  • Square.SetHeight(5) set Width and Height to 5 (overwrite Width from 4).
  • Rectangle,GetArea calculare Width * Height = 25
    Good example fix i - different GetArea() with one SetLength

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran it and it gives 20, 20, 20 as result. The right approach would be to overrite the Rectangle::SetWidth() and Rectangle::SetHeight() in Square implementation.

Copy link

@dmitryvhf dmitryvhf Jan 16, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You right. Code result is 20. But i sure, this must be return 25 for BAD example.
But as fact - this LSP examples incorrect. This code is not executable. Alot errors, types, missed signatures and etc.

All problems into RenderLargeRectangles method. This cant be in correct code.

  • This must be only render. But now this change sizes.
  • In foreach object cast to Rectangle. Sure we cant set sizes for square.
    Initialization of array must be into main block. This normaly for simple example.
Rectangle rectangle1 = new Rectangle();
rectangle1.SetWidth(2);
rectangle1.SetHeight(3);

Rectangle rectangle2 = new Rectangle();
rectangle2.SetWidth(4);
rectangle2.SetHeight(5);

Square square = new Square();
square.SetWidth(4);
square.SetHeight(5);

var rectangles = new[] { rectangle1, rectangle2, square };
RenderLargeRectangles(rectangles);

BAD:

  • RenderLargeRectangles(Rectangle rectangles) -> Rectangle[] rectangles
  • SetWidth, SetHeight must have return type is void or double.
  • Render() signature is void. Drawable not used into examples.

GOOD:

  • RenderLargeRectangles: parameter must be ShapeBase of type. And array too.
  • RenderLargeRectangles: for set size we must use casting for each object type:
    ((Square)rectangle).SetLength(5);

and etc. errors.
Can you fix both examples?

rectangle.Render(area);
}
}
Expand Down