Submit #1
Submit #1
Conversation
There was a problem hiding this comment.
This is a big pull request (over 50 files). So tried to focus on the key areas. Things that we pay attention to in this exercise are "data structures", "separation of concerns", "single responsibility principle", "code reuse" and preferrably utilization of the "design patterns".
The Good Stuff 👍
Your solution is functioning as expected and successfully processes the provided JSON file as input. You used the right data structures here for example a Map for storing statuses by asset id. You also utilised the "Observer" pattern along with other design patterns like "Dependency Injection" which we expected. Also liked that you chose composition over inheritance in a few places in your implementation. Looks like you also implemented the web service which is a good addition although I hope you did not spend too much time on it since it was not required.
Not sure about 😕
It feels like the implementation is overly complicated and could be further simplified. For example the usage of queues for handling asset status changes as well as other code associated with it was excessive and unnecessary for this exercise. Also the requirements were probably not clear enough since you implemented the "Observer" pattern to subscribe to status changes per asset instead of all assets. Understanding requirements is very important and if something is unclear (we deliberately miss some details in these exercises) developer should ask the right questions to fill the gaps. Overall the code could have been separated into classes and interfaces differently for more clarity, better separation of concerns and classes and methods could be named differently to improve readability.
| sendMessages(file, applicationContext); | ||
| } | ||
|
|
||
| waitForProcessToFinish(applicationContext); |
There was a problem hiding this comment.
Is this necessary to wait here for 10ms? Doesn't look like sendMessage() called asynchronously so the execution would not proceed untill the for loop finishes
There was a problem hiding this comment.
I added this waiting time so the application will finish all its process before I start to get reports. It is not necessary for the outcome but I wanted the console output to be clear and readable. First program will do all the process and print notifications in the console. Then it will print the reports.
I could have also divide output to separate files for this matter.
| } | ||
| } | ||
|
|
||
| private static void sendMessages(File file, ConfigurableApplicationContext applicationContext) { |
There was a problem hiding this comment.
Is there a need for the whole ConfigurableApplicationContext here? Looks like you're just using an instance of a MessageHandler
| JsonArray jsonArray = (JsonArray) obj; | ||
| for (JsonElement element : jsonArray) { | ||
| AssetStatusMessage message = gson.fromJson(element, AssetStatusMessage.class); | ||
| MessageHandler messageHandler = applicationContext.getBean(MessageHandler.class); |
There was a problem hiding this comment.
This messageHandler is going to be redefined on every JSON message since it's in the loop. Why not pull it outside of the for loop?
| assetsInStateReport(applicationContext); | ||
| } | ||
|
|
||
| private static void subscribeSomeListeners(ConfigurableApplicationContext applicationContext) { |
There was a problem hiding this comment.
Is there a need for the whole ConfigurableApplicationContext here? Looks like you're just using an instance of an EventHandlerFacade
| for (JsonElement element : jsonArray) { | ||
| AssetStatusMessage message = gson.fromJson(element, AssetStatusMessage.class); | ||
| MessageHandler messageHandler = applicationContext.getBean(MessageHandler.class); | ||
| messageHandler.assetStatus(message); |
There was a problem hiding this comment.
Since it's a message handler I'd call this method:
messageHandler.handle(message);
usually we use nouns for constructors but methods are named after their actions using verbs
There was a problem hiding this comment.
Thank you for your advice, I will keep that in mind.
| try { | ||
| AssetStatusEvent event = AssetStatusEvent.valueOf(subscribeInfo[1]); | ||
| String assetId = subscribeInfo[0]; | ||
| SubscribeRequest subscribeRequest = new SubscribeRequest(assetId, event, new SampleListener()); |
There was a problem hiding this comment.
This would subscribe to events for a particular asset only. We need to be able to subscribe to status changes of any asset since we don't know about possible assets in advance. Also, given that there could be many assets, we would require a lot of subscriptions.
There was a problem hiding this comment.
I should have ask you to clarify this for me. Actually, in a real situation I would ask what does "subscribe to certain asset status events" mean exactly. But here I made an assumption that it means "subscribe to a specific asset rather than all assets". My mistake.
| Date updatedAt = assetStatusMessage.getCreatedAt(); | ||
| Status currentStatus = assetStatusMap.get(assetId); | ||
| //updateStatus asset status in map if it's a new status | ||
| if (currentStatus == null || updatedAt.getTime() >= currentStatus.getCreatedAt()) { |
There was a problem hiding this comment.
If there is a need to comment any code it's an indicator that this code can be improved for readability. In this case, we could extract the condition into a method:
| if (currentStatus == null || updatedAt.getTime() >= currentStatus.getCreatedAt()) { | |
| if (isNewStatus(assetStatusMessage)) { |
| Status currentStatus = assetStatusMap.get(assetId); | ||
| //updateStatus asset status in map if it's a new status | ||
| if (currentStatus == null || updatedAt.getTime() >= currentStatus.getCreatedAt()) { | ||
| assetStatusMap.put(assetId, newStatusType, updatedAt); |
There was a problem hiding this comment.
To simplify the put() method signature I'd suggest the following:
| assetStatusMap.put(assetId, newStatusType, updatedAt); | |
| assetStatusMap.put(assetId, new Status(assetStatusMessage)); |
To be able to do the above I would provide additional constructor for the Status class that could populate a Status instance from AssetStatusMessage:
public Status (AssetStatusMessage message) {
this(message.statusType, message.createdAt);
}| public GeneralResponse assetStatus(AssetStatusMessage assetStatusMessage) { | ||
| try { | ||
| //store asset status | ||
| StatusType oldStatusType = assetStatusService.updateStatus(assetStatusMessage); |
There was a problem hiding this comment.
There are two collaborators in this method one is AssetStatusService which contains asset statuses and is essentially a "state object". The second collaborator here is EventHandlerFacade which is used to notify observers/listeners about state changes. Observers use EventHandlerFacade to subscribe to events hence it is a "Subject" that observers listening to. Since all the state is stored in AssetStatusService and the EventHandlerFacade is not aware of those state changes this MessageHandler class is used to bridge the gap between the two collaborators.
I see a discrepancy here. Usually, the "Subject" is something that is aware of the state and can produce those events about the state change. I think in this case the implementation would be simpler if the observers would subscribe to events on AssetStatusService which would be able to produce those status change events. Then we probably wouldn't need this MessageHandler class as well as the EventHandlerFacade.
There was a problem hiding this comment.
I totally agree with you. Thank you.
Here is my solution.