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

Unit class hierarchy inconsistency #1197

Open
bevans2000 opened this issue Jan 1, 2024 · 4 comments
Open

Unit class hierarchy inconsistency #1197

bevans2000 opened this issue Jan 1, 2024 · 4 comments
Labels
design change A change to the code to improve performance or reusability
Milestone

Comments

@bevans2000
Copy link
Member

bevans2000 commented Jan 1, 2024

Describe the bug
A part of #647 the Unit hierarchy has been shown to not be accurate and the Unit class is populated with methods that are only applicable to some of the subtypes. Hence it creates an inconsistent design/model.
Unit contains methods that are only relevant to entities that can move around; e.g., setBuilding, setSettlement and more complex logic that needed to identify location state.
The reverse is also true in that classes in the Structure sub-hierarchy have to implement methods that will never be called.
This means that the Unit class has more logic than needed.

Expected and unexpected behavior
The solution is to create a new class MovableUnit that is a sibling on the existing Structure class that represents Unit that can move. The latter represents Unit that cannot be moved. This give a clean logic structure.
It will allow some properties/methods to be move from the Unit to the new class as well as some default implementation removed in the Structure hierarchy.
In addition, it can strengthen the use of the transfer method as this would become abstract on the MoveableUnit class

@bevans2000 bevans2000 added the design change A change to the code to improve performance or reusability label Jan 1, 2024
@mokun
Copy link
Member

mokun commented Jan 2, 2024

Yes it's good idea to create Moveable vs. Nonmoveable Unit.

@bevans2000 bevans2000 added this to the 3.9.0 milestone Jan 14, 2024
@bevans2000
Copy link
Member Author

I think the new Unit hierarchy should be as below. Key changes:

  • Settlement parented off ``Unit```
  • MovableUnit new class for those Units that can move around a Settlement

Unit hierachy

@mokun
Copy link
Member

mokun commented Jan 28, 2024

MovableUnitnew class for those Units that can move around aSettlement```

Sure. I like the concept of separating movable and non-movable units.

@bevans2000
Copy link
Member Author

It will help up the Unit class as it has methods not applicable to all subclasses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design change A change to the code to improve performance or reusability
Projects
None yet
Development

No branches or pull requests

2 participants