OBPIH-6215 Implement displayStatus method for returns with util, that…#4587
Conversation
… can be extended to other workflows
kchelstowski
requested review from
alannadolny,
awalkowiak,
drodzewicz,
ewaterman and
jmiranda
| Location currentLocation = AuthService.currentLocation | ||
| return getStockMovementDirection(currentLocation) == StockMovementDirection.INBOUND |
There was a problem hiding this comment.
Basically, if the currentLocation is used only once, I don't see a need to create a new variable for that 🤔
| Location currentLocation = AuthService.currentLocation | |
| return getStockMovementDirection(currentLocation) == StockMovementDirection.INBOUND | |
| return getStockMovementDirection(AuthService.currentLocation) == StockMovementDirection.INBOUND |
There was a problem hiding this comment.
For me it looked cleaner to keep it in a separate variable. It might not make a sense from the „memory” POV, but readability is also important.
If you guys prefer not creating a variable for that, then I’ll change it.
There was a problem hiding this comment.
To be honest, for me, the readability is the same in both cases, but I got your point of view, so lgtm
| StockMovementDirection getStockMovementDirection(Location currentLocation) { | ||
| if (currentLocation == origin) { | ||
| return StockMovementDirection.OUTBOUND | ||
| } | ||
| if (currentLocation == destination || origin?.isSupplier()) { | ||
| return StockMovementDirection.INBOUND | ||
| } | ||
| return null | ||
| } |
There was a problem hiding this comment.
Wouldn't it be better to somehow adjust the code to use the method from StockMovement? Or it is not possible? Maybe you can make it static and call it by StockMovement.getStockMovementDirection?
There was a problem hiding this comment.
I think it’s better to keep it in the helper class, because otherwise I’d have to refactor the existing method in the StockMovement to receive the locations in the params etc.
I think this helper class should be „a central place” for any type of DTOs we have (StockMovement, OutboundStockMovement, etc.)
I wouldn’t say there is a sense to do it now, since this can be extended and refactored for Dariusz’s needs for regular SMs. |
|
Ok, that sounds good to me |
3 participants
… can be extended to other workflows