Intermezzo
We were asked to review the project of ese2012-team5 according to the guidlines provided here.
UML Use Case diagram:
According to criteria found here.
Controller responsible for deleting user and user's item images (delete_account)?!
Not given - direct access to model attributes. Helper objects used for functionality that doesn't clearly belong to either model or controllers.
Limited to User, Item and Database "Master" object (singleton).
Is Database as such really necessary?
It might be easier and more intuitive to just store items and users in arrays in Item and User.
Why not call the validator CredentialsValidator as that's all it can do?
No consistent invariant checking seems to be used.
Unused handler get '/buy' do
What does categories_items do? Why not use a verb in the name? Why not have an object for stuff related to categories?
# categories all ACTIVE items by their name
# @return [Array of Arrays] array with arrays for every different item.name
def categories_items
Not always used, e.g.
if password != password_conf
return "error ~ password and confirmation don't match. "
end
if password.length<length
return "error ~ password too short. "
end
if !(password =~ /[0-9]/)
return "error ~ no number in password. "
end
cloud be written with return ... if/unless ...
Why always copy params[] to local variables?
password = params[:password]
password_conf = params[:password_conf]
hash = params[:hash]
This calls for a helper method:
begin
!(Integer(price))
rescue ArgumentError
session[:message] = "error ~ Price was not a number!"
redirect '/createItem'
end
begin
!(Integer(quantity))
rescue ArgumentError
session[:message] = "error ~ Quantity was not a number!"
redirect '/createItem'
end
Not used (at least not with exceptions).
Not used, would be possible by using custom accessors/setters, even in ruby.
raise and fail never used...
Used rarely, controllers would benefit from this.
Documentation is generally good, comments are short, but informative. Really short methods where the name already gives away the purpose don't have comments, but sometimes comments are missing from methods complex enough to "deserve" and own comment. Most comments also use @param and @return semantics.
Some comments are not very descriptive, e.g:
# append image at the end
The item.split(at) method does not do what is described in the comment, the newly created item actually has the quantity at instead of rest and vice versa for the current item. For the current stage of the project this doesn't really make a difference though.
Why is this method necessary?
# move image to position 0 (profile)
# @param [Integer] position
def move_image_to_front (nr)
Class comments are missing.
"Remove" or "delete"?
# removes this item from the item list
def delete_item(item)
Most test functions have clear names, it is evident what they do/test, the use cases in store_simulation_test are transparent.
Database's test coverage is below 50%. user_test sets up instances of database.rb for every test without using it (since it is not needed for creating/manipulating/deleting users).
There are unfinished tests in database_test.