John Hofman's Design Study

From CSSEMediaWiki
(Difference between revisions)
Jump to: navigation, search
Line 28: Line 28:
 
[[image:JohnHofmanStudyClient1.0.gif|frame|centre|'''Figure 1: The initial working design for the Instant Messenger Client''']]
 
[[image:JohnHofmanStudyClient1.0.gif|frame|centre|'''Figure 1: The initial working design for the Instant Messenger Client''']]
  
−
===Description===
+
===General Description===
 
The initial design is event driven. There are two threads that respond to events, the Inbox and the GUI.
 
The initial design is event driven. There are two threads that respond to events, the Inbox and the GUI.
 
* The Inbox responds to messages from the server. It receives, decodes and executes packets that it gets from the socket. It executes the packets by calling operations on the UserManager and Chat objects.
 
* The Inbox responds to messages from the server. It receives, decodes and executes packets that it gets from the socket. It executes the packets by calling operations on the UserManager and Chat objects.
Line 41: Line 41:
  
  
−
====Class Descriptions====
+
===Class Descriptions===
 
* '''Socket/ClientSocket''': The Socket encapsulates a TCP Socket. It provides an interface to read/write packets to the socket. The ClientSocket has a different constructor because it uses the IP address of the server to connect. There are bad things about this part of the design and will be discussed below.
 
* '''Socket/ClientSocket''': The Socket encapsulates a TCP Socket. It provides an interface to read/write packets to the socket. The ClientSocket has a different constructor because it uses the IP address of the server to connect. There are bad things about this part of the design and will be discussed below.
 
* '''Inbox''': Inherits from a thread class. Reads packets from the ClientSocket, unpacks them and executes operations on the Chats and the UserManager. The Inbox also contains a map that links Chat objects to their ChatID's (This is nasty, the inbox should not have this map).  
 
* '''Inbox''': Inherits from a thread class. Reads packets from the ClientSocket, unpacks them and executes operations on the Chats and the UserManager. The Inbox also contains a map that links Chat objects to their ChatID's (This is nasty, the inbox should not have this map).  
Line 55: Line 55:
 
* '''Fl_WindowedUserManager''': Encapsulates the GUI for a UserManager. Is an observer of a UserManager. Implemented using fltk-1.1.9
 
* '''Fl_WindowedUserManager''': Encapsulates the GUI for a UserManager. Is an observer of a UserManager. Implemented using fltk-1.1.9
  
−
==Design Review==
+
==Initial Design Review==
 +
 
 +
===Bad Things to be Corrected===
 
There are several areas of the design that are bad and need to be reviewed.
 
There are several areas of the design that are bad and need to be reviewed.
−
* '''The Inbox contains a list of Chat objects''' - This breaks the [[Single responsibility principle]], Inbox is responsible for executing incoming packets, not constructing and mapping Chat objects to ChatID's. Removing this responsibility from the Inbox requires a new class to manage Chat objects, a ChatManager. The interface of the ChatManager follows the [[Tell, don't ask]] principle by passing operation calls to the correct Chat without revealing the actual Chat objects.  
+
 
−
* '''Inbox switch smell to execute packets''' - I don't know which solution is better, or which side effect is worse, a [[Switch statement smell|Switch smell]] or a cyclic dependancy.
+
====The Inbox Contains a Map of Chat objects====
−
** The current solution is a switch statement based on the command type of packet received. A Packet is read from the socket as a string, the string is tokenised into the packet data structure. One of the fields in the Packet is the command type enumeration.
+
This breaks the [[Single responsibility principle]]. Inbox should be responsible for executing incoming packets, not constructing and mapping Chat objects to ChatID's. Removing this responsibility from the Inbox requires a new class to manage Chat objects, a ChatManager. The interface of the ChatManager follows the [[Tell, don't ask]] principle by passing operation calls to the correct Chat without revealing the actual Chat objects.  
−
**[[Polymorphism]] is an alternative solution. Make Packet into an abstract base class with concrete sub classes for each command type. However, the switch smell would still exist where the Packets were constructed. In addition, the Packets would need added behavior to execute themselves, including dependancies on the UserManager, Chats etc. This new dependency will introduce a cyclic dependency (Packet->UserManager/ChatManager->Outbox->Packet).
+
 
 +
====UserManager Handles Local User Login Status====
 +
This also breaks the [[Single responsibility principle|single responsibility principle]]. The UserManager records online users not the Login protocol of the local user. This extra responsibilty can be encapsulated in a LocalUser class. 
 +
 
 +
====ClientSocket Weirdly Inherits Concrete Socket Base Class====
 +
The Socket class was shared across the client and server programs, it has concrete implementation of the Send() and Receive() operations. The ClientSocket only has a different constructor, so it can connect to an existing socket. This breaks the [[Avoid concrete base classes]] principle. To make this design extensible [[Polymorphism|polymorphism]] should be used to change the behavior of the Socket. Different protocols may have very different implementation of socket connection, sending and receiving, so a abstract base class is better. Socket should only define the interface.  
 +
 
 +
====Inbox Switch Smell for Executing Packets====
 +
The current solution is a switch statement based on the command type of Packet received. A Packet is read from the socket as a string, the string is tokenised into the Packet data structure. One of the fields in the Packet is the command type enumeration. The Packet data structure is also used by the Outbox. The Outbox builds a Packet with the appropriate command type.
 +
 
 +
[[Polymorphism]] is an alternative solution. Make Packet into an abstract base class with concrete sub classes for each command type. However, there are several things to consider:
 +
* The switch smell would still exist where the Packets were constructed, in ClientSocket and Outbox.
 +
* Packets would need added behavior to execute or serialise themselves. This introduces dependancies on the UserManager, Chats etc, and dependency on the serialisation strategy.
 +
* Some packet types are only received, or only sent, some both. C++ allows multiple inheritance so we could make two Packet base classes (Send and Receive), or have one base class and some Packet types where Serialise() or Execute() do nothing.
 +
 
 +
My chosen solution is to make a single abstract Packet base class that has an interface for Execution and Serialisation of a Packet. Each Packet type will be a concrete Packet subclass. On the downside, some of the Packets will have unimplemented, unused, Execute() or Serialise() operations. But this removes the requirement for a type switch in Inbox and Socket, and the requirement of a complex multiple inheritance hierarchy. It also makes the program easier to extend because new Packet types with different execution behavior, contained data, or serialisation strategies could be implemented and used in the program following
 +
 
 +
This leads into the serialisation strategy. It comes to mind that the strategy pattern could be used to remove the concrete implementation of the serialisation from each Packet subclass. However, the serialisation process needs access to the data in the Packet which is achieved by either:
 +
* Defining the interface of the strategy operation so the data is passed as arguments to the serialisation process, OR
 +
* Providing a common interface for all Packets that lets the serialisation operation get the data it needs.
 +
Both these approaches restrict the possible data contained in a Packet subclass to adhere to the interface. This removes any gains in extensibility. Therefore each Packet subclass will have its own concrete implementation for serialisation. Other strategies will require new Packet subclasses.
  
  
  
 
===Stuff that needs fixing, maybe?===
 
===Stuff that needs fixing, maybe?===
−
* Chat map in Inbox. Fix: Add chat manager but get (law of demeter vs repeat code)  e.g. chat_manager.getChat(id).Receive(message) vs ChatManager having the same interface as chat (except also with a ChatID function parameter) so it can pass the call to the correct chat.
 
−
* Switch smell in Inbox to deal with packets? Polymorphism..? Which means that Socket will need a switch.
 
−
* Chat doesn't record participants, might need that for other protocol (YAGNI vs OCP)
 
−
* Socket/ClientSocket inheritance is weird, socket should just be an interface so that it is extendable.
 
−
* Packet should know how to serialise itself.
 
 
* Outbox shouldn't record who is logged in.
 
* Outbox shouldn't record who is logged in.
 
* Names of classes are a bit meh.
 
* Names of classes are a bit meh.
 
* State switch in Fl_WindowedUserManager, switch statement smell -> State pattern...
 
* State switch in Fl_WindowedUserManager, switch statement smell -> State pattern...
−
* Thread inheritance by inbox, Bad?
+
 
 +
* [[Don't burn your base class|Burning base class]] with observer, subject and thread?
 +
* [[Keep related data and behavior in one place|Data and functionality not is same place]] for GUI, observer pattern requires getters on subject class. Alright because the GUI functionality is not related to the actual Chat's or UserManager
  
 
Good Stuff:
 
Good Stuff:
 
* observer decouples GUI from implementation, coupled with Factory means you can build whatever kind of GUI you fancy.
 
* observer decouples GUI from implementation, coupled with Factory means you can build whatever kind of GUI you fancy.
 +
* command query separation used, all functions that return something take no parameters. Works because its an event driven system, most operations are void-return commands.
 +
  
 +
==TODO==
 +
* Make ChatManager
 +
* Rename getters and setters
 +
* Rearrange socket and client socket
  
 
==Other Stuff==
 
==Other Stuff==
 
[[John Hofman US001| Simple chat]]
 
[[John Hofman US001| Simple chat]]
 
[[John Hofman's Log|Log]]
 
[[John Hofman's Log|Log]]

Revision as of 11:58, 31 August 2010

Contents

My Project

This design study was introduced by my ENEL428 software assignment. The purpose of the assignment was to design a prototype of an Instant Messenger System using concurrent programming. The system was broken into two separate parts a client and a server. This design study is regarding the client program.

Design Study

The initial system uses a simple login model, a user attempts to log in with just a username which the server accepts or rejects.

Functional requirements of the client:

  • Connect to a server.
  • Login.
  • Logout.
  • Display the other users online.
  • Start a conversation with another online user.
  • Post Messages in a conversation.
  • Invite other online users to a conversation.
  • Leave a conversation.

Constraints:

  • C++
  • Uses POSIX threads for concurrency.

Room for Expansion:

  • Other protocols, XMPP etc.
  • Other GUI implementations, currently uses fltk-1.1.9

Initial Design

This is the design which is fully functional.

Figure 1: The initial working design for the Instant Messenger Client

General Description

The initial design is event driven. There are two threads that respond to events, the Inbox and the GUI.

  • The Inbox responds to messages from the server. It receives, decodes and executes packets that it gets from the socket. It executes the packets by calling operations on the UserManager and Chat objects.
  • The GUI responds to input from the user. It calls operations on the UserManager and Chat objects according to the input from the user.

The UserManager and Chats use Mutexs to synchronise access to their data. They also use the Outbox in some operations to write packets to the socket. The relationships between the classes are designed to remove any cyclic dependancies from the chain of operations triggered by an event from the GUI or server:

Inbox->Socket

Inbox/GUI->UserManager/Chat->Outbox->Socket


Class Descriptions

  • Socket/ClientSocket: The Socket encapsulates a TCP Socket. It provides an interface to read/write packets to the socket. The ClientSocket has a different constructor because it uses the IP address of the server to connect. There are bad things about this part of the design and will be discussed below.
  • Inbox: Inherits from a thread class. Reads packets from the ClientSocket, unpacks them and executes operations on the Chats and the UserManager. The Inbox also contains a map that links Chat objects to their ChatID's (This is nasty, the inbox should not have this map).
  • Chat: Each chat represents a conversation with another user. The chat maintains a list (history) of messages but not the participants of the conversation (YAGNI vs OCP?). Chats are subjects in the Observer pattern so they notify their observers whenever their state changes.
  • UserManager: The UserManager keeps a record of the other users online. The server pushes updates to the client when users log on and off so they can be added and removed from the UserManager. The user manager also handles the login/logout actions of the local user. The UserManager is a subject in the Observer pattern so it notifies its observers if its state changes.
  • Outbox: The outbox builds packets to write to the socket. It is used by Chats and the UserManager to send information to the server. The outbox currently records the userID of the local user so it can tag outgoing packets, this seems like it shouldn't be there. (If it wasn't there then the outbox is just behavior without state, except a reference to the Socket)
  • Mutex: Wraps a POSIX threads mutex and condition variable. The interface can lock and unlock the mutex. I also implemented an interface to wait and signal the condition variable, but that is unused in the current design. This breaks YAGNI but makes the Mutex class more reusable for future concurrent designs.


GUI:

  • Factory/Fl_Factory: Sort of implements the factory pattern, in a strange way. It makes Chats and UserManagers and also makes the associated Fl_Window'ed objects (Fl_WindowedChat and FL_WindowedUserManager) which are then linked with the observer pattern.
  • Fl_WindowedChat: Encapsulates the GUI for a Chat. In an observer of a Chat. Implemented using fltk-1.1.9
  • Fl_WindowedUserManager: Encapsulates the GUI for a UserManager. Is an observer of a UserManager. Implemented using fltk-1.1.9

Initial Design Review

Bad Things to be Corrected

There are several areas of the design that are bad and need to be reviewed.

The Inbox Contains a Map of Chat objects

This breaks the Single responsibility principle. Inbox should be responsible for executing incoming packets, not constructing and mapping Chat objects to ChatID's. Removing this responsibility from the Inbox requires a new class to manage Chat objects, a ChatManager. The interface of the ChatManager follows the Tell, don't ask principle by passing operation calls to the correct Chat without revealing the actual Chat objects.

UserManager Handles Local User Login Status

This also breaks the single responsibility principle. The UserManager records online users not the Login protocol of the local user. This extra responsibilty can be encapsulated in a LocalUser class.

ClientSocket Weirdly Inherits Concrete Socket Base Class

The Socket class was shared across the client and server programs, it has concrete implementation of the Send() and Receive() operations. The ClientSocket only has a different constructor, so it can connect to an existing socket. This breaks the Avoid concrete base classes principle. To make this design extensible polymorphism should be used to change the behavior of the Socket. Different protocols may have very different implementation of socket connection, sending and receiving, so a abstract base class is better. Socket should only define the interface.

Inbox Switch Smell for Executing Packets

The current solution is a switch statement based on the command type of Packet received. A Packet is read from the socket as a string, the string is tokenised into the Packet data structure. One of the fields in the Packet is the command type enumeration. The Packet data structure is also used by the Outbox. The Outbox builds a Packet with the appropriate command type.

Polymorphism is an alternative solution. Make Packet into an abstract base class with concrete sub classes for each command type. However, there are several things to consider:

  • The switch smell would still exist where the Packets were constructed, in ClientSocket and Outbox.
  • Packets would need added behavior to execute or serialise themselves. This introduces dependancies on the UserManager, Chats etc, and dependency on the serialisation strategy.
  • Some packet types are only received, or only sent, some both. C++ allows multiple inheritance so we could make two Packet base classes (Send and Receive), or have one base class and some Packet types where Serialise() or Execute() do nothing.

My chosen solution is to make a single abstract Packet base class that has an interface for Execution and Serialisation of a Packet. Each Packet type will be a concrete Packet subclass. On the downside, some of the Packets will have unimplemented, unused, Execute() or Serialise() operations. But this removes the requirement for a type switch in Inbox and Socket, and the requirement of a complex multiple inheritance hierarchy. It also makes the program easier to extend because new Packet types with different execution behavior, contained data, or serialisation strategies could be implemented and used in the program following

This leads into the serialisation strategy. It comes to mind that the strategy pattern could be used to remove the concrete implementation of the serialisation from each Packet subclass. However, the serialisation process needs access to the data in the Packet which is achieved by either:

  • Defining the interface of the strategy operation so the data is passed as arguments to the serialisation process, OR
  • Providing a common interface for all Packets that lets the serialisation operation get the data it needs.

Both these approaches restrict the possible data contained in a Packet subclass to adhere to the interface. This removes any gains in extensibility. Therefore each Packet subclass will have its own concrete implementation for serialisation. Other strategies will require new Packet subclasses.


Stuff that needs fixing, maybe?

  • Outbox shouldn't record who is logged in.
  • Names of classes are a bit meh.
  • State switch in Fl_WindowedUserManager, switch statement smell -> State pattern...

Good Stuff:

  • observer decouples GUI from implementation, coupled with Factory means you can build whatever kind of GUI you fancy.
  • command query separation used, all functions that return something take no parameters. Works because its an event driven system, most operations are void-return commands.


TODO

  • Make ChatManager
  • Rename getters and setters
  • Rearrange socket and client socket

Other Stuff

Simple chat Log

Retrieved from "https://oowisdom.csse.canterbury.ac.nz/index.php?title=John_Hofman%27s_Design_Study&oldid=7011"
Personal tools