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

✨: LIB-538 Refactor + add Dafalgan support #40

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Coniface
Copy link

@Coniface Coniface commented Jun 11, 2022

Notes

  • I supposed this was a single task handed with ticket number LIB-538 but I think it should be split in several sub-tickets and therefore successive pull requests. I did only one pull request to ease your review process and not spread it across multiple PRs.
  • I created a wiki.md file in the project to keep the documentation in the same place than my project. But I would recommend using Github's wiki feature if possible. I didn't use it here to avoid creating a second project.
  • I did not spend more time on Typescript configuration but it should be tuned for using node in production (and not ts-node!)
  • I used a JS getter/setter on Drug.benefit because the exercises specifies that the public API should not be changed and I wanted to add the check for benefit value ($0 ≤ benefit ≤ 50$). I would prefer using a setBenefit method as it is more explicit to me but it was not possible here. I'd love to hear what you think about that setter.

Process

  • I started with writing a quick documentation that regroups all the important information in a single place. I added some visuals to it (one diagram and tables) to make things clear before starting developments.
  • I then refactored the project's structure.
  • I replaced babel with typescript straight away so I could use it for my tests (although the tests should have been the second thing to do after documentation to ensure no regressions).
  • I added more tests to the existing test file. Although it is not the target architecture, it is important to avoid regressions during developments.
  • I refactored the Drug class as to me, the Drug (model) must be aware of its own logic, not the Pharmacy (controller).
  • BREAKING CHANGE To use my newly implemented classes, I had to forbid the creation of a Drug based on a name. If the Drug class is not abstract, it is possible to create a new Drug('Fervex', 30, 20) that will behave as a Drug instance despite the name 'Fervex' passed to the constructor. The developper should use the Fervex class so its instance has the overridden method of updateValues() corresponding to Fervex's logic.
  • I completed my quick documentation with the new Dafalgan rules.
  • I implemented the Dafalgan support.

I did not add the Dafalgan to the index.ts as it is not required by the instructions and it would modify the output.txt.

Time Management

In total, I spent around 4 hours on the tests:

  • ~1h30 reading the test, analyzing the legacy code and writing a first documentation.
  • ~1h setting up the environment and project with proper tooling (could be greatly improved, lost some time here)
  • ~1h30 refactoring the drugs with a clean API, writing tests for each of them
  • ~10m adding the Dafalgan (easy and enjoyable once everything is neat 🚀)

Many Thanks! 🎉

If you're reading this, it means you took time to review my PR properly so... Thanks a lot!
I never had a refactoring project as a technical test, I think it's a great idea and this one was fun to tackle.

I would be really glad to hear your feedbacks regarding my work, whether it's a full review here or a call.
I would also love to know how you would have done it, if you have any advice regarding anything (code, patterns, tests, tools, git practices...), I am always eager to further improve.

Coniface added 11 commits June 12, 2022 09:22
Here is a quick documentation that recaps the current status of the project.
- Adds an activity diagram for pharmacy's system
- Adds "truth tables" for each drug
According to yarn's warning, it is best to not mix package managers.
Moved sources in an /src tree to ease next refactor steps.
Current unit test and program outputs have been checked.
These unit test will ensure system still works after refactoring.
The Drug class now implements the logic of updating the drug depending on its rules.
The class should be abstract but it is left usable due to exercise's instructions.

Original unit tests are still working, output is not modified when program runs ensuring no regression.
Refactored the pharmacy class to remove all the logic from it.
The drugs containing their own logic, they are in charge of updating themselves.

Drug is made abstract because instantiating a drug with its name and expecting the logic to remain does not work anymore. The appropriate class should be used instead.
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.

1 participant