Skip to content
meilae edited this page Nov 14, 2012 · 17 revisions

Team4's project


Reverse-engineering: Use-case-diagramm

use-case-diagramm


Code review

Design

  • Violation of MVC layers:
    • No violation of MVC
  • Usage of helper objects between view and model
  • Rich OO domain model and clear responsibilities:
    • The responsibilities of each class aren't very clear, because there are only few model classes
    • responsibility of data management spread out on many classes(item, user, holding)
  • Sound invariants:
    • No invariants implemented.
  • Overall code organization & reuse, e.g. views:
    • Classes are unclearly structured.

Coding style

  • Consistency:
    • The requires aren't consistent in every class.
    • see item.rb: 6 and user.rb: 1 ( the requires are inside or outside the class or module)
  • Intention-revealing names:
    • Some method names aren't logically chosen, so confusion is caused.
    • see user.rb: 232 (login method seems to set the password?!)
  • Ruby idioms:
    • Not everywhere Ruby idioms are used because of unfamiliarity.
    • see user.rb:154 (blocks would be nice here)
  • Do not repeat yourself
  • Exception, testing null values:
    • Null values are tested.
  • Encapsulation
  • Assertion, contracts, invariant checks:
    • No assertion, contracts and invariant are implemented.
  • Utility methods:
    • The methods are mostly small, so repetitions of codes are minimized.

Documentation

  • Understandable:
    • Documentation of classes are very well understandable.
  • Intention-revealing:
    • Special cases aren't noted. There a lot of unnecessary documentation and some complicated methods aren't documented. Some parts of the documentation don't match with the type of the parameters (inconsistency)
    • see item.rb: 175 (itemid stated as Integer but actually a String is passed)
  • 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:
    • A lot of the tests are unnecessary, because they test the same cases.
  • Readability