nlkluth / cs414-f17-801-tba Goto Github PK

• Design doc:
  • Package diagram: missing
• TLM: the matrix should contain all the implemented classes (21 as far as I could count)
• Refactoring and design pattern list: missing
• Code
  • next classes are excessively large and complex, due to its methods having a high cyclomatic complexity and nesting level. They use many attributes from external classes, directly or via accessor methods. They are also very non-cohesive, in terms of how class attributes are used by their methods.
    • edu.colostate.cs.cs414.tba.services.CLIController
  • next are some methods that contain duplication with others and could be refactored:
    • edu.colostate.cs.cs414.tba.services.WorkoutCreator.createWorkout(Trainer, GymSystem) and edu.colostate.cs.cs414.tba.services.WorkoutCreator.modifyRoutine(Trainer, GymSystem)
    • edu.colostate.cs.cs414.tba.services.TrainerCreator.createTrainer(Manager), edu.colostate.cs.cs414.tba.services.CustomerCreator.modifyCustomer(Manager, GymSystem), edu.colostate.cs.cs414.tba.services.CustomerCreator.registerCustomer(Manager) and edu.colostate.cs.cs414.tba.services.TrainerCreator.modifyTrainer(Manager, GymSystem)
  • Call to edu.colostate.cs.cs414.tba.domain.Equipment.equals(String) in edu.colostate.cs.cs414.tba.services.CLIController.modifyEquipment() comparing different types
  • Next classes define an equals(Object) method but not a hashCode() method, and thus doesn't fulfill the requirement that equal objects have equal hashCodes.
    • edu.colostate.cs.cs414.tba.domain.Customer
    • edu.colostate.cs.cs414.tba.domain.Equipment
  • Next fields are read but never initialized. Dereferencing these fields will likely cause a null pointer exception:
    • edu.colostate.cs.cs414.tba.services.CLIController.customerCreator
    • edu.colostate.cs.cs414.tba.services.CLIController.trainerCreator
    • edu.colostate.cs.cs414.tba.services.CLIController.workoutCreator
  • The equals(Object) method of the next classes does not check for null argument:
    • edu.colostate.cs.cs414.tba.domain.Customer
    • edu.colostate.cs.cs414.tba.domain.Equipment
    • edu.colostate.cs.cs414.tba.domain.Insurance
    • edu.colostate.cs.cs414.tba.domain.Trainer
  • Next methods present a dereference of the results of readLine() without nullcheck. If there are no more lines of text to read, readLine() will return null and dereferencing that will generate a null pointer exception:
    • edu.colostate.cs.cs414.tba.services.CLIController.addEquipment()
    • edu.colostate.cs.cs414.tba.services.CLIController.modifyEquipment()
    • edu.colostate.cs.cs414.tba.services.WorkoutCreator.createWorkout(Trainer, GymSystem)
    • edu.colostate.cs.cs414.tba.services.WorkoutCreator.modifyRoutine(Trainer, GymSystem)

Graded version: 10/11/2017

Good job! Next are a few comments to consider in the next iterations:

*** General comments ***

• Terms in the glossary should be sorted in ascending alphabetical order
• The glossary needs to describe also the attributes of the conceptual classes.

*** Manager ***

• Does the manger own customers? It might not be the best name for this association
• The next attributes are also needed here: first name, last name, address, phone number, email, id, insurance
• Doesn't the manager need credentials to access the system?

*** Trainer ***

• Doesn't the trainer need credentials to access the system?
• What is the availability attribute?
• Address seems to be a non-primitive attribute. Maybe a conceptual class is needed.

*** Customer ***

• Address seems to be a non-primitive attribute. Maybe a conceptual class is needed.

*** WorkoutRoutine ***

• Is the entire WorkoutRoutine associated to Equipment, or is the equipment used by an Exercise in the routine?

Good job! Next are a few comments to consider in the next iterations:

1. The use case diagram summarizes the use cases of the system, so it is better to place it before the use case descriptions
2. Trainer and customer as secondary actors?
3. GSU-001
   a. Trainer information to be entered to the system should be specified in the success scenario
   b. Why is the Trainer a secondary actor if he's not interacting with the system?
   c. Open issue: it's a good idea.
4. GSU-002
   a. Customer information to be entered to the system should be specified in the success scenario
   b. Why is the Customer a secondary actor if he's not interacting with the system?
5. GSU-003
   a. Equipment information to be entered to the system should be specified in the success scenario
   b. Open issue: no duplicate entries are allowed
6. GSU-004
   a. How does the manager find the trainer to be modified?
   b. Open issue: revisions are not necessary
7. GSU-005
   a. How does the manager find the customer to be modified?
8. GSU-006
   a. How does the manager find the equipment to be modified?
9. GSU-007
   a. Workout routine information to be entered to the system should be specified in the success scenario
   b. An exercise can be reused by several routines. This is not currently reflected by this use case.
10. GSU-008
    a. How does the trainer find the workout routine to be modified?
    b. Open issue: no log of modifications is necessary
11. GSU-009
    a. How does the trainer find the customer and workout routine to be linked?
12. GSU-010
    a. Little detail is provided about this use case.
13. How is it ensured that a user is a manager or a trainer?
    a. A login use case is necessary
14. Check for typos

• Great job in this iteration!
• Design doc: although the system sequence diagrams are helpful, the assignment required sequence diagrams. Please double check the slides and Larman's book to understand the difference.
• TLM: the matrix should contain all the implemented classes, not only the ones representing the domain.
• Code:
  • Consider restructuring the packages. Code and tests are usually in different folders (A1 was an exception, not the norm).