anqitong / LangageDessinVectoriel

1 stars 2 forks source link

Open/closed principle : ShapeName #52

Closed anqitong closed 9 years ago

anqitong commented 9 years ago

This is a principle that should be respected and is wanted from the subject. Wikipedia article : https://en.wikipedia.org/wiki/Open/closed_principle

Problem with the class ShapeName : if we want to add a new geometric shape, we need to add its name in ShapeName. So this means we'll have to modify ShapeName, which ic contrary to the open/closed principle. The structure therefore shoudl be reviewed.

starsasumi commented 9 years ago

For me, modification in this level is acceptable and this brings more advantage than cons because users need to know what shapes we supported. Pure string name is a little bit not save.

Thomas-dot-G commented 9 years ago

I do agree, and actually with path, and polyline, and text, you can do anything (except the font for the text). All other shapes can be done by one of the three above. Plus, I took every shapes that a SVG can recognize. And if we need to add one, that is a really small thing, and an enum, not even a class.

anqitong commented 9 years ago

@Thomas : if you need to add for example Square, you'll add it in ShapeName put you'll also create a class Square that extends Shape, so it is still a class

anqitong commented 9 years ago

@starsasumi : I'm not hundred percent sure if that is okay, but I will check it and keep you updated.

Thomas-dot-G commented 9 years ago

Yes, sure. That will require to change two classes: factory and ShapeName. But actually it is only adding stuff, not really modifying them.

anqitong commented 9 years ago

Issue solved.