Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Extract Viewer Commands to ViewerCommand class #6059

Merged
merged 8 commits into from
Apr 22, 2020

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Apr 21, 2020

Non-functional prep work for #6052

Also removes code from AbstractFlashCardViewer, which is a side-goal of mine.

Don't mind if this is squashed, commits are small to ensure they're readable in review.

Checklist

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code

@timrae
Copy link
Member

timrae commented Apr 21, 2020

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 2
- Added 3
           

Complexity decreasing per file
==============================
+ AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.java  -3
         

See the complete overview on Codacy

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one tiny question but otherwise LGTM

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Author Reply Waiting for a reply from the original author labels Apr 21, 2020
@Arthur-Milchior
Copy link
Member

Why do you want to remove code from abstrractflashcardviewer ?

@david-allison
Copy link
Member Author

david-allison commented Apr 21, 2020

Why do you want to remove code from abstrractflashcardviewer ?

Primarily: It's so large that it's become hard to reason about the state of the object. There's a lot of responsibilities which are combined, and some state is implicitly modelled as the state of the UI elements, rather than explicitly. "Single responsibility principle" is the typical term to Google, but everyone has different opinions on what it means.

If you want to add new code, then it's still fairly easy, but it increases the class size. If you want to modify existing code, then it takes much longer as you'll struggle to mentally manage the context required to make the change without introducing regressions. Testing also becomes harder for this reason, as it's hard to exhaustively test all code paths.

Finally, there's a lot of code in AbstractFlashcardViewer that is review-only (ease calculation was one that I moved recently, gestures is up for discussion) which would fit much better in the Reviewer class.

@mikehardy mikehardy merged commit 5ea753f into ankidroid:master Apr 22, 2020
@mikehardy mikehardy removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Apr 22, 2020
@david-allison david-allison deleted the reviewer_add_commands branch April 22, 2020 13:52
@mikehardy
Copy link
Member

This went out with 2.10alpha72 - available as soon as google is done processing it

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants