Development: Thoughts about implementation redesign?

[ronzulu]

Hi @st3v3nmw

Thanks for all your hard work on this amazing plug-in, I find it very useful.
https://ko-fi.com/Home/CoffeeShop?txid=12badaed-331f-47e6-bcb2-bd93ffce5379&mode=p&img=ogbuymeacoffee

I was wanting to implement a couple of enhancements, but found fair difficulty in getting started.

I think with some redesign and the addition of unit test cases, it might be easier for new developers to get involved.

I've started to look into this, with a focus on restructuring the code to enable unit testing on a greater portion of the code base.

Anyway, just wanted to put this "out there".

Cheers
Ronny

[ronzulu]

Hi @st3v3nmw

You have been quiet on GitHub for your wonderful plug-in, hope you are ok!

I have made good progress in restructuring the code, perhaps 75% done.

I've extracted nearly all the "business logic" from the user interface code, as well as other refactoring, and written many unit test cases.

There are now 90 test cases (up from 21)

Once I've completed this, will you be able to review and merge into the master branch?

If not, are you able to suggest someone else?

Cheers
Ronny

[st3v3nmw]

Hi @ronzulu!

I'm doing Ok, I hope you are as well.

Thanks for the effort! This is a much needed change. And yes, I'll be able to review and merge into the master branch.

[ronzulu]

I've made fair progress, but I haven't completed the restructure. I thought it might be useful for you to see it in its current state, as I work towards completion.

This is the first time I've worked on an open source project, apologies if I've stepped on anybody's toes.

Scope of changes

All implementation related to flashcards. No restructuring of code related to reviewing of notes at this stage.

Functionality identical to the latest master branch, 1.10.1. At least this was the intention.

Aim

Separating out the business logic from the user interface code, as well as other re-factoring so that the code is in a state where unit test cases can be written.

The purpose here is so that new developers can make contributions with confidence that changes haven't broken the existing code base.

Design Principles

Separation of Concerns (SoC)
Don't repeat yourself (DRY)

Specifics (highlights only)

Refactored findFlashcardsInNote into code in new classes: NoteFileLoader, Note, Question, QuestionType, NoteQuestionParser and others.

Extracted some of the properties from the existing Card interface into new classes Question and QuestionText. A Question instance relates to one or more sibling card instances, and holds all the properties common to sibling cards.

A new class TopicPath encapsulates path: string[] with simple methods for parsing, manipulation etc

There is a new interface called ISRFile with simple file operations read(), write() etc. Apart from a concrete class ObsidianTFile that wraps the obsidian TFile, there is one called UnitTestSRFile. This way methods that impact files use the interface, and is now testable. Unit test cases create instances of UnitTestSRFile which can then be checked by the test code.

Similarly, there is a new interface IDateProvider with concrete classes LiveDateProvider and StaticDateProvider. The static date provider is used during unit testing to fix the current date to a known value so that calculations based on the date will return fixed values.

Based on SoC, the code that reads and parses the note into questions and cards has narrower scope than the original findFlashcardsInNote. For example, functionality for keeping track of statistics has been separated out into DeckTreeStatsCalculator etc.

All the business logic contain within FlashcardModal has been extracted into a new class with interface IFlashcardReviewSequencer. This enables unit testing of operations such have reviewing a card or skipping a card.

I know this is your baby, and you've put in so much work over a long period of time. I've tried to only make changes when there is IMO a benefit, and not just stylistic changes. However, I probably did go overboard at times and made changes that were more cosmetic in nature. Apologies!

Also, there have been some cases where I've probably made the code less efficient in favour of readability. The only case that comes to mind is keeping track of card counts in multi-level decks. I'm happy to optimise if necessary.

Next steps

Again, this is largely complete, but not fully complete. Hopefully you will largely be happy with the changes I've made and I'll let you know when I've completed the work and added some more test cases.

Any feedback or questions is welcome, especially as I haven't contributed to an open source project before.

Repository

https://github.com/ronzulu/obsidian-spaced-repetition

Regards
Ronny

[st3v3nmw]

Hi @ronzulu,

I'm very happy with the changes so far and I've started testing the 1.11.0-beta.0 release locally.

And no worries, you haven't stepped on anyone's toes. The refactor is a much needed redesign.

Thanks for the effort!

[ronzulu]

Hi @st3v3nmw

I've completed the restructure, and added more test cases. Now around 160.

If I create a pull request, will you have time to review and merge?

I see that there are 12 existing requests, that have been around for a while. Does anyone else have repository permission to perform the merge, or only your good self?

The intention has been to keep the existing functionality. However, the restructure may have fixed some bugs. For example, the following has been fixed:
#745

(Hopefully haven't created new bugs in the process!)

Cheers
Ronny

[st3v3nmw]

I'm currently reviewing and testing the changes 😃

[ronzulu]

Great 😃