Review

UML

Can be found here and here

#Code Review

Guidelines

Design

• Violation of MVC layers
• The view don't implement any business process
• The controller don't implement any business process
• Usage of helper objects between view and model
• Rich OO domain model
• Clear responsibilities
• Each object has one or maximal two responsibilities
• Sound invariants
• Invariants are defined
• Invariants cover important aspects
• Overall code organization & reuse, e.g. views
• No code is doublicated
• Code is reused where possible

Coding style

• Consistency
• Intention-revealing names
• Ruby idioms
• Do not repeat yourself
• Exception, testing null values
• Encapsulation
• Assertion, contracts, invariant checks
• Utility methods

Documentation

• Exists?
• Understandable
• Intention-revealing
• Describe responsibilities
• Match a consistent domain vocabulary

Test

• Clear and distinct test cases
• Number/coverage of test cases
• Easy to understand the case that is tested
• Well crafted set of test data
• Readability

Controller Review

Design aspects

There are from time to time code parts which handle business processes, meaning that they should belong to the model not to the controller according to the MVC model. The responsibilities are clear and can be mostly guessed from the name of the controller.

Coding style

The controllers are in general consistent except the part of the error handling which is managed in two different ways in the controllers. The names of the variables are straightforward as well as the names of the views which are needed for the post/get statements so the intention of the code parts can be easily derived. Possible exceptions are being handled and nil values are taken into account. The handling always consists of a feedback to the user of the application so that the action can be redone. Preconditions are not always being checked for, thus there is the possibility that an error can occur which could have been easily avoided.

Documentation

Well, there isn't a lot of documentation in the controllers although it is sometimes desirable. The given documentation reveals the inten- tions of the following code part quite well. The responsibilities are rarely stated, especially the responsibility of the controller itself.

Models Review

Design

There should be some more invariants, especially in organization.rb and item.rb. The distribution of responsibilities between the classes seams quiet clean and there is also a good reuse of code through inheritance (agents). There are however always the same class methods, that keep repeating. But I can think of no better solution.

Coding style

Talking about class methods, what I find slightly irritating is that sometimes you start your id counter by 0 and other times by 1. I find it most confusing in user.rb where the counter is initially 0 and after .delete_all it is reset to 0. Naming of methods are in a way that help understand the code. There is however the issue, where within the method you use 'user' when you actually mean 'agent' or 'organisation', as far as I understand it (see organisation.rb).

Documentation

The class documentation is very scarce to almost non existing. Although the documentation for methods are provided, it'd be very nice, if there was a description for each class, explaining its main responsibilities and its usage.

Views Review

Design aspects

It is good that the requested URLs and the name of the views correspond, this makes it easier to search for a specific piece of code. Most names of the views are quite self-explanatory (some exceptions: setting.erb should be user_settings.erb and layout.erb seems to send messages to the user and sets up the main layout). What is not so beautiful is that those messages in layout.rb are hard coded. I would suggest a more flexible solution. You could do this via session:

Create an Alert class:

class Alert {
  attr_accessor :title, :message

  def self.create(title, message)
    alert = Alter.new
    alert.title = title
    alert.message = message

    alert
  end
}

Pass it like this:

if ( your_condition)
    session[:alert] = Alert.create("Title", "Message")
    redirect "/?alert"
end

Then check if an alert is set in layout.rb

<% if ! session[:alert].nil?%>
<div class="alert-error alert" id="alert"><strong><%= "#{session[:alert].title}" %></strong><br />
<%= "#{session[:alert].message}" %></div>

<% session[:alert] = nil
 end %>

With that solution you probably could leave as well that fancy java-script stuff to hide all those hard coded messages and you wouldn't even need the "?alert" part.

There are 16 views with the prefix "_", which seem to be reused in various other parts so the code reuse is quite big. Because of the great amount of views I would suggest to make subfolders, but its also ok the way it is.

Coding style

The views are quite consistent. The names of the variables are most of the time well chosen. At the beginning of most views is a check if the parameters are null, if they are they get a default value assigned, if not they keep their assigned value. This is done with a conditional operator. Generally some more empty lines would be good because now sometimes the code is hard to read.

Documentation

It would really help if there was a short description of what a view should show. Some of the parts could be understood easier if you add some really short comments above a bit of code that performs a functionally more or less independent part within the file. The description of the necessary parameters of a view are very helpfull but why was sometimes "*" and sometimes ":" used to list the parameters? I assume * is required and : is optional, but I am not sure since in _agent_list.rb version seems to be a parameter that is both. Perhaps a explanation of these symbols is needed in the readme.