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.

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

Fix example LSP #107

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

Fix example LSP #107

wants to merge 1 commit into from

Conversation

SirFX
Copy link

@SirFX SirFX commented Jan 15, 2022

Fix comment

Copy link

@dmitryvhf dmitryvhf left a comment

Choose a reason for hiding this comment

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

Changes is not correct.

@@ -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.

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

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?

@dmitryvhf dmitryvhf mentioned this pull request Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants