Refactoring- Removed 3 design smells #856
Conversation
There was a problem hiding this comment.
That's amazing! Great and thanks again for your contribution 🥇
I have just two comments:
-
We can place the method
formatErrorMessageWithParametersdirectly into theWorkflowException. This is a good place and your method makes of course sense. And so we can leave the other Exceptions Types as the are -
Place the method
modelVersionTableToStringinto theModelRestServiceas this is a very specific part of the REST API. TheModelServicein the Engine package should not handle this part.
There was a problem hiding this comment.
lets move this method into the abstract class WorkflowException
There was a problem hiding this comment.
lets move this method into the abstract class WorkflowException
There was a problem hiding this comment.
I kept the implementation of this method in both pluginException and validationException as the implementation is dependent on the params field of these classes.
There was a problem hiding this comment.
I changed this now in the Issue #857 . This simplifies the code much more and we have the feature now for all Imixs Exception Types.
Thanks for you contribution!
There was a problem hiding this comment.
remove this as we can have a valid implementation directly in the abstract WorkflowException class
There was a problem hiding this comment.
This was the method which was defined in interface modelManager and overriden in model service, now I have moved it to modelRestservice as per your suggestion, and it no longer a part of the modelManager hence not required to be overriden here.
There was a problem hiding this comment.
just put the implementation here
There was a problem hiding this comment.
I understand your attention, but lets have the method modelVersionTableToString still in the ModelRestService as this is a very specific part of the REST API
There was a problem hiding this comment.
Moved the function to modelRestService.
Refactored to resolve cyclic dependency
Location-
Sub-Project: imixs-workflow-engine
Package: org.imixs.workflow.engine
Classes: AsyncEventScheduler and AsyncEventService
Refactoring applied- Move method to increase cohesion.
Location(s)- Sub-project: imixs-workflow-jax-rs
Package: org.imixs.workflow.jaxrs
Class: ModelRestService
Sub-project: imixs-workflow-engine
Package: org.imixs.workflow.engine
Class: ModelService
Sub-project: imixs-workflow-core
Package: org.imixs.workflow
Class: ModelManager
ModelRestService was using ModelService’s methods quite a few times in its method printVersionTable. I moved the method to perform the desired operation in the ModelService where it belonged and then called that method once in the ModelRestService to get the output.
The model was also defined in the ModelManager interface as ModelService implements it.
Refactoring applied- Replace conditional with polymorphism.
Location(s)- Sub-project: imixs-workflow-faces
Package: org.imixs.workflow.faces.util
Class: ErrorHandler
Sub-project: imixs-workflow-faces
Package: org.imixs.workflow.faces.util
Class: ValidationException
Sub-project: imixs-workflow-core
Package: org.imixs.workflow.exceptions
Class: PluginException
Sub-project: imixs-workflow-core
Package: org.imixs.workflow.exceptions
Class: WorkflowException
ErrorHandler class had conditionals in the addErrorMessage where it was checking the instance of PluginException and ValidationException and performing action based on that. I created a method formatErrorMessageWithParameters in the abstract class workflowException and overrode this method in PluginException and ValidationException child classes. In the ErrorHandler class I changed the conditional with the call to this new method and based on the object type it will perform desired behavior.