John Hofman's Design Study

From CSSEMediaWiki
(Difference between revisions)
Jump to: navigation, search
(Design Study)
 
(29 intermediate revisions by one user not shown)
Line 3: Line 3:
  
 
=Design Study=
 
=Design Study=
The initial system uses a simple login model, a user attempts to log in with just a username. The server accepts a login attempt if the username is not already taken. When a user logs out their username is freed for anyone else to use.  
+
The initial server-client system uses a simple login model, a user attempts to log in with just a username. The server accepts a login attempt if the username is not already taken. When a user logs out their username is freed for anyone else to use. The client and server communicate using a basic protocol. Commands are serailised packets containing a command type and any required data (username, message, chat ID's etc).  
  
 
Functional requirements of the client:
 
Functional requirements of the client:
Line 9: Line 9:
 
* Login.
 
* Login.
 
* Logout.
 
* Logout.
* Display the other users online.
+
* Display the other users online - receive updates from the server.
* Start a conversation with another online user.
+
* Start a conversation with another online user - started by the local user, or an online user.
* Post Messages in a conversation.
+
* Post messages in a conversation.
 +
* Receive messages in a conversation - from other users via the server.
 
* Invite other online users to a conversation.
 
* Invite other online users to a conversation.
 
* Leave a conversation.
 
* Leave a conversation.
Line 24: Line 25:
  
 
==Initial Design==
 
==Initial Design==
This is the design which is fully functional.
+
This is the first design I developed and it is fully functional. I started out trying to do a really good OO design but found it difficult to get started. So I developed this design for submission for my other paper which is focussed on concurrent design. I'm now going to review the OO aspect of this design and try to improve it. 
  
 
[[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''']]
Line 34: Line 35:
  
 
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 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===
 
===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).  
* '''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.
+
* '''Chat''': Each chat represents a conversation with another user. The chat maintains a list (history) of messages. 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.
 
* '''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)
+
* '''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.
* '''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 [[You ain't gonna need it|YAGNI]] but makes the Mutex class more reusable for future concurrent designs.
+
  
  
 
'''GUI''':
 
'''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.
 
* '''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.
 +
* '''Observer/Subject''': These classes implement the [[Gang of four|GoF]] [[Observer|observer design pattern]]. C++ allows multiple inheritance so the Observer and Subject base classes can be inherited without [[Don't burn your base class|burning my base class]] of Chat, UserManager or the Fl_Window'd classes.
 
* '''Fl_WindowedChat''': Encapsulates the GUI for a Chat. In an observer of a Chat. Implemented using fltk-1.1.9
 
* '''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
 
* '''Fl_WindowedUserManager''': Encapsulates the GUI for a UserManager. Is an observer of a UserManager. Implemented using fltk-1.1.9
 +
 +
 +
'''Concurrency''':
 +
* '''Thread''': The thread class wraps the POSIX threads library. It is inherited by Inbox so that so that it can run concurrently.
 +
* '''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 [[You ain't gonna need it|YAGNI]] but makes the Mutex class more reusable for further concurrent designs.
  
 
==Initial Design Review==
 
==Initial Design Review==
 +
This section is a discussion of the good, bad and other points about the design. As I was reviewing the design I was making changes to it as I went so the reviewed design shown in Figure 2 is also a helpful reference.
  
===Bad Things to be Corrected===
+
===Good Things===
There are several areas of the design that are bad and need to be reviewed.
+
The use of the Observer and Abstract Factory patterns completely removes the implementation of the GUI from the Client implementation. The GUI objects are observers of the client model objects and the concrete factory is responsible for building the GUI objects  and linking them to their subjects.
  
====The Inbox Contains a Map of Chat objects====
+
====Observer Pattern====
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.  
+
This pattern is used to allow the GUI windows to observe the Client's state without coupling.
  
====UserManager Handles Local User Login Status====
+
Participants:
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. 
+
*Subject - Subject.  
 +
*Concrete Subject - Chat and UserManager.
 +
*Observer - Observer
 +
*Concrete Observer - Fl_WindowedChat and Fl_WindowedUserManager
  
====ClientSocket Weirdly Inherits Concrete Socket Base Class====
+
====Abstract Factory Pattern====
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.  
+
This implementation of an abstract factory doesn't follow the exact [[Gang of four|GoF]] architecture, however the intent is still to decouple the interface for making a Chat or UserManager from its implementation. There are no abstract products, just concrete Chat and concrete UserManager. However, the Factory is also responsible for constructing the GUI object linked to the concrete products using the Observer Pattern.
  
====Inbox Switch Smell for Executing Packets====
+
Participants:
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.
+
*Abstract Factory - Factory
 +
*Concrete Factory - Fl_Factory
 +
*Abstract Product - None
 +
*Concrete Product - Chat (+ Fl_WindowedChat), UserManager (+ Fl_WindowedUserManger)
  
[[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:
+
====Command Query Separation====
* The switch smell would still exist where the Packets were constructed, in ClientSocket and Outbox.
+
The whole design follows the [[Command query separation|command query separation]] principle.  All operations either take no parameters or return void. This helps ensure that all behavior and state are in the appropriate place. Most of the operations are commands so the design also follows the [[Tell, Don't Ask|tell, don't ask]] maxim. The only query operations are used by the GUI to request the state of the model.
 +
 
 +
====Acyclic dependancies====
 +
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
 +
 
 +
The [[Acyclic dependencies principle|acyclic dependencies principle]] concerns packages not classes, but it is still good that there are no cyclic relationships between classes. It also helps with the concurrent analysis of the safety and liveliness of the program.
 +
 
 +
====MVC Pattern====
 +
The MVC pattern is used in this design. It is not the same structure as the Smalltalk "Observer-Strategy" version of MVC. The Observer pattern is used to decouple the Model from the View, but the Fltk GUI Library merges the View and Controller roles in the pattern.
 +
 
 +
There is a strange aspect to the pattern involving the Inbox, it acts as a Controller that responds to input packets from the server but it has no knowledge of the View. It is not an Observer of the Socket because it polls the Socket for Packets, this is a blocking operation and is the only responsibility of the Inbox.
 +
 
 +
*'''Model''':UserManager, Chat, Outbox, Socket
 +
*'''View/Controller''':Fl_WindowedUserManager, Fl_WindowedChat
 +
*'''Controller''':Inbox
 +
 
 +
===Bad Things===
 +
 
 +
====Single Responsibility Broken====
 +
*'''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/operations and keeping track of the online users. This also breaks the [[Single responsibility principle|single responsibility principle]]. The UserManager is doing two jobs. This class can be split into a UserList and a LocalUser class. The UserList only maintains a record of online users, the LocalUser handles the Login state and behavior of the local user. 
 +
 
 +
Changes:
 +
*New '''ChatManager'''
 +
*UserManager becomes '''UserList''' and '''LocalUser'''
 +
 
 +
 
 +
====God-like Manager Class====
 +
Solving the above Single responsibility issues leads to a ChatManager class. This class is responsible for constructing/destructing Chats and mapping Chats to ChatID's. In order to follow the [[Tell, Don't Ask|tell, don't ask]] maxim the interface of the ChatManager needs to support some of the same operations as a Chat object, it merely passes the operation call to the appropriate Chat object. This leads to a God-like manager class that contains a collection of Chat objects. While this seems bad, and there are alternative solutions, I think it is the best I have thought of so far. Other solutions are:
 +
* Use the chain of responsibility pattern (Thanks Jimmy for this suggestion). Make Chat objects into a linked list so that a operation can be called on the first Chat, if the ChatID doesn't match that Chat then the operation call is passed on down the chain. This is a elegant way of removing the need of a ChatManager and would be a good solution if the program wasn't concurrent. The problem arises when one thread (the Inbox) is moving down the list calling operations and the other thread (the GUI) jumps in and removes a Chat causing a segfault. This could be solved with a global ChatList mutex but thats nasty and we may as well use the ChatManager to protect the list of Chats.
 +
* Remove a lot of the functionality from the ChatManager so it is just a ChatRecord. The ChatRecord would return a reference to a Chat object associated given a ChatID. This protects the Chat list but also breaks the [[Tell, Don't Ask|tell, don't ask]] principle.
 +
 
 +
Changes:
 +
*'''None'''
 +
 
 +
====Bad Inheritance Structure for ClientSocket====
 +
ClientSocket weirdly inherits from the concrete base class Socket. The Socket class was shared across the client and server programs so 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.
 +
 
 +
Changes:
 +
*'''Socket''' becomes abstract
 +
*'''ClientSocket''' gets implementation that was previously in '''Socket'''
 +
 
 +
====Switch Smell for Executing Packets====
 +
There will always be some sort of switch smell related to the packets. The packets are received from the Socket prepended with a command type enumeration. This enumeration dictates how the data in the packet is to be handled.
 +
 
 +
The current solution builds a generic packet (including a type field) in ClientSocket which is then executed by the Inbox. The Inbox uses a switch statement based on the command type of Packet received. The Packet data structure is also used by the Outbox which builds Packets with the appropriate command type and sends them to the server via Socket.
 +
 
 +
An alternative solution is [[Polymorphism]], make Packets execute themselves. Use an abstract base class with concrete sub classes for each Packet type. However, there are several things to consider:
 +
* The switch smell would still exist where the Packets were constructed, in the ClientSocket.
 
* Packets would need added behavior to execute or serialise themselves. This introduces dependancies on the UserManager, Chats etc, and dependency on the serialisation strategy.
 
* 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.
+
* Some packet types are only received, or only sent. If we have one base class there will be some Packet types where Serialise() or Execute() do nothing.
 +
 
 +
My chosen solution is to leave the switch statement smell in the Inbox. I believe I can get a way with this because the Inbox is an alright place for the switch, and the switch is only in one place. The Inbox has the most knowledge of the model and how it should behave so it should define how packets are executed. Also, moving the switch to the ClientSocket would require a Packet inheritance hierarchy and makes the software complex.
 +
 
 +
Changes:
 +
*'''None'''
 +
 
 +
 
 +
====Switch Smell for Logged in State====
 +
The UserManager maintains a state variable for the logged in state of the local user. This data will move into the new LocalUser class. The switch smell occurs in the Fl_WindowedUserManager which changes its appearance based on the logged in state (Logged out, logging in, logged in) by showing/hiding button widgets.  The LocalUser doesn't change behavior as it changes state, it is just used differently by the View/Controller.
 +
 
 +
The original MVC pattern used the strategy pattern for changing controller behavior. This could be used in the Fl_WindowedUserManager but it adds a lot of complexity to the class. The interface of the Fl_WindowedUserManager would need to reveal its widgets so the strategy can modify them and a strategy hierarchy needs to be implemented. This extra complexity is for the sake of extensibility of the GUI, but the GUI can already be extended by defining new View/Controller classes.
 +
 
 +
So I'm going to leave the switch smell in the Fl_WindowedUserManager because it is only in one place, and it doesn't really limit the extensibility of the GUI because another GUI can easily be implemented.
 +
 
 +
Changes:
 +
*'''None'''
 +
 
 +
 
 +
====Outbox contains a UserID====
 +
The Outbox currently maintains a UserID field for the currently logged in user. It uses this information to tag outgoing packets with the UserID. Having this information in Outbox means that the UserManager is responsible for ensuring the UserID is up to date all the time. It also breaks the idea of keeping data and behavior in the same place. To solve this problem the UserID is being removed from Outbox. The packets will still need the sender's id so this is incorporated into the Outbox interface which makes the classes using Outbox responsible for providing the UserID. This responsibility is pushed up to the View/Controllers which have access to the up to date UserID in the LocalUser class.
 +
 
 +
Changes:
 +
*Interface of '''Outbox''' requires user id of sender, this pushes the responsibility of providing a sender user id up to the Controller classes.
 +
 
  
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
+
===Other Things===
  
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:
+
====Common Element of Model Classes====
* Defining the interface of the strategy operation so the data is passed as arguments to the serialisation process, OR
+
The classes that make up the model part of the MVC pattern in this design have common elements. The LocalUser, UserList, ChatManager and Chats all have a Mutex, use the Outbox and, inherit from Subject. These common elements could be pulled into a separate class but there is no common behavior which is easily transferred. The classes all use the outbox and lock/unlock their Mutex, but in different ways. If the common superclass was used it would be a lazy class with no behavior. This would also break the [[Keep related data and behavior in one place]] Maxim because the outbox and Mutex would be in the superclass, but only used by the base classes.
* 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.
+
  
 +
====Multiple Inheritance, Burning Base Class, Inheritance for Implementation====
 +
This design is written in C++ so its supports multiple inheritance. The Thread, Subject and Observer classes are utility classes that are inherited by various classes in this design. The thread class encapsulates the infrastructure required to run a class as a separate thread using the pthreads library, the Observer and Subject classes implement the GoF Observer pattern. Using inheritance in this way breaks a few principles and maxims. Firstly, the idea that I'm [[Don't burn your base class|burning my base classes]] doesn't apply because C++ allows multiple inheritance. I am also [[Avoid inheritance for implementation|Inheriting for Implementation]], but in a way that is common in C++. The threading behavior and implementation is completely encapsulated in the Thread class, the Inbox is inheriting the Thread class to gain this functionality but keep it separate from what the intent of Inbox actually is. A similar thing is achieved with the Subject class, the classes that make up the model part of the design inherit Subject so that they can be observed by the View/Controller's in the design.
  
 +
Implementing the Observer pattern this way means that I need to use multiple inheritance to use the Fltk library. Riel has a heuristic, [[Avoid multiple inheritance]], but he doesn't think multiple inheritance is evil, just not often used correctly. I think this is an appropriate use of multiple inheritance. It lets the Windowed classes inherit the Fl_DoubleWindow class so that they can draw and handle events, it also lets them be Observers so they can observe the model classes. These two inherited classes don't conflict in what they are trying to achieve. It also doesn't introduce any inheritance ambiguity into the design because the base classes are unrelated.
  
===Stuff that needs fixing, maybe?===
+
====Broken Abstract Factory Pattern====
* Outbox shouldn't record who is logged in.
+
As the changes were made to the design the Factory class moved further away from the Abstract Factory Pattern and started to incorporate elements of the Builder pattern. The ChatManager, UserList and LocalUser classes are all required to make a Fl_WindowedUser. This means that the Factory needs to build and keep track of one of each of the classes required for a Fl_WindowedUser before it can return any one of them. The operation names have been changed to reflect this change. This reduces the ability of the Factory to only being able to build a single Fl_WindowedUser and its associated UserList, ChatManager and LocalUser. The Factory can still build multiple Chats. The intent is still to decouple the GUI implementation from the model implementation and this is still achieved.
* Names of classes are a bit meh.
+
* State switch in Fl_WindowedUserManager, switch statement smell -> State pattern...
+
  
* [[Don't burn your base class|Burning base class]] with observer, subject and thread?
+
===Summary of Changes===
* [[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
+
* New '''LocalUser''' class
 +
* New '''ChatManager''' class
 +
* Rename UserManager to '''UserList'''
 +
* Make '''Socket''' abstract
 +
* Move Socket implementation to '''ClientSocket'''
 +
* Move UserID out of '''Outbox''', update the interface.
 +
* Update '''Factory''' to handle new classes
  
Good Stuff:
+
==Reviewed Design==
* 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.
+
  
 +
[[image:JohnHofmanStudyClient2.1.gif|frame|centre|'''Figure 2: The reviewed working design for the Instant Messenger Client''']]
  
==TODO==
+
Note: The Mutex class has been omitted from this diagram. It is unchanged and a member object of the UserList, LocalUser, ChatManager and Chat classes. The Packet Structure is also omitted. It is also unchanged and used in the same way.
* Make ChatManager
+
* Rename getters and setters
+
* Rearrange socket and client socket
+
  
==Other Stuff==
+
==Implemented Reviewed Design==
[[John Hofman US001| Simple chat]]
+
[[Media:JohnHofmanDesignStudySrc.zip |Here]] is the source code for my design. It will compile and run on Linux or Mac OS X but requires the fltk-1.1.9 library.
[[John Hofman's Log|Log]]
+

Latest revision as of 00:51, 1 October 2010

Contents

My Project - Instant Messenger Client

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 server-client system uses a simple login model, a user attempts to log in with just a username. The server accepts a login attempt if the username is not already taken. When a user logs out their username is freed for anyone else to use. The client and server communicate using a basic protocol. Commands are serailised packets containing a command type and any required data (username, message, chat ID's etc).

Functional requirements of the client:

  • Connect to a server.
  • Login.
  • Logout.
  • Display the other users online - receive updates from the server.
  • Start a conversation with another online user - started by the local user, or an online user.
  • Post messages in a conversation.
  • Receive messages in a conversation - from other users via the server.
  • 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 first design I developed and it is fully functional. I started out trying to do a really good OO design but found it difficult to get started. So I developed this design for submission for my other paper which is focussed on concurrent design. I'm now going to review the OO aspect of this design and try to improve it.

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.

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. 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.


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.
  • Observer/Subject: These classes implement the GoF observer design pattern. C++ allows multiple inheritance so the Observer and Subject base classes can be inherited without burning my base class of Chat, UserManager or the Fl_Window'd classes.
  • 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


Concurrency:

  • Thread: The thread class wraps the POSIX threads library. It is inherited by Inbox so that so that it can run concurrently.
  • 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 further concurrent designs.

Initial Design Review

This section is a discussion of the good, bad and other points about the design. As I was reviewing the design I was making changes to it as I went so the reviewed design shown in Figure 2 is also a helpful reference.

Good Things

The use of the Observer and Abstract Factory patterns completely removes the implementation of the GUI from the Client implementation. The GUI objects are observers of the client model objects and the concrete factory is responsible for building the GUI objects and linking them to their subjects.

Observer Pattern

This pattern is used to allow the GUI windows to observe the Client's state without coupling.

Participants:

  • Subject - Subject.
  • Concrete Subject - Chat and UserManager.
  • Observer - Observer
  • Concrete Observer - Fl_WindowedChat and Fl_WindowedUserManager

Abstract Factory Pattern

This implementation of an abstract factory doesn't follow the exact GoF architecture, however the intent is still to decouple the interface for making a Chat or UserManager from its implementation. There are no abstract products, just concrete Chat and concrete UserManager. However, the Factory is also responsible for constructing the GUI object linked to the concrete products using the Observer Pattern.

Participants:

  • Abstract Factory - Factory
  • Concrete Factory - Fl_Factory
  • Abstract Product - None
  • Concrete Product - Chat (+ Fl_WindowedChat), UserManager (+ Fl_WindowedUserManger)

Command Query Separation

The whole design follows the command query separation principle. All operations either take no parameters or return void. This helps ensure that all behavior and state are in the appropriate place. Most of the operations are commands so the design also follows the tell, don't ask maxim. The only query operations are used by the GUI to request the state of the model.

Acyclic dependancies

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

The acyclic dependencies principle concerns packages not classes, but it is still good that there are no cyclic relationships between classes. It also helps with the concurrent analysis of the safety and liveliness of the program.

MVC Pattern

The MVC pattern is used in this design. It is not the same structure as the Smalltalk "Observer-Strategy" version of MVC. The Observer pattern is used to decouple the Model from the View, but the Fltk GUI Library merges the View and Controller roles in the pattern.

There is a strange aspect to the pattern involving the Inbox, it acts as a Controller that responds to input packets from the server but it has no knowledge of the View. It is not an Observer of the Socket because it polls the Socket for Packets, this is a blocking operation and is the only responsibility of the Inbox.

  • Model:UserManager, Chat, Outbox, Socket
  • View/Controller:Fl_WindowedUserManager, Fl_WindowedChat
  • Controller:Inbox

Bad Things

Single Responsibility Broken

  • 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/operations and keeping track of the online users. This also breaks the single responsibility principle. The UserManager is doing two jobs. This class can be split into a UserList and a LocalUser class. The UserList only maintains a record of online users, the LocalUser handles the Login state and behavior of the local user.

Changes:

  • New ChatManager
  • UserManager becomes UserList and LocalUser


God-like Manager Class

Solving the above Single responsibility issues leads to a ChatManager class. This class is responsible for constructing/destructing Chats and mapping Chats to ChatID's. In order to follow the tell, don't ask maxim the interface of the ChatManager needs to support some of the same operations as a Chat object, it merely passes the operation call to the appropriate Chat object. This leads to a God-like manager class that contains a collection of Chat objects. While this seems bad, and there are alternative solutions, I think it is the best I have thought of so far. Other solutions are:

  • Use the chain of responsibility pattern (Thanks Jimmy for this suggestion). Make Chat objects into a linked list so that a operation can be called on the first Chat, if the ChatID doesn't match that Chat then the operation call is passed on down the chain. This is a elegant way of removing the need of a ChatManager and would be a good solution if the program wasn't concurrent. The problem arises when one thread (the Inbox) is moving down the list calling operations and the other thread (the GUI) jumps in and removes a Chat causing a segfault. This could be solved with a global ChatList mutex but thats nasty and we may as well use the ChatManager to protect the list of Chats.
  • Remove a lot of the functionality from the ChatManager so it is just a ChatRecord. The ChatRecord would return a reference to a Chat object associated given a ChatID. This protects the Chat list but also breaks the tell, don't ask principle.

Changes:

  • None

Bad Inheritance Structure for ClientSocket

ClientSocket weirdly inherits from the concrete base class Socket. The Socket class was shared across the client and server programs so 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.

Changes:

  • Socket becomes abstract
  • ClientSocket gets implementation that was previously in Socket

Switch Smell for Executing Packets

There will always be some sort of switch smell related to the packets. The packets are received from the Socket prepended with a command type enumeration. This enumeration dictates how the data in the packet is to be handled.

The current solution builds a generic packet (including a type field) in ClientSocket which is then executed by the Inbox. The Inbox uses a switch statement based on the command type of Packet received. The Packet data structure is also used by the Outbox which builds Packets with the appropriate command type and sends them to the server via Socket.

An alternative solution is Polymorphism, make Packets execute themselves. Use an abstract base class with concrete sub classes for each Packet type. However, there are several things to consider:

  • The switch smell would still exist where the Packets were constructed, in the ClientSocket.
  • 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. If we have one base class there will be some Packet types where Serialise() or Execute() do nothing.

My chosen solution is to leave the switch statement smell in the Inbox. I believe I can get a way with this because the Inbox is an alright place for the switch, and the switch is only in one place. The Inbox has the most knowledge of the model and how it should behave so it should define how packets are executed. Also, moving the switch to the ClientSocket would require a Packet inheritance hierarchy and makes the software complex.

Changes:

  • None


Switch Smell for Logged in State

The UserManager maintains a state variable for the logged in state of the local user. This data will move into the new LocalUser class. The switch smell occurs in the Fl_WindowedUserManager which changes its appearance based on the logged in state (Logged out, logging in, logged in) by showing/hiding button widgets. The LocalUser doesn't change behavior as it changes state, it is just used differently by the View/Controller.

The original MVC pattern used the strategy pattern for changing controller behavior. This could be used in the Fl_WindowedUserManager but it adds a lot of complexity to the class. The interface of the Fl_WindowedUserManager would need to reveal its widgets so the strategy can modify them and a strategy hierarchy needs to be implemented. This extra complexity is for the sake of extensibility of the GUI, but the GUI can already be extended by defining new View/Controller classes.

So I'm going to leave the switch smell in the Fl_WindowedUserManager because it is only in one place, and it doesn't really limit the extensibility of the GUI because another GUI can easily be implemented.

Changes:

  • None


Outbox contains a UserID

The Outbox currently maintains a UserID field for the currently logged in user. It uses this information to tag outgoing packets with the UserID. Having this information in Outbox means that the UserManager is responsible for ensuring the UserID is up to date all the time. It also breaks the idea of keeping data and behavior in the same place. To solve this problem the UserID is being removed from Outbox. The packets will still need the sender's id so this is incorporated into the Outbox interface which makes the classes using Outbox responsible for providing the UserID. This responsibility is pushed up to the View/Controllers which have access to the up to date UserID in the LocalUser class.

Changes:

  • Interface of Outbox requires user id of sender, this pushes the responsibility of providing a sender user id up to the Controller classes.


Other Things

Common Element of Model Classes

The classes that make up the model part of the MVC pattern in this design have common elements. The LocalUser, UserList, ChatManager and Chats all have a Mutex, use the Outbox and, inherit from Subject. These common elements could be pulled into a separate class but there is no common behavior which is easily transferred. The classes all use the outbox and lock/unlock their Mutex, but in different ways. If the common superclass was used it would be a lazy class with no behavior. This would also break the Keep related data and behavior in one place Maxim because the outbox and Mutex would be in the superclass, but only used by the base classes.

Multiple Inheritance, Burning Base Class, Inheritance for Implementation

This design is written in C++ so its supports multiple inheritance. The Thread, Subject and Observer classes are utility classes that are inherited by various classes in this design. The thread class encapsulates the infrastructure required to run a class as a separate thread using the pthreads library, the Observer and Subject classes implement the GoF Observer pattern. Using inheritance in this way breaks a few principles and maxims. Firstly, the idea that I'm burning my base classes doesn't apply because C++ allows multiple inheritance. I am also Inheriting for Implementation, but in a way that is common in C++. The threading behavior and implementation is completely encapsulated in the Thread class, the Inbox is inheriting the Thread class to gain this functionality but keep it separate from what the intent of Inbox actually is. A similar thing is achieved with the Subject class, the classes that make up the model part of the design inherit Subject so that they can be observed by the View/Controller's in the design.

Implementing the Observer pattern this way means that I need to use multiple inheritance to use the Fltk library. Riel has a heuristic, Avoid multiple inheritance, but he doesn't think multiple inheritance is evil, just not often used correctly. I think this is an appropriate use of multiple inheritance. It lets the Windowed classes inherit the Fl_DoubleWindow class so that they can draw and handle events, it also lets them be Observers so they can observe the model classes. These two inherited classes don't conflict in what they are trying to achieve. It also doesn't introduce any inheritance ambiguity into the design because the base classes are unrelated.

Broken Abstract Factory Pattern

As the changes were made to the design the Factory class moved further away from the Abstract Factory Pattern and started to incorporate elements of the Builder pattern. The ChatManager, UserList and LocalUser classes are all required to make a Fl_WindowedUser. This means that the Factory needs to build and keep track of one of each of the classes required for a Fl_WindowedUser before it can return any one of them. The operation names have been changed to reflect this change. This reduces the ability of the Factory to only being able to build a single Fl_WindowedUser and its associated UserList, ChatManager and LocalUser. The Factory can still build multiple Chats. The intent is still to decouple the GUI implementation from the model implementation and this is still achieved.

Summary of Changes

  • New LocalUser class
  • New ChatManager class
  • Rename UserManager to UserList
  • Make Socket abstract
  • Move Socket implementation to ClientSocket
  • Move UserID out of Outbox, update the interface.
  • Update Factory to handle new classes

Reviewed Design

Figure 2: The reviewed working design for the Instant Messenger Client

Note: The Mutex class has been omitted from this diagram. It is unchanged and a member object of the UserList, LocalUser, ChatManager and Chat classes. The Packet Structure is also omitted. It is also unchanged and used in the same way.

Implemented Reviewed Design

Here is the source code for my design. It will compile and run on Linux or Mac OS X but requires the fltk-1.1.9 library.

Personal tools