-
Notifications
You must be signed in to change notification settings - Fork 0
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
Separation of Concern #8
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… tests for the newly added class
…and updating the process of getting p1 and p2 into tracker.top_left and tracker.bottom_right
…cv2(), .stream_as_image(), .generate_raw_outputs()
…elated data easier and reduce duplication.
…ormation related tasks can be orchestrated through one place.
… frame due to the new computed property
…as image, cv2 window is not also started
…rpose of the file
…e file to improve extendability
…e flask's app object to eliminate circular import
…ing debugging related code to another file
alvinwatner
requested changes
Aug 7, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, great work Krisna! I know this is taking a lot of pain and sweat, but I bet it will be worth it down the road and we surely learn a lot from this. Just couple more things to be improved, look forward for the update!
Krishna this is a amazing ⚡️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Separation of concerns
Separation of concern is important because right now the methods are very long and can contain different algorithm, each conveying different meaning. I want to further simplify it by breaking it into different classes and method. This has proven reduce the lines of code due to the removal of duplicating codes.
Another thing that I see is how related items are being treated as primitives, which leads into primitive obsession. I want to remediate that by encapsulating related items into singular class which can be instantiated using an object.
Long parameter list is also a symptom to lack of objects which can grow and become harder to maintain. Remediation plan for that is by creating sub methods that can reduce the number of parameter required for an object.
Moving on, by performing separation of concerns, development process can be made more streamlined as well as easier to maintain. This is mainly because our future endeavor for expanding the app is on the horizon, so ensuring that the development can be made as clear and as simple as possible is going to be a definite win.
Different concerns
AutoMapper should only have three methods, namely
stream_using_cv2
,stream_as_image
, andgenerate_raw_outputs
. Other methods should be delegated to different classes, according to their purpose.CV2Handler
should handle tasks related to CV2 operations, such as calculating and drawing framerate, calculating bottom center, drawing detection boxes, and saving video result.ImageHandler
should handle tasks related to the images used, namely the image2d and image3d. It should contain properties such as the image object itself, max height, and total width.HomographicHandler
should handle tasks related to homographic transformation, which revolves around loading coordinates and transforming coordinates.Frame
is a data class that holds properties such as the frame3d, frame2d, and frame count. Every used frame must be stored within the data class so that other methods can access and modify the same object.TrackerHandler
should handle tasks related to tracking detected objects from the ModelInterpreter, it also needs to calculate bottom center automatically rather than processing it on the caller.Notes on Backward Compatibility
I'm intentionally not removing the previous
dot_vision.py
andauto_mapper.py
. Instead I took an initiative to create a new file to allow real time comparison between the previous version and the new version.