Active record pattern implementation #206
Conversation
|
I will review it this weekend. |
| if (this.calculateMagicPower() >= 10.0) { | ||
| System.out.println(this.toString() + ": a fireball spell is casted"); | ||
| } else { | ||
| throw new SpellCastException("Can not cast lighting! Not enough magic power..."); |
There was a problem hiding this comment.
A typo here, the message should be Can not cast fireball!.
|
We should document disadvantages one of them is violation of SRP, the object knows how it is going to be persisted. How do we manage transaction with Active record? ActiceJDBC does it, but I haven't got time to go in depth. Something about transaction should be said because some real applications rely heavily on transactions. |
|
It's disputable point about SRP violation as we just put all the logic including domain and persistence related to some record into one class. |
| statement.close(); | ||
| connection.close(); | ||
| } | ||
| catch (final SQLException | ClassNotFoundException e) { |
There was a problem hiding this comment.
Style nit: catch should be in the line of closing try curly brace.
try {
...
} catch (Exception ex) {
...
}I personally prefer Google style guide. I will also raise this point in chat room. What do you prefer here?
There was a problem hiding this comment.
Fixed. I do not have any particular preferences :)
|
Regarding Transaction management, though it is an orthogonal topic but it will affect the implementation. Imagine what would we do if the example was about |
|
@stephen-lazarionok I have a few doubts and need your clarification on them. Please go through my comments and respond. After that I think we are good to go. |
|
@npathai I commented on your points. I guess it's worth discussing the point about transactions and patterns related to DBs. I checked out other patterns implemented earlier and did not find any references to transactions. My opinion is still that transactions is a separate topic and we should not mix it up with patterns. We could add this information later if needed. |
|
@stephen-lazarionok Yes as far as the previous patterns are concerned, they were created by very few people maybe by @iluwatar and now we have a team. So we can improve upon many fronts. We will be happy to hear your thoughts on transactions on previous patterns as well it's something worth mentioning. Also in future we will be visiting previous patterns and improve upon them. Open points:
|
|
@iluwatar LGTM. If you are satisfied with implementation then we can proceed with README and web site changes. |
|
Any updates @stephen-lazarionok ? |
|
Sorry, was busy other stuff on my projects. Will take care of that this weekend. |
|
Had to take care of some urgent stuff this weekend, will do tomorrow. |
|
@stephen-lazarionok Not a problem 👍 |
|
Format code according to Google Style Guide ---> done |
|
@stephen-lazarionok How can I help? Did you face any issue while generating the diagram? |
|
Will install Eclipse and fix soon. Sorry, do not have enough time as I am in the middle of my trip and also have stuff to do on my projects. |
|
Not an issue :) take ur time.. |
|
This pull request made no progress during the last 30 days, so I will close it. |
* Added explanation and example of public IP address * Removed the IP and ISP bit * Added information about APIPA
No description provided.