P4: Feedback

[lmorenoc]

• Great job in this iteration!
• TLM: a spreadsheet would be easier to navigate
• 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.p4.banqi.BanqiGame
  • edu.colostate.cs.cs414.p4.client_server.server.session.ClientSession
  • edu.colostate.cs.cs414.p4.client_server.server.Server
  • edu.colostate.cs.cs414.p4.console.PlayerConsole
  • Next are classes whose public interface is large and used non-cohesively by client methods, i.e., disjoint groups of client classes use disjoint fragments of the class interface in an exclusive fashion. You could explore seemingly unused services (i.e., public methods that don't seem to be used at all in the system) and remove them. Also, you could explore seemingly local services (i.e., public methods that seem to only be used from inside the class) and make them private.
  • edu.colostate.cs.cs414.p4.client_server.client.Client
  • The equals(Object) method of the next classes always returns false. This means that an object is not equal to itself, and it is impossible to create useful Maps or Sets of this class. More fundamentally, it means that equals is not reflexive, one of the requirements of the equals method. The likely intended semantics are object identity: that an object is equal to itself. This is the behavior inherited from class Object. If you need to override an equals inherited from a different superclass, you can use: public boolean equals(Object o) { return this == o; }
  • edu.colostate.cs.cs414.p4.client_server.server.session.ClientSession
  • edu.colostate.cs.cs414.p4.client_server.server.session.ServerSession
  • Apparent infinite recursive loop in edu.colostate.cs.cs414.p4.console.AbstractConsole.accept(). This method unconditionally invokes itself. This would seem to indicate an infinite recursive loop that will result in a stack overflow.
  • Next classes define equals(Object) but no hashCode()
  • edu.colostate.cs.cs414.p4.banqi.Square
  • edu.colostate.cs.cs414.p4.user.Profile
  • edu.colostate.cs.cs414.p4.client_server.server.session.ServerSession.toString() returns null. A liberal reading of the spec could be interpreted as allowing this, but it is probably a bad idea and could cause other code to break. Return the empty string or some other appropriate string rather than null.
  • Some of the edu.colostate.cs.cs414.p4.test.ProfileTest tests do not have any assert
  • Dereference of the result of readLine() without nullcheck in edu.colostate.cs.cs414.p4.client_server.transmission.game.invite.InviteTask.oldrun(). If there are no more lines of text to read, readLine() will return null and dereferencing that will generate a null pointer exception.

[Pflagert]

Regarding "edu.colostate.cs.cs414.p4.console.AbstractConsole.accept(). This method unconditionally invokes itself. This would seem to indicate an infinite recursive loop that will result in a stack overflow."

This infinite recursive loop is intentional (definitely not good practice). However, there is really only one condition that will trigger the recursion and that's when the server's JVM process terminated. Thanks to the blocking nature of "readLine()" it is unlikely that this method will result in a stack overflow, but I agree that this method needs improvement.

The recursive nature of this method is to allow for a potential new command "connect-to-server". Which should allow a user to switch between servers (especially useful in the event of one server disconnecting).

If the accept() method does not recurse when a server goes down, then the user's process will terminate abruptly (as this method is performing the final instructions).

What would be the best way to address this issue?