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

fixed_design split into smaller export function #263

Closed
elong0527 opened this issue Jun 24, 2023 · 4 comments · Fixed by #271
Closed

fixed_design split into smaller export function #263

elong0527 opened this issue Jun 24, 2023 · 4 comments · Fixed by #271
Assignees

Comments

@elong0527
Copy link
Collaborator

elong0527 commented Jun 24, 2023

Each method in fixed_design() should be its own function

  1. For better documentation. each method may require specific arguments in the ...
  2. Reduce maintenance efforts. The function body contain > 600 lines

We probably should have functions corresponding to each method, e.g.:

  • fixed_design_ahr()
  • fixed_design_wlr()
  • fixed_design_maxcombo()
  • fixed_design_rmst()
  • fixed_design_milestone()
@nanxstats
Copy link
Collaborator

This would be a much welcomed change. Functions should have cyclomatic complexity of less than 15. fixed_design() has 73. I had to disable this complexity check in .lintr so it can pass.

@LittleBeannie
Copy link
Collaborator

I agree! Can we have fixed_design to call fixed_design_ahr, fixed_design_wlr, ...?

@nanxstats
Copy link
Collaborator

nanxstats commented Jun 30, 2023

I agree! Can we have fixed_design to call fixed_design_ahr, fixed_design_wlr, ...?

Let's discuss how we can balance these considerations. Some initial thoughts below as a start...

Reasons to still include a fixed_design() function:

  1. Abstraction and simplicity: If users typically use all or most methods at once, a fixed_design() function provides a simpler interface.
  2. Backward compatibility: If fixed_design() is widely used in existing code, maintaining it can ease the transition and prevent breaking changes.

Reasons to not include a fixed_design() function per SOLID principles:

  1. Single Responsibility Principle: Our current fixed_design() does more than it should. By breaking it into smaller functions, each function has one clear responsibility, improving readability and maintainability.
  2. Open-Closed Principle: If we add more methods in the future, we will need to modify fixed_design(), violating OCP (software entities should be open for extension, but closed for modification). Using smaller functions makes extension easier and less error-prone.
  3. Liskov Substitution Principle: A fixed_design() introduces variability that can lead to inconsistency and unexpected behavior, violating LSP. Smaller functions provide predictability and consistency.
  4. Interface Segregation Principle: A larger fixed_design() might force unnecessary dependencies. Smaller functions allow users to depend only on what they need.
  5. Dependency Inversion Principle: A large fixed_design() may become tightly coupled with smaller functions' implementation details, violating DIP. Separating them would promote better encapsulation and abstraction.

@LittleBeannie
Copy link
Collaborator

Decision: delete fixed_design and divide them into

fixed_design_ahr()
fixed_design_wlr()
fixed_design_maxcombo()
fixed_design_rmst()
fixed_design_milestone()

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 a pull request may close this issue.

3 participants