Skip to content

First delivery review

Jump to bottom Edit New page
Masterxilo edited this page Dec 16, 2012 · 22 revisions

Review Team 3

Replies: in bold

Design 3/4

  • Layers are mostly respected. Views do no contain significant logic, controller are mostly about routing and parameter handling, and the model deals with validation and relationships.

  • Still, there's slightly too much logic in controllers

    1. Logic in under path / to aggregate activities of followers should be in User class (model) DONE
    2. Logic to change status of an item should be entirely in model Added change_status method to item model, although I kept the old "activate" method (for adding new items and stuff)
    3. If-test to toggle inclusion/exclusion of item in wish list should be in model, if necessary in a Whishlist entity. Added change_wishlist method to agent, removes item if present, adds it if not.
    4. addable_users = User.all.select {|u| !u.is_member_of?(@org)} --> move to User.all_outside_organization(org)? DONE
    5. etc. There are several small pieces of logic that could be easily moved in the model.

  • Several if-test around to distinguish between the case when current agent is a user or an organization

    1. tests aren't always the same (e.g. metaprogramming respond_to?('following'), @current_user != @current_agent) DONE sorry mx, same time. I just replaced @current_user != @current_agent by noop implementation, rothfahl
    2. A proper OO solution should be possible with a common interface, and different implementations (possibly no-op implementation) of the methods (following, add_orgactivity). This kind of design doesn't scale. DONE
    3. Tests to track activity of user or organization are replicated multiple times in controllers. This is error prone and lacks encapsulation. You can model this elegantly with the observer pattern: user and item send notifications to the observer(s) when a change occurs. When the user works on behalf of the organization, you register the organization as observer. When he shops for himself, there is no observer. Nice but much effort

  • If feel like the treatment one images (to upload/replace) is not fully consistent. Either the model has only paths, and the actual storage is handled somewhere else, or the model deals with files and their storage all the time. Seems like it's mixed, e.g. Organization#delete_image_file remove the file on the disk, but the storage is in add_image in the controller… would require too many changes to our design

  • Why not always validate values in the model? E.g. image size, password strength could be validated in the corresponding entity for better encapsulation. If validation fails, an exception is raised, caught be the controller, and an error message is displayed. Would indeed be nice, but too much effort now

  • Activities use a list of symbol. Instantiation is not always consistent :message => "activated #{@item.name} vs. :message => "activated item #{@item.name}. Why not have a few factory methods createActivation(…) or why not use inheritance? DONE

  • File organization: mostly good. For real production, I would not store upload pictures in /public but in a separate folder and have rules in the web server to rewrite the URL or so. But it's fine for ESE.

Coding Style 1.5/2

  • Code is easy to read, follows mostly ruby idioms (?, snake_case) and formatted in a way that is easy to skim through. Names are mostly intention revealing.

  • The use of helps seems sound. It seems like you already considered XSS, that's good.

  • The line @org = Organization.organization_by_id(params[:id].to_i) appears frequently. Could it be moved into a before handler? DONE

  • DONE Mostly uses list comprehension, except in few places, e.g. is_admin?

  • Some missing checks, and some ugliness, but there are TODO (e.g. User#delete, User#profile_route). Better than nothing ;) Reasoned with all TODOs

  • (Very minor remark) For slightly more elegant version (subjective interpretation) of the password checker, you could consider each check as a "filter". The checker is then a pipeline of filters, and the password is strong if it passes all filters. The filters can be modeled either with objects, or anonymous lambdas, e.g.

      filters.add ( "Must be longer than 6", { |pwd, user| pwd.length > 6 )
  filters.add ( "Must not contain username", { |pwd, user| pwd.includes(user)! )
  ...

  • Utility function could be moved to a dedicate place, and should not depend on elements form other layers. It would be better to pass the image file and image data in the parameters, so the function is truly agnostic of the view:

      # Expects image file to be in params[:image_file]
  def add_image(rootdir, id)
  file = params[:image_file]

Done & all utility functions are in helpers.rb now

  • The views are split in different files. Apparently, files starting with underscore are nested in other files. I like it. Thanks!

Documentation 1/2

Documentation much stronger now.

  • Class documentation seem inconsistent. The stories are copied in some classes, in some other there a short description, in some other nothing.

  • Constructor seems to be consistently documented with the parameters they accept. Other methods have "random" documentation.

  • Trivial methods are documented e.g. User#user_by_name, but less trivial not e.g. User#delete (does it delete the item as well?). The contracts of methods is not clear.

  • Overall: I wouldn't trust too much this documentation, that seems not very consistent, and covers only trivial cases.

Tests 3/4

  • Names are mostly good and intention revealing.
  • Test case are clear and small
  • No test data in setup (except the user), instead test methods create test data. Maybe having a common set of test data would help?
  • Some trivial tests (e.g. initialization, basic properties), but also tests for real logic/invariants (cascade delete of user and its item, ownership change).
  • Not clear the overlap between tests for user and organization. Things the similarities between the two, I'm not clear whether there should be 3 sets of tests: for features common to user and org, only for user, only for org. Created an agent_test class that tests common features, but they have not yet been removed from the original tests
  • Probably some tests are missing, for instance adding/removing comments (I could not spot this) Do we have some for adding? Removing is not possible. Added test for adding comments. I think removing comments was never required, right? <-- MX: Right.

Notes

  • The password check was just unusable to me. However if the customer is happy, it's fine. Essentially, it promotes cryptic and hard to memorize passwords (forbidding 'in' or 'we' is way to fine grain I think) and it took me 10 attempts to find a password to checked! Raised length limit for know words to 3

Customer evaulation (11/12)

A full set of features (9) and good installation instructions (1). General customer feeling was mostly okay, but sometimes they argued too technical. I also wished they would ask more questions when discussing the stories, as then the exact requirements are better defined (1.5).

Add a custom footer
Add a custom sidebar
Clone this wiki locally