TobiW's Design Study
(→Design Flaws) |
(→Conflicts and Compromises) |
||
(42 intermediate revisions by one user not shown) | |||
Line 1: | Line 1: | ||
== Overview == | == Overview == | ||
− | I decided to do the design study on a program I wrote back in Germany before I came here. I wrote it to have a single interface to the many meteorological sites I use to predict and learn something about the weather (mainly for my hobby flying sailplanes). It is quite | + | I decided to do the design study on a program I wrote back in Germany before I came here. I wrote it to have a single interface to the many meteorological sites I use to predict and learn something about the weather (mainly for my hobby of flying sailplanes). It is quite usable but far from being finished. Technical details: it is written in Python using wxPython for the GUI, and has approx. 2500 lines of code (counted with [http://cloc.sourceforge.net CLOC]). |
The project is hosted on sourceforge: [http://sf.net/projects/freemet] | The project is hosted on sourceforge: [http://sf.net/projects/freemet] | ||
+ | |||
+ | The source code of the old (before COSC427) version can also be found here: [http://wiki3.cosc.canterbury.ac.nz/index.php/Image:Freemet-olddesign.tar.gz Freemet-olddesign.tar.gz] | ||
Here's the description I put up on Sourceforge: FreeMet is a meteorological program which provides weather maps, radar movies and forecasts in one GUI. New function can be added easily by writing new plugins which download, parse or process existing data available on the internet. | Here's the description I put up on Sourceforge: FreeMet is a meteorological program which provides weather maps, radar movies and forecasts in one GUI. New function can be added easily by writing new plugins which download, parse or process existing data available on the internet. | ||
+ | |||
+ | From a more technical perspective, FreeMet is a program that collects data from various sources on the internet (images, animations, text from websites or ftp servers) and displays them in one GUI. The user can manage this data by using Services which normally represent one common source of a bunch of data, e.g. one website that offers several satellite animations (visible, infrared). Another way to group data in one Service is a common property of the data, e.g. the Webcams Service displays an arbitrary amount of webcam images that can be collected from different websites. To manage all these Services, the user can create Sets that contain a selection of all available Services. This way, he can have different Sets for different locations, e.g. one for New Zealand and one for Germany. | ||
== Design Study Goals == | == Design Study Goals == | ||
Line 12: | Line 16: | ||
* learn all those design patterns and code smells by looking at my code and trying to find them. | * learn all those design patterns and code smells by looking at my code and trying to find them. | ||
− | == Design Flaws == | + | First I want to correct some major mistakes I did at my first design (like making too many attributes public). This first stage will only involve the most important classes (the main program class and the plugins). A third design will then incorporate improvements to other design flaws I made. Also, this will be the place to apply some design patterns that are not absolutely necessary but that could make the code more maintainable. |
− | *I usually write my programs around one central class which handles most of the logic. This could | + | |
+ | == Python == | ||
+ | Here, I want to list some Python-specific stuff: | ||
+ | * A list can store arbitrary values, a set automatically removes duplicates. | ||
+ | * A dictionary in Python is a Map/HashMap which maps string keys to arbitrary values. | ||
+ | * __init__() is the class's contructor and __del__() its destructor. | ||
+ | * As far as I know Python always passes arguments and return values as references to objects thus violating [[Don't expose mutable attributes]] sometimes (this can be avoided by passing/returning a copy of the attribute, e.g. | ||
+ | return self.__aList[:] | ||
+ | instead of | ||
+ | return self.__aList | ||
+ | |||
+ | == Current Design == | ||
+ | * I included parts of wxPython to illustrate dependencies. | ||
+ | * The service class implements a plugin architecture which means that the program automatically loads all Python files in the 'services' directory. | ||
+ | [[Image:CdFreemet.png]] | ||
+ | |||
+ | === Classes === | ||
+ | * ''FreeMetApp'': Parses command line parameters and initialises the main frame. | ||
+ | * ''Config'': This is simply a Python module which stores some preferences for the program. | ||
+ | * ''Frame'': This is the central point of the application. It manages events from the main menu, the tree view (which is used to selected services), as well as from the timer which triggers the services to update themselves. Therefore, view, controller, and some data are all combined in this class. | ||
+ | * ''SplashScreen'': Used by the main frame to indicate loading of a new set of services which can take up to a minute depending on the internet connection. | ||
+ | * ''Service'': Base class/interface for all services. It is not abstract, i.e. not a pure interface, since common functionality for all services is implemented here, e.g. downloading from the internet and adding an image to the program's GUI. | ||
+ | * ''Webcam'': Example of a concrete service. It has to implement at least ''__init__'', ''Download'', and ''ShowNotebook''. It uses a lot of functions that are provided by its super class. | ||
+ | |||
+ | === OO Principles and Patterns === | ||
+ | * [[State]] design pattern: Depending on the active ''Service'' downloading, parsing, and displaying information is different but the interface is always the same. | ||
+ | * [[Facade]]: ''Frame'' and ''Service'' simplify the use of the wxPython GUI API. | ||
+ | * [[Beware type switches]] and [[Avoid downcasting]]: I've learned from previous projects that controlling the logic with type switches is really bad and almost always leads to unmaintainable code hence the plugin architecture for the services. | ||
+ | * [[Don't repeat yourself]]: there is no unnecessary duplicate code. For instance, functionality that is used by at least two services has been moved up to the Service base class. | ||
+ | * [[You ain't gonna need it]]: Since this is a one-man project I never wrote a big design or interface before I started implementing functionality ([[Big design up front]]). Therefore, there is no unnecessary code. | ||
+ | |||
+ | === Design Flaws === | ||
+ | * I usually write my programs around one central class (''Frame'') which handles most of the logic. This could be a [[large class smell]] and conflicts with [[avoid god classes]] as well as [[separation of concerns]]. Solution: [[Split large classes]], [[Distribute system intelligence]] | ||
+ | * One reason why ''Frame'' is so big: Display and logic functionalities are mixed in one class (violating [[MVC]]), as well as different sorts of data and behaviour, e.g. managing plugins, managing sets, and reading/writing config files (violating [[Single responsibility principle]] and [[One key abstraction]]). | ||
+ | * The ''Service'' class is also too big because it combines display (build notebook pages) and logic (downloading and parsing). | ||
* After learning a lot about the [[Getter and setter policy]], I'm sure I'll be able to eliminate many accessor methods. | * After learning a lot about the [[Getter and setter policy]], I'm sure I'll be able to eliminate many accessor methods. | ||
+ | * Related to getter/setter: some parts apply [[Tell, Don't Ask]] very well, others don't do at all and ask for information all the time. | ||
+ | * Probably because I was too lazy I didn't make all attributes private ([[Information hiding]]). | ||
+ | * Similar to the previous point, there are too many public methods that are only used internally. | ||
+ | * The ''Service'' base class implements a lot of functionality that is used by all concrete services: [[Avoid concrete base classes]] | ||
+ | * The ''Service'' class hierarchy is not deep and almost all subclasses inherit directly from the base class: [[Favour deep hierarchies]] | ||
+ | |||
+ | === Possible Improvements === | ||
+ | * Apply [[Singleton]] pattern to ''Frame'' and ''SplashScreen'' ([http://code.activestate.com/recipes/52558/ Singleton in Python]). | ||
+ | * Split ''Frame'' and ''Service'' classes. | ||
+ | * Improve information hiding/encapsulation. | ||
+ | |||
+ | == The Service Class Problem == | ||
+ | Taking the ''Service'' class apart seems to be particularly difficult since it combines so many different things. Although it is not hard to implement ("quick and dirty") the design is quite ugly and causes a lot of problems with maintainability and software reuse (see Design Flaws above). I thought about several possibilities to improve its design and taking functionality out of the Service base class (services that use static images only don't need to have support for animations). | ||
+ | * Using the [[decorator]] patter to add functionality to a basic Service instance, | ||
+ | * using the [[strategy]] or [[state]] pattern to change functionality of parts of the Service instance, or | ||
+ | * using an [[adapter]] or a [[bridge]] to make the interface and the implementation more independent. | ||
+ | However, all those 'solutions' had some problems. The first two patterns are normally used to change objects dynamically while my services are generated only once at program startup. There are no two instances of a service class at any time. The last 'solution' is not a good approach since [[adapter]] and [[bridge]] are normally used to fix or combine interfaces. The interface is not the problem in my case, though. | ||
+ | |||
+ | ==== Taking it apart as much as possible ==== | ||
+ | While talking the design over with Wal the following extreme design for the service/plugin architecture was developed. It represents a very detailed view and is supposed to be simplified in the next step. (Client is the application, namely ''FreemetMain'' and ''FreemetFrame''.) | ||
+ | |||
+ | [[Image:CdServiceDetailed.png]] | ||
+ | |||
+ | * ''ServicePlugin'' manages the different parts of service (model and view) and exposes a common interface for all concrete services. | ||
+ | * ''ServiceGui'' contains the ''ServiceTab'' objects that display data, and connects them to the application's notebook (a GUIobject that contains tabs). | ||
+ | * ''SettingsTab'' provides functionality to create a settings page for a service. For instance, in the ''WebcamSettingsTab'' the user can add or remove webcam URLs and adjust their update intervals. | ||
+ | * ''ServiceDataProcessor'' takes a resource (''Glyph'') from the an internet web site (''WebPage'') and processes it using the ''Parser''. One example is taking the URL of a website, searching for a specific webcam image (parser ''KeywordSearch''), and download that image. It can then be used by the GUI. | ||
+ | |||
+ | |||
+ | ==== A practical solution ==== | ||
+ | A simplified and more practical solution for the services plugin architecture. It successfully separates model, view, and controller ([[MVC]]) and provides the ability to parse and display all kinds of content without having a base class that is too big. Examples of two Services using images and animations are given. They are omitted in the final UML class diagram (see 'Final Design') for clarity. | ||
+ | |||
+ | [[Image:CdService2.png]] | ||
+ | |||
+ | == Final Design == | ||
+ | After having specified the mistakes I did in my original design I came up with the following improvements: | ||
+ | # Try to apply [[Tell, Don't Ask]] whenever I add methods or functionality to a class | ||
+ | # ''Frame'' and ''Service'': Make all attributes and methods that are not accessed by other classes private | ||
+ | # Split ''Frame'' class in | ||
+ | #* a class called ''FreemetFrame'' which represents only the main window | ||
+ | #* a class that represents the central part of the main program (creating components, loading plugins): ''SetManager'', ''PluginManager'' | ||
+ | #* a class that handles the menu calls (including creating dialogs): ''FreemetMenu'' | ||
+ | #* a class that encapsulates the config management: ''ConfigManager'' | ||
+ | # Split ''Service'' class in | ||
+ | #* a class called ''ServicePlugin'' which represents the main interface to a service (because of the other Service classes it might have to act as a [[Bridge]] or [[Adapter]]) | ||
+ | #* a class ''ServiceGui'' which encapsulates the management of the ''ServiceTabs'' | ||
+ | #* a class ''ServiceDataProcessor'' which encapsulates the network and parsing part (downloading from the internet) of ''ServiceData'' | ||
+ | #* a class ''Parser'' which encapsulates different parsing [[Strategy|strategies]] | ||
+ | |||
+ | For clarity reasons the final class diagram does not include exemplary services. | ||
+ | |||
+ | [[Image:CdFreemet2.png]] | ||
+ | |||
+ | === Maxims and Design Patterns === | ||
+ | This leads to the following maxims/principles/patterns implemented (improvements or in addition to the ones in the original design): | ||
+ | * [[Separation of concerns]]: Much better separation of the parts of ''Frame'' and ''Service'' than before | ||
+ | * [[Interface segregation principle]]: Especially in the old fat ''Service'' class implementing this principle made using it much easier to use | ||
+ | * [[Intelligent children pattern]]: In the service architecture specific services know about the specific tabs and data they are using | ||
+ | * [[Strategy]]: Subclasses of ''Parser'' implement different concrete parsing algorithms such as searching for a keyword or processing a web form. This is used by ''ServiceDataProcessor'' to parse incoming ''ServiceData''. | ||
+ | * [[State]]: Depending which Service is selected by the user different content is displayed and handled (which involves different parsing routines and downloading data from different sources). | ||
+ | * [[Facade]]: ''FreemetFrame'', ''ServiceGui'' and ''ServiceTab'' simplify the use of the wxPython GUI API, e.g. when adding a new tab that displays an image. | ||
+ | |||
+ | === Conflicts and Compromises === | ||
+ | However, there are also conflicting maxims and patterns: | ||
+ | * [[Separation of concerns]]/[[MVC]]: By separating data and behavior as much as possible, related data/behavior is now in different places, e.g. updating services takes place in ''FreemetMenu'' (OnUpdateServices() and OnUpdateCurrentService()), ''FreemetMain'' (OnTimer()), and ''PluginManager'' (calling each plugin's Download() method). This conflicts with [[Keep related data and behavior in one place]]. As a result, [[Tell, don't ask]] has to be applied more often than in the old design because there is a chain of responsibility (''FreemetMenu'' tells ''FreemetMain'' that all services has to be updated, ''FreemetMain'' tells the ''PluginManager'' to update all services). The separation and the resulting conflict is acceptable since there are a lot of reasons to split up the old ''Frame'' class ([[Avoid god classes]], [[large class smell]], [[Distribute system intelligence]], [[One key abstraction]]). | ||
+ | * [[Data class smell]]: ''ServiceData'' mainly contains downloaded images, animations or other data from the internet. This is because [[Model the real world]] which was done even more excessively in the first 'Service Class Problem' design. Since it helps to keep the code and the design clean and easy to maintain it is a useful class and stays in my final design (not a [[Lazy class smell]] or [[Eliminate irrelevant classes]]). | ||
+ | * [[You ain't gonna need it]]: The new plugin architecture involves many new classes and methods particularly on the data handling side (''Parser'', ''ServiceData'', ''ServiceDataProcessor''). The first design of the new Service architecture was even bigger because of the [[Model the real world]] principle. It has been simplified and might get even simpler once the design is implemented. For now, I think the final design is a good compromise between flexibility (staying closer to the real world) and usability/maintainability. | ||
+ | |||
+ | === Code === | ||
+ | Coding the final design was not possible due to the limited amount of time. It would have involved rewriting most parts of the program either by refactoring in many iterations or by writing completely new code. I think the design is thoroughly thought through so that only minor modifications have to be made while implementing it. I hope I'll find the time to improve my application in the near future. |
Latest revision as of 23:14, 4 October 2009
Contents |
Overview
I decided to do the design study on a program I wrote back in Germany before I came here. I wrote it to have a single interface to the many meteorological sites I use to predict and learn something about the weather (mainly for my hobby of flying sailplanes). It is quite usable but far from being finished. Technical details: it is written in Python using wxPython for the GUI, and has approx. 2500 lines of code (counted with CLOC).
The project is hosted on sourceforge: [1]
The source code of the old (before COSC427) version can also be found here: Freemet-olddesign.tar.gz
Here's the description I put up on Sourceforge: FreeMet is a meteorological program which provides weather maps, radar movies and forecasts in one GUI. New function can be added easily by writing new plugins which download, parse or process existing data available on the internet.
From a more technical perspective, FreeMet is a program that collects data from various sources on the internet (images, animations, text from websites or ftp servers) and displays them in one GUI. The user can manage this data by using Services which normally represent one common source of a bunch of data, e.g. one website that offers several satellite animations (visible, infrared). Another way to group data in one Service is a common property of the data, e.g. the Webcams Service displays an arbitrary amount of webcam images that can be collected from different websites. To manage all these Services, the user can create Sets that contain a selection of all available Services. This way, he can have different Sets for different locations, e.g. one for New Zealand and one for Germany.
Design Study Goals
After having done a thorough design study I hope to
- have good documentation to make it easier for myself and others to start working on the program (again),
- discover major and minor design flaws,
- learn all those design patterns and code smells by looking at my code and trying to find them.
First I want to correct some major mistakes I did at my first design (like making too many attributes public). This first stage will only involve the most important classes (the main program class and the plugins). A third design will then incorporate improvements to other design flaws I made. Also, this will be the place to apply some design patterns that are not absolutely necessary but that could make the code more maintainable.
Python
Here, I want to list some Python-specific stuff:
- A list can store arbitrary values, a set automatically removes duplicates.
- A dictionary in Python is a Map/HashMap which maps string keys to arbitrary values.
- __init__() is the class's contructor and __del__() its destructor.
- As far as I know Python always passes arguments and return values as references to objects thus violating Don't expose mutable attributes sometimes (this can be avoided by passing/returning a copy of the attribute, e.g.
return self.__aList[:]
instead of
return self.__aList
Current Design
- I included parts of wxPython to illustrate dependencies.
- The service class implements a plugin architecture which means that the program automatically loads all Python files in the 'services' directory.
Classes
- FreeMetApp: Parses command line parameters and initialises the main frame.
- Config: This is simply a Python module which stores some preferences for the program.
- Frame: This is the central point of the application. It manages events from the main menu, the tree view (which is used to selected services), as well as from the timer which triggers the services to update themselves. Therefore, view, controller, and some data are all combined in this class.
- SplashScreen: Used by the main frame to indicate loading of a new set of services which can take up to a minute depending on the internet connection.
- Service: Base class/interface for all services. It is not abstract, i.e. not a pure interface, since common functionality for all services is implemented here, e.g. downloading from the internet and adding an image to the program's GUI.
- Webcam: Example of a concrete service. It has to implement at least __init__, Download, and ShowNotebook. It uses a lot of functions that are provided by its super class.
OO Principles and Patterns
- State design pattern: Depending on the active Service downloading, parsing, and displaying information is different but the interface is always the same.
- Facade: Frame and Service simplify the use of the wxPython GUI API.
- Beware type switches and Avoid downcasting: I've learned from previous projects that controlling the logic with type switches is really bad and almost always leads to unmaintainable code hence the plugin architecture for the services.
- Don't repeat yourself: there is no unnecessary duplicate code. For instance, functionality that is used by at least two services has been moved up to the Service base class.
- You ain't gonna need it: Since this is a one-man project I never wrote a big design or interface before I started implementing functionality (Big design up front). Therefore, there is no unnecessary code.
Design Flaws
- I usually write my programs around one central class (Frame) which handles most of the logic. This could be a large class smell and conflicts with avoid god classes as well as separation of concerns. Solution: Split large classes, Distribute system intelligence
- One reason why Frame is so big: Display and logic functionalities are mixed in one class (violating MVC), as well as different sorts of data and behaviour, e.g. managing plugins, managing sets, and reading/writing config files (violating Single responsibility principle and One key abstraction).
- The Service class is also too big because it combines display (build notebook pages) and logic (downloading and parsing).
- After learning a lot about the Getter and setter policy, I'm sure I'll be able to eliminate many accessor methods.
- Related to getter/setter: some parts apply Tell, Don't Ask very well, others don't do at all and ask for information all the time.
- Probably because I was too lazy I didn't make all attributes private (Information hiding).
- Similar to the previous point, there are too many public methods that are only used internally.
- The Service base class implements a lot of functionality that is used by all concrete services: Avoid concrete base classes
- The Service class hierarchy is not deep and almost all subclasses inherit directly from the base class: Favour deep hierarchies
Possible Improvements
- Apply Singleton pattern to Frame and SplashScreen (Singleton in Python).
- Split Frame and Service classes.
- Improve information hiding/encapsulation.
The Service Class Problem
Taking the Service class apart seems to be particularly difficult since it combines so many different things. Although it is not hard to implement ("quick and dirty") the design is quite ugly and causes a lot of problems with maintainability and software reuse (see Design Flaws above). I thought about several possibilities to improve its design and taking functionality out of the Service base class (services that use static images only don't need to have support for animations).
- Using the decorator patter to add functionality to a basic Service instance,
- using the strategy or state pattern to change functionality of parts of the Service instance, or
- using an adapter or a bridge to make the interface and the implementation more independent.
However, all those 'solutions' had some problems. The first two patterns are normally used to change objects dynamically while my services are generated only once at program startup. There are no two instances of a service class at any time. The last 'solution' is not a good approach since adapter and bridge are normally used to fix or combine interfaces. The interface is not the problem in my case, though.
Taking it apart as much as possible
While talking the design over with Wal the following extreme design for the service/plugin architecture was developed. It represents a very detailed view and is supposed to be simplified in the next step. (Client is the application, namely FreemetMain and FreemetFrame.)
- ServicePlugin manages the different parts of service (model and view) and exposes a common interface for all concrete services.
- ServiceGui contains the ServiceTab objects that display data, and connects them to the application's notebook (a GUIobject that contains tabs).
- SettingsTab provides functionality to create a settings page for a service. For instance, in the WebcamSettingsTab the user can add or remove webcam URLs and adjust their update intervals.
- ServiceDataProcessor takes a resource (Glyph) from the an internet web site (WebPage) and processes it using the Parser. One example is taking the URL of a website, searching for a specific webcam image (parser KeywordSearch), and download that image. It can then be used by the GUI.
A practical solution
A simplified and more practical solution for the services plugin architecture. It successfully separates model, view, and controller (MVC) and provides the ability to parse and display all kinds of content without having a base class that is too big. Examples of two Services using images and animations are given. They are omitted in the final UML class diagram (see 'Final Design') for clarity.
Final Design
After having specified the mistakes I did in my original design I came up with the following improvements:
- Try to apply Tell, Don't Ask whenever I add methods or functionality to a class
- Frame and Service: Make all attributes and methods that are not accessed by other classes private
- Split Frame class in
- a class called FreemetFrame which represents only the main window
- a class that represents the central part of the main program (creating components, loading plugins): SetManager, PluginManager
- a class that handles the menu calls (including creating dialogs): FreemetMenu
- a class that encapsulates the config management: ConfigManager
- Split Service class in
- a class called ServicePlugin which represents the main interface to a service (because of the other Service classes it might have to act as a Bridge or Adapter)
- a class ServiceGui which encapsulates the management of the ServiceTabs
- a class ServiceDataProcessor which encapsulates the network and parsing part (downloading from the internet) of ServiceData
- a class Parser which encapsulates different parsing strategies
For clarity reasons the final class diagram does not include exemplary services.
Maxims and Design Patterns
This leads to the following maxims/principles/patterns implemented (improvements or in addition to the ones in the original design):
- Separation of concerns: Much better separation of the parts of Frame and Service than before
- Interface segregation principle: Especially in the old fat Service class implementing this principle made using it much easier to use
- Intelligent children pattern: In the service architecture specific services know about the specific tabs and data they are using
- Strategy: Subclasses of Parser implement different concrete parsing algorithms such as searching for a keyword or processing a web form. This is used by ServiceDataProcessor to parse incoming ServiceData.
- State: Depending which Service is selected by the user different content is displayed and handled (which involves different parsing routines and downloading data from different sources).
- Facade: FreemetFrame, ServiceGui and ServiceTab simplify the use of the wxPython GUI API, e.g. when adding a new tab that displays an image.
Conflicts and Compromises
However, there are also conflicting maxims and patterns:
- Separation of concerns/MVC: By separating data and behavior as much as possible, related data/behavior is now in different places, e.g. updating services takes place in FreemetMenu (OnUpdateServices() and OnUpdateCurrentService()), FreemetMain (OnTimer()), and PluginManager (calling each plugin's Download() method). This conflicts with Keep related data and behavior in one place. As a result, Tell, don't ask has to be applied more often than in the old design because there is a chain of responsibility (FreemetMenu tells FreemetMain that all services has to be updated, FreemetMain tells the PluginManager to update all services). The separation and the resulting conflict is acceptable since there are a lot of reasons to split up the old Frame class (Avoid god classes, large class smell, Distribute system intelligence, One key abstraction).
- Data class smell: ServiceData mainly contains downloaded images, animations or other data from the internet. This is because Model the real world which was done even more excessively in the first 'Service Class Problem' design. Since it helps to keep the code and the design clean and easy to maintain it is a useful class and stays in my final design (not a Lazy class smell or Eliminate irrelevant classes).
- You ain't gonna need it: The new plugin architecture involves many new classes and methods particularly on the data handling side (Parser, ServiceData, ServiceDataProcessor). The first design of the new Service architecture was even bigger because of the Model the real world principle. It has been simplified and might get even simpler once the design is implemented. For now, I think the final design is a good compromise between flexibility (staying closer to the real world) and usability/maintainability.
Code
Coding the final design was not possible due to the limited amount of time. It would have involved rewriting most parts of the program either by refactoring in many iterations or by writing completely new code. I think the design is thoroughly thought through so that only minor modifications have to be made while implementing it. I hope I'll find the time to improve my application in the near future.