Simplifying structure of the class ApplicationManagerImpl#13
Simplifying structure of the class ApplicationManagerImpl#13VadymLuchko wants to merge 3 commits intomasterfrom
Conversation
VadymLuchko
requested review from
AKalinich-Luxoft,
KhrystynaDubovyk,
VladSemenyuk and
ychernysheva
AKalinich-Luxoft
left a comment
AKalinich-Luxoft
left a comment
There was a problem hiding this comment.
@VadymLuchko I think it would be more descriptive to attach some UML diagram like this:
ApplicationManager.zip
It is easier to read and understand
| To simplify the class ApplicationManagerImpl, some edits are proposed: | ||
|
|
||
| - replace some predicates functions with lambdas | ||
| - move some functions to appropriates places | ||
| - move some methods to separate classes |
There was a problem hiding this comment.
@VadymLuchko this is not a motivation, its better to move down to proposed solution.
Motivation is mostly about which benefits this proposal can give to us.
For example, you can write that ApplicationManagerImpl will be easier to use as all functionality will be regrouped on corresponding calsses. Each class will be more obvious from functionality point of view. etc.
|
|
||
|
|
||
| Move methods to new sub class `ApplicationDataProvider` because they are have very similar implementation | ||
| And probably we could simplify this methods by move coomon part to base class |
There was a problem hiding this comment.
| And probably we could simplify this methods by move coomon part to base class | |
| And probably we could simplify these methods by moving common parts to the base class |
| Move methods to new sub class `ApplicationDataProvider` because they are have very similar implementation | ||
| And probably we could simplify this methods by move coomon part to base class | ||
| ``` | ||
| DataAccessor<ApplicationSet> ApplicationManagerImpl::applications() |
There was a problem hiding this comment.
@VadymLuchko for this list, I would recommend to mention only function names like
applications()
pending_applications()
...
As it is pretty hard to read
| Move methods to new class `WayPointsHandler` | ||
| ``` | ||
| IsAppSubscribedForWayPoints | ||
| SubscribeAppForWayPoints | ||
| UnsubscribeAppFromWayPoints | ||
| SaveWayPointsMessage | ||
| IsAnyAppSubscribedForWayPoints | ||
| GetAppsSubscribedForWayPoints | ||
| ``` |
There was a problem hiding this comment.
@VadymLuchko maybe we can make a waypoints_plugin for this functionality?
There was a problem hiding this comment.
@VadymLuchko please add () for each function in the list
There was a problem hiding this comment.
I also think that it's confusing a bit that SDL RPC plugin is responsible for processing waypoints-related data
|
|
||
|
|
||
|
|
||
| Move methods to new class `HMIHandler` |
There was a problem hiding this comment.
| Move methods to new class `HMIHandler` | |
| Move methods to new class `HmiHandler` |
| ``` | ||
|
|
||
|
|
||
| Move methods to new class `NavigationHandler` |
There was a problem hiding this comment.
@VadymLuchko I think this should be merged with StreamingHandler or be a part of it
| ``` | ||
|
|
||
|
|
||
| Move methods to new class `FilesystemHelper` |
There was a problem hiding this comment.
@VadymLuchko we already have file_system as a good place for these functions
| ``` | ||
|
|
||
|
|
||
| Move methods to new class `ApplicationsHandler` |
There was a problem hiding this comment.
@VadymLuchko I think this should be a part of ApplicationDataProvider as it works with the same collection
3 participants
Concentration of various functionality within one class violates the single responsibility principle and makes it difficult to work with the class file.
ApplicationManagerImpl.cccontains more then 5000 lines of code that solve various problems and can be thematically categorized into separate classes.