-
Notifications
You must be signed in to change notification settings - Fork 1
Intermezzo Review
There is some logic that could be handled by the model instead of the controllers. Examples for this are:
- Type casting and stripping input in controllers
- Huge error handling in main_ctrl instead of in a separate class
- More error handling in register_ctrl and item_ctrl
Apart from that, it is visible that you tried to keep the logic in the model with very well chosen helper classes like the string_checker.
Some of the views do contain some checks, but they help sending less parameters to the controllers and are well chosen. Otherwise there is no logic in the views, as it should be.
All the objects do have clear responsibilities. The helper classes do have one single responsibility, the Trader does have more than one responsibility (keeping track of it's items, keeping track of all the traders in the system, providing services for trading items and creating and editing items), but as you explained in your presentation you had good reason for doing this and we support this solution as well.
You did check all the user inputs, but you did not create invariants in your objects. Invariants help creating a safe code and we recommend implementing and checking these - at least in the core models ( Trader, Item, ...).
There is only minimal amount of duplicate code in the views. We like the partial haml file for editing an item very much.
One note on the design of the website: You could rework the display of the timestamps on the Store page, like you did on the Activities page. Otherwise the page looks quite nice and seems to run stably.
Generally, we perceived the coding style in the Models as being astonishingly good. Not only are the Methods sensibly named (apart from the swing_hammer_of_doom method of the class TradingAuthority, which, while admittedly fittingly named, could be annoying for some), they are (in most cases) short, precise and easy to understand. Classes are very well organized, we are particularly fond of the way methods of the class and the instance are separated in the code. There is very little to no duplicate code and coding style is consistent and clean. Encapsulation does not seem to be a problem, most often the accessible attributes of objects are sensibly chosen. In the controllers, GET, POST, PUT and DELETE seem to be handled correctly and the clean style seen in the models continues.
There are some issues, however, which are worth the mention. For example, class invariants are often neglected. Moreover, boundary conditions are mostly checked in the controllers, and while that may work well, it makes using the models harder if you are not very familiar with them.
At the beginning of each class, its responsibilities are listed, but we miss one responsibilities of Trader: it keeps track of all the traders in the System as well.
The methods are documented where needed and create understanding of what the methods are for. But the method documentation always comes without the pre- and postconditions you make. You expect the user of your model to cast the inputs to the right types and check the boundary values before he invokes the method, otherwise the system crashes because of "fail" statements. We don't consider this being bad coding style, but the contract must be declared in the documentation.
Sometimes the documentation could have been more thorough and descriptive, but they are clear and understandable and you don't mess up the vocabulary.
Test coverage is high enough (68% for the models), but you already knew that. The tests are easily readable and their name clearly states what they are testing. However, I can't shake the feeling that boundary conditions and class invariants are insufficiently tested. Considering that such "critical" input values are handled in the controllers, testing them would not have been very useful, though. The tests seem to be well conceived in the sense that they represent real and frequent scenarios and are quite thorough.
We generally liked your code as well as the website itself. It proved to be quite nice to work with (apart from minor hiccups, which were to be expected) and we would like to congratulate you on your work.
Greetings, Team 4