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

Split code #74

Closed
wants to merge 33 commits into from
Closed

Split code #74

wants to merge 33 commits into from

Conversation

Katsyuta
Copy link

@Katsyuta Katsyuta commented Jan 14, 2025

#2

Problems:

  1. Violanse SRP:
    The NodeTranslator class has multiple responsibilities. It manages several states and includes many methods for different logic, which makes the class difficult to maintain. As a result, the overall logic of the class is hard to understand and make changes.

The some responsible:

  • Observing the intersection of DOM elements with the visible part of the screen for implementing lazy translation.
  • Setting up handlers to process DOM mutation (translation of added and updated nodes)
  • Managing the translation of DOM elements (the logic for handlig tree of elements, methods for add, update and delete nodes, keeping node storage)

Solution: Split the class into multiple entities:

  • The LazyTranslator class: Translate element when it intersect viewport.
  • The NodesTranslator class: Manages the translation of DOM nodes, prvide method for add, update and etc.
  • The DomTranslator class: Sets handlers for DOM mutations and creates the necessary dependencies.

Profit:
After this, each class will have a single responsibility and will be easier to maintain. We can add new logic without complicating the class NodeTranslator. Example, LazyTranslator can be used outside the project or we easy can replace implimentation.

  1. Tightly coupled code
    The methods are directly connected to each other. This lack of flexibility makes the code tightly coupled, meaning that changes in one part require changes in another part.
    If the logic for lazy translation changes, the code that relies on it must also be modified, as the methods are directly dependent on each other, or if we change the logic for storing nodes, we should update the code that uses node storage

This may make changes harder to implement, take more time, increase the likelihood of errors, and reduce the willingness to modify the code.

Solution: Split code as in the previous proposal aand pass all dependencies through the constructor.

  • The LazyTranslator class receives a callback for translating nodes in the constructor. This allows the class to be used independently of the rest of the code.
  • The NodesTranslator class receives the LazyTranslator instance in the constructor and uses its methods. This allows any implementation of LazyTranslator to be provided.
  • The DomTranslation class creates the necessary dependencies and use the methods from NodesTranslator for setting mutation handlers.

Profit:
After these changes, the classes are no longer tightly coupled and can be modified independently.

After all the changes, we have a few classes that can be replaced or updated independently, and each class is easier to understand.

@Katsyuta Katsyuta changed the title 2 split code Split code Jan 16, 2025
@Katsyuta Katsyuta marked this pull request as draft January 16, 2025 20:17
@Katsyuta Katsyuta marked this pull request as ready for review January 17, 2025 00:41
Copy link
Contributor

@vitonsky vitonsky left a comment

Choose a reason for hiding this comment

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

  • Explain what exactly problem we have and what changes needed
  • Refactor all code, since it is not clear why we need current changes
  • Explain changes in this PR. Elaborate why new entities has been introduced
  • Add tests to cover all changes. Add unit tests + integration tests that would reproduce real world interactions of user with DOM

@Katsyuta Katsyuta requested a review from vitonsky January 22, 2025 22:24
@Katsyuta
Copy link
Author

@vitonsky can you repeat the review?

Key changes:
Several classes have been created:

  • The LazyTranslator class: Translates an element when it intersects with the viewport.
  • The NodesTranslator class: Manages the translation of DOM nodes, providing methods for adding, updating, etc.
  • The DomTranslator class: Sets handlers for DOM mutations and creates the necessary dependencies.

Unit tests have been added for the LazyTranslator and NodesTranslator classes, as well as a test for their combined functionality.

@vitonsky
Copy link
Contributor

I close this PR since there are no reasons to make changes in description.
I see only generic "problems" from wikipedia, but not specific code lines mentions and its problems demonstration.

Also as i see this PR have breaking changes:

  • it broke a public API, so we have to up major version + users must update code usage
  • tests have been changed, snapshots has been deleted, so it is not possible to verify that after this changes code works fine as before

If you still see problems in code, please file the issue with explanation of specific problems and your plan how to fix them. Thank you

@vitonsky vitonsky closed this Jan 31, 2025
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.

2 participants