Interview task model implementation(Event Source Java)

Question

I've got a task to implement simple event source and i did: project

Events structure

### order base 
- id
- employee_id
- datetime
### order registered
- fields from order base
- client_id
- delivery_time
- merch_id
- merch_price
### order canceled
- fields from order base
- reason
### other orders have the same fields as order base

OrderEvent (order base)

Here's how my base class looks like, i override publish and checkRegister/CanceledOrOut in subclasses.

public class OrderEvent implements OrderCheck {
 final int orderId;
 final int employeeId;
 final Timestamp eventDate;
 EventType eventType;
 public OrderEvent(int orderId, int employeeId, Timestamp eventDate) {
 this.orderId = orderId;
 this.employeeId = employeeId;
 this.eventDate = eventDate;
 }
 public OrderEvent(OrderEvent event) {
 this(event.orderId, event.employeeId, event.eventDate);
 }
 public int publish() throws OrderIllegalStateException, SQLException {
 checkRegister();
 checkCanceledOrOut();
 int event_id = -1;
 try (Connection conn = DbUtil.getConnection()) {
 String sql = "INSERT INTO event " +
 "(event_type, employee_id, event_date) " +
 "VALUES(?, ?, ?) RETURNING event.id";
 PreparedStatement stmt = conn.prepareStatement(sql);
 stmt.setString(1, eventType.getType());
 stmt.setInt(2, employeeId);
 stmt.setTimestamp(3, eventDate);
 ResultSet rs = stmt.executeQuery();
 event_id = rs.getInt(1);
 } catch (SQLException e) {
 throw e;
 }
 try (Connection conn = DbUtil.getConnection()) {
 PreparedStatement stmt = conn.prepareStatement("INSERT INTO order_base VALUES(?, ?)");
 stmt.setInt(1, orderId);
 stmt.setInt(2, event_id);
 stmt.execute();
 } catch (SQLException e) {
 throw e;
 }
 return event_id;
 }
 @Override
 public void checkRegister() throws OrderIllegalStateException
 @Override
 public void checkCanceledOrOut() throws OrderIllegalStateException
}

With some additional logic like, order should always start with registration and that you can't cancel or ship order that already cancelled or shipped.

The reviewer didn't like that orders where polymorphic but I honestly can't imagine any other way to do it like with MVC way of 1:1 DAO:Order.

Am i missing some technique here or it's just nitpicking?

• TorbenPutkonen: Please attach the code, just posting a link to a Git repo is not enough. See the help center for more info: codereview.stackexchange.com/help
• silver_mx: Unsure what the reviewer said about the polymorphic solution, but I am thinking that he maybe expected you to use composition instead of inheritance (maybe he/she is into DDD). So it could be that your OrderEvent is your root aggregate with the base fields and then you compose your event with references to OrderRegistration and OrderCancellation, etc... This would actually map closer your database schema.
• silver_mx: Having said that, I see several things that can be improved in the code. 1) Instance fields are not private and it feels weird that you give state by setting eventType. Ideally your order could have a method register()/cancel() and that internally sets the state of the order. 2) Your order objects include persistence logic. This is technical detail and ideally should be in a separate class outside of your domain object. The goal is that your domain object include exclusively logic about the domain so that an eventual change to another database or a message queue would not impact.
• silver_mx: Regarding my first comment, what I meant is that you could have a single OrderEvent that transitions into different states depending on the internal value. For example, if OrderRegistration is given and OrderCancellation is null, then your state is registered. This would avoid the need of having multiple classes like you have now (e.g. OrderCancelled, OrderRegistered).
• silver_mx: Last issue I see is that your persistence layer is not transactional, meaning that you insert in the parent class in a connection and then in the child class in another connection. If the parent class insert succeeds but the one in the child fails then you end up in an inconsistent state. Ideally you change the database as an atomic operation, either everything succeeds or everything is rollebacked. Handling your persistence transactionally is another reason to extract the database logic in a separate class/service.

Answer 1

A major issue in your implementation is that you have included the database code into the event object in the publish method. The event objects should only contain data related to the events. You should look into single responsibility principle and separation of concerns.

Silver_mx suggested in the comments that you make an OrderEvent with references to both OrderRegistration and OrderCancellation but that leads to excessive logic where you have to manage the nullability of the references. Also it requires a lot of code changes to OrderEvent if you want to add new event types later, violating the open-closed principle.

Instead I would make the association other way: make an OrderRegistration event that has a reference to OrderEvent. And same for OrderCancellation. Maybe rename OrderEvent to OrderBaseData or similar. In the service layer you have separate services for handling OrderRegistration and OrderCancellation objects and a common (private) service for handling the base data in the same database transaction. There may be some aspects in the events that can benefit from having the same base class, but the attached source code doesn't reveal those.

• silver_mx: Right, probably having separate classes (OrderRegistered, OrderCancelled) makes the code more readable and the classes more specialized. And of course, order would be an attribute/field of the events.
• verbedt: How would you dispatch service then? By having map with Class -> Service relation or a method in Order class? Next bit is my humble opinion: All the points you brought up i think wasn't really important(except for transactional writes, that's a real shortcoming) in that task because, at least for me it's a matter of trivial rewrite in this state and all in all premature abstraction. Besides the task was for a junior level with Java Core knowledge only, i don't think it's fair to appoint architecture design to a zero real experience programmer.
• TorbenPutkonen: You didn't include the dispatching code so it does not get a review. I don't think you get to have an opinion on what is relevant or not considering that you got rejected by the interviewer and came us for help.
• verbedt: I included a link to the code, did i need to paste whole thing into op? It's OrderService calling publish in OrderEvent. Who are YOU to say what i can do i what i can't? Don't need to get emotional i have job experience and i came here just to see what the fuss was about, if you consider every job rejection as personal flaw it's your problem mate, modern jobs are monkey cruding in $FRAMEWORK anyway. Task wasn't about architecture or scalability, so it was interviewer fault to provide clarity. Unironically touch grass, man.
• TorbenPutkonen: Please refer to the help center. Linking to code repositories are explained there. Anyway, this is a service where people use their free time in order to educate you. If you are unwilling to receive the given education, it would be best if you didn't waste our time by asking in the first place. At the minimum it would be considered good manners to get acquainted with the rules of this site before engaging in combative discussion.