PaulDesignStudy

From CSSEMediaWiki

(Difference between revisions)

Latest revision as of 01:13, 5 October 2009

This layout was copied from Janina's design.

For my research project for Cosc366 I developed a simple GUI that would allow a user to construct MPML3D scripts that control Non Player Characters (NPCs) in 3D Virtual World. I was under pressure to complete the project so I basically threw design out the window and started hacking away. By chance I did implement some parts that were not too smelly but would be unable to tell what pattern they followed.
I will first decide on the requirements for my program and what I would like the program to do. I will give a brief explanation of MPML3D. Then, present and critique my current version to help me to highlight areas I feel require refactoring.

Requirements

The GUI needs to be intuitive and easy to use.
The GUI must produce valid lines of MPML3D script.
The GUI must incorporate all the options under MPML3D.

Constraints

I cannot change the MPML3D language syntax and semantics or change its code.

MPML3D

MPML3D is a XML-based scripting language to describe the behaviour of computer controlled agents. Although it is not bound to a single platform by design, the current version only supports avatars inhabiting Second Life (SecondLife).It can be extended to OpenSimulator as well (OpenSim). With MPML3D, it is easy to construct agents that can give appealing presentation, including speech, gestures and movement. For a more in-depth explanation of the language goto Manual.

Initial Design

Original UML

Classes

MPML3DGUI.Classes
NPCCreator –
Class that has the main runs the whole application. Basically creates an instance of MainGui and displays it.

Entity –
An Entity is a representation of a Second Life avatar. It has all the information required to log on to Second Life.

MPML3D.Classes.gui
MainGui –
This creates the parent GUI that the other types of GUI get created and displayed within. This class runs the whole show and creates the HeaderGui and BodyGui objects. When the script is ready to be written this class creates an MPML3DWriter and passes it the HeaderGui and BodyGui to be written to a script.

HeaderGui –
This GUI is used by the user to create the header of the script. It holds a list of Entities which are used to write to the script and a list of strings that have are used to write out the External perceptions created. This class also creates an instance of MPML3DHeader.

BodyGui –
The user uses this GUI to create the body of the script. This class creates an instance of an MPML3DBody.

MPML3D.Classes.Writer
MPML3DHeader –
This class has all the data required to write the header of a valid MPML3D script. It has a list of entities and a list of external perceptions.
MPML3DBody –
This class has all the data required to write the body of a valid MPML3D script. It has a list of activities that can also have a list of activities nested with in it.
MPML3DWriter –
This class gets the MPML3DHeader and MPML3DBody classes from the MainGui and writes all the data from the header and body to a file.

MPML3D.Classes.activities
These classes inherit from the AbstractActivity class. Each activity contains the data to create the required lines of valid MPML3D code. Each activity performs a unique action on the avatar.
AbstractActivity
ActionActivity
ParallelActivity
PerceptionActivity
SequentialActivity
SelectedActivity
TaskActivity
For more information of what each activity does please refer to the manual Manual.

Walkthrough

When first running the program the MainGui interface is presented. From here the user can create a new script or edit an existing script. Once an option is selected a HeaderGui window is opened and the user can create entities and external perceptions that they require. Once finished the user clicks the “Create body” button. This calls a method within the MainGui that opens a BodyGui. The user can then enter the activities that control the entities here. When finished the user clicks “Write script” where a file browser opens and the user can navigate to the required folder, as well as naming the script.

Design Critique

I feel there are a lot of things that can be improved with in this design.

Specific design maxims that are violated by the initial design:

No-op overrides/Design by contract/Liskov Principle: Some of the subclasses of AbstractActivity donot require some of the super classes methods and have been overwritten with a stub that says this method not required.

Coupling: The Gui and backend are strongly coupled making it hard to create a new Gui for the system.

Data class smell / Eliminate irrelevant classes:A few of the subclasses of AbstractActivity do not require any additional functionality.

Avoid downcasting / Beware type switches: I sometimes need to check what activity I have and cast them to a type. This is ugly.

Aims for my first design

Create a MVC pattern
Use a composite design pattern for AbstractActivity and subclasses
or try and remove the no-op methods

AbstractActivity and subclasses.

The parallel and selected activity classes have no additional functionality to the super class. The ActionActivity cannot contain any other activities so the add remove activity method from the super are no ops. My first guess is to use a Composite design pattern but I feel as though I need to move some of the classes around. On closer inspection the PerceptionActivity should not be here as I misread the information on what a perception is so I will move this class to a new hierarchy with two other classes.

By following a similar pattern for the AbstarctActivity hierarchy I run into the problem of what to do with TaskActivity as it can add Activites to its list but cannot be part of any other activities list.

At Paul's request... I came up with an adjustment to the design. --Matthew Harward 01:48, 6 August 2009 (UTC)

A specialised inhertance hierachy using a Decorator.

Using an interface to avoid no-ops.. Does this work?

Yet another option --Michal Connole 02:41, 6 August 2009 (UTC)

Thanks to Michal, Mathew and class for there input here is the final design that I choose I thought I was close to a solution with my AbstractElement solution but by adding the interface wrapable it helped to fix the problem with task activity with it being able to hold other activities but could not actually be wrapped.

Model-View-Controller

The model consists of the MPML3DHeader and MPML3DBody. The view consists of a all encompassing starting GUI that can has a HeaderGui and a BodyGui. For now I will not concentrate on where the heck the MPML3DWriter should be as I am as of yet undecided.

Will a clear cut between model and viewer i now need to create a controller and decide how to design it. Using it more as a mediator pattern I have decided to break it up in two separate controllers as they control two separate things the header and the body of the script.

Followed design principles

Avoid downcasting : This was accomplished using the way Wal does it

Single responsibility principle / One responsibility rule : This is the main reasoning for having a MPML3D header, body and writer. Each one of these classes takes care of the 3 distinct areas of a MPML3D script.

Keep related data and behavior in one place: Also for the above reasonings.

Avoid no-op overrides : By using a variation of the composite design pattern and using a wrapable interface the problem of no ops has been addressed.

MVC : Adding the MVC pattern allows the backend and gui to be totally uncoupled making it easier to create a new front end gui.

Open closed principle : By using inheritance for the activitys and elements it is easy to add new members to these hierarchys.

Design patterns used

Composite : This pattern was used twice with a slight alteration in than all leaf values are placed outside the composite part. It was used for activities and elements

MVC : This pattern was used to totally decouple front and back ends

Design conflicts

Eliminate irrelevant classes - I have created a couple of subclasses selected and parallel that do nothing more than what the super class does. Although this goes against this hieristic I feel for better understanding and separating the activities apart I have decided to keep this classes in the design.

Summary

I feel the new design has a more pleasing or less smelly feel to it. With the use of the MVC the whole system just looks a lot less coupled. As well as the modified composite design patterns adding extensibility as well as freeing from no ops.

Final Layout

Media:ScriptWriterMPML3D.zip

@@ Line 1: / Line 1: @@
 This layout was copied from [[Janina%27s_Design_Study|Janina's design]].
-As part of my honours project, I am working on a program to analyse the encapsulation in software. This program uses Wal's JST (at least will use JST once it works :)) to extract information from Java code.
+For my research project for Cosc366 I developed a simple GUI that would allow a user to construct MPML3D scripts that control Non Player Characters (NPCs) in 3D Virtual World.
+I was under pressure to complete the project so I basically threw design out the window and started hacking away. By chance I did implement some parts that were not too smelly but would be unable to tell what pattern they followed.<br>
+I will first decide on the requirements for my program and what I would like the program to do. I will give a brief explanation of MPML3D. Then, present and critique my current version to help me to highlight areas I feel require refactoring.
-I originally didn't put much effort into my design so it has about 4 classes and is very ugly in its current state. Part of the reason why I decided to use this for my design study is that I know that the code isn't too pretty at the moment and I want to improve it. I also chose it because I think the code could become unmanageable if I worked with it a lot more. At least because the current design is so ugly there should be lots I can improve on.
+==Requirements==
+* The GUI needs to be intuitive and easy to use.<br>
+* The GUI must produce valid lines of MPML3D script.<br>
+* The GUI must incorporate all the options under MPML3D.<br>
-I will first describe the requirements for my program to get my head around exactly what I need my program to do. I will then give a short introduction to JST for those readers who are not familiar with what it is and how it works. Then, I will present and critique my current design and show all the design heuristics I have broken before making an attempt to improve the design.
+==Constraints==
+* I cannot change the MPML3D language syntax and semantics or change its code.<br>
-==Requirements==
+==MPML3D==
-The program needs to be able to:
+MPML3D is a XML-based scripting language to describe the behaviour of computer controlled agents. Although it is not bound to a single platform by design, the current version only supports avatars inhabiting Second Life ([http://www.secondlife.com SecondLife]).It can be extended to OpenSimulator as well ([http://opensimulator.org OpenSim]). With MPML3D, it is easy to construct agents that can give appealing presentation, including speech, gestures and movement. For a more in-depth explanation of the language goto [http://research.nii.ac.jp/~prendinger/MPML3D/MPML3D.html#MPML3D_Reference_Manual Manual].
-==Constraints==
+==Initial Design==
-*I cannot modify the code for JST including the code for the semantic model and the parsetree code.
-==JST==
+[[image:uml1.png|center|frame|Original UML]]
+===Classes===
+MPML3DGUI.Classes<br>
+NPCCreator –<br>
+	Class that has the main runs the whole application. Basically creates an instance of MainGui and displays it.<br>
-Java Symbol Table (JST) is a semantic model for Java. It constructs a model of a Java program in memory, capturing various semantic
+Entity –<br>
-concepts. This includes concepts such as packages, classes, methods, constructors, parameters, ﬁelds and local variables. The relationships between these entities are also represented by the model.
+	An Entity is a representation of a Second Life avatar. It has all the information required to log on to Second Life.<br>
-JST is a much richer model than other existing Java semantic models like Javasrc. These models often only include simple relationships between entities such as method invocation and commonly struggle to resolve polymorphic and inherited method calls, leading to an inaccurate model.
+MPML3D.Classes.gui<br>
+MainGui –<br>
+	This creates the parent GUI that the other types of GUI get created and displayed within. This class runs the whole show and creates the HeaderGui and BodyGui objects. When the script is ready to be written this class creates an MPML3DWriter and passes it the HeaderGui and BodyGui to be written to a script.<br>
-JST currently accepts valid source code written in any Java version up to Java 1.6.
+HeaderGui –<br>
+	This GUI is used by the user to create the header of the script. It holds a list of Entities which are used to write to the script and a list of strings that have are used to write out the External perceptions created. This class also creates an instance of MPML3DHeader.<br>
-Despite the size and complexity of JST, information can be extracted from JST quite easily by ‘walking’ the semantic model. This can be done using a Visitor design pattern.
+BodyGui –<br>
+	The user uses this GUI to create the body of the script. This class creates an instance of an MPML3DBody.<br>
-JST reads Java programs in from XML parsetree files which can be obtained by parsing java files using a Java parser. By walking the parsetrees, JST builds up the model in memory. These parsetrees can be used for code generation. They can be modified and written back out to a java file, resulting in a modified java program.
+MPML3D.Classes.Writer<br>
+MPML3DHeader –<br>
+	This class has all the data required to write the header of a valid MPML3D script. It has a list of entities and a list of external perceptions.<br>
+MPML3DBody –<br>
+	This class has all the data required to write the body of a valid MPML3D script. It has a list of activities that can also have a list of activities nested with in it.<br>
+MPML3DWriter –<br>
+	This class gets the MPML3DHeader and MPML3DBody classes from the MainGui and writes all the data from the header and body to a file.<br>
-==Initial Design==
+MPML3D.Classes.activities<br>
+These classes inherit from the AbstractActivity class. Each activity contains the data to create the required lines of valid MPML3D code. Each activity performs a unique action on the avatar.<br>
+AbstractActivity<br>
+ActionActivity<br>
+ParallelActivity<br>
+PerceptionActivity<br>
+SequentialActivity<br>
+SelectedActivity<br>
+TaskActivity<br>
+For more information of what each activity does please refer to the manual [http://research.nii.ac.jp/~prendinger/MPML3D/MPML3D.html#MPML3D_Reference_Manual Manual].<br>
-UML diagram of initial design to come.
-===Classes===
+===Walkthrough===
-* Main: This is the starting point for the program. The Main class contains the main() method which reads in the XML parsetree files from the location it is given as a parameter. It then creates an EncapsulationAnalysisVisitor which walks the model of the program to report back on the encapsulation used. It then creates an AccessTighteningVisitor which goes through the model and tightens the access of methods and fields as far as possible and generates new java code from the resulting parsetrees.
-* EncapsulationAnalysisVisitor: The EncapsulationAnalysisVisitor visits relevant parts of the Java program's model and records information about the encapsulation used. In particular, it visits classes, operations (methods and constructors), fields And blocks of code. It contains a large number of fields to record various information and various private utility methods.
+When first running the program the MainGui interface is presented. From here the user can create a new script or edit an existing script. Once an option is selected a HeaderGui window is opened and the user can create entities and external perceptions that they require. Once finished the user clicks the “Create body” button. This calls a method within the MainGui that opens a BodyGui. The user can then enter the activities that control the entities here. When finished the user clicks “Write script” where a file browser opens and the user can navigate to the required folder, as well as naming the script.
-*AccessTighteningVisitor: This class first creates a FieldSeparationVisitor to ensure that field declarations like ''public int i, j;'' are separated into ''public int i; public int j;''. It then walks through the model to record all operations, fields and where they are accessed from. Once it has finished visiting the model, it then decides which access modifiers can be tightened and modifies the parsetree before writing it back out to java files. It contains a significant amount of code to modify the parsetrees.
+==Design Critique==
+I feel there are a lot of things that can be improved with in this design.
-*FieldSeparationVisitor: This class visits each field in turn and checks if it is declared at the same time as another field as in ''public int i, j;''. If this is the case, it modifies the parsetrees to separate the field declaration into parts: ''public int i; public int j;''. This requires a significant amount of code to modify the parsetrees and some of that code is similar to the code in AccessTighteningVisitor.
+Specific design maxims that are violated by the initial design:
+*[[Avoid_no-op_overrides | No-op overrides]]/[[Design_by_contract|Design by contract]]/[[Liskov_substitution_principle|Liskov Principle]]: Some of the subclasses of AbstractActivity donot require some of the super classes methods and have been overwritten with a stub that says this method not required.
-===Collaborations===
+*[[Coupling_and_cohesion|Coupling]]: The Gui and backend are strongly coupled making it hard to create a new Gui for the system.
-*Main creates an instance of EncapsulationAnalysisVisitor and starts the visiting process by passing the visitor to the default package of the java program model. The default package contains all other program entities. When the visiting process has finished, Main asks the EncapsulationAnalysisVisitor to print its results.
+*[[Data_class_smell|Data class smell]] / [[Eliminate_irrelevant_classes|Eliminate irrelevant classes]]:A few of the subclasses of AbstractActivity do not require any additional functionality.
-*Main creates an instance of AccessTighteningVisitor and starts the visiting process by passing the visitor to the default package of the java program model. When the visiting process is over, Main tells the visitor to resolve the accesses it found and change the parsetrees to tighten access where possible (by calling the resolveAccesses() method).It then asks the visitor to write the new parsetrees out to java files, giving it the directory to write the files into.
-*When the AccessTighteningVisitor first starts its visiting process, it makes an instance of FieldSeparationVisitor and starts the visiting process for that visitor by passing it the default package of the java program model. When the visiting process finishes, all field declarations will have been separated as described above.
-UML Sequence Diagram of the last two collaborations to come soon.
+*[[Avoid downcasting]] / [[Beware type switches]]: I sometimes need to check what activity I have and cast them to a type. This is ugly.
-==Design Critique==
+==Aims for my first design==
-There are a number of problems with the initial design of my program. The code is quite complex, with long methods and large classes which hints at the fact that I should really refactor and break the visitors up into several classes.
-Specific design maxims that are violated by the initial design:
+* Create a MVC pattern
+* Use a composite design pattern for AbstractActivity and subclasses
+* or try and remove the no-op methods
-*[[Avoid downcasting]] / [[Beware type switches]]: In quite a few places in my program, I check if I am looking at a method or a field using the instanceof operator. I then downcast from a Decl to an OperationDecl or FieldDecl before carrying on my analysis. This smells fishy to me. The downcasting suggests that maybe I should use subclassing instead.
+==AbstractActivity and subclasses.==
+[[image:firstactivityuml.jpg|center|frame]]<br><br>
+The parallel and selected activity classes have no additional functionality to the super class. The ActionActivity cannot contain any other activities so the add remove activity method from the super are no ops. My first guess is to use a Composite design pattern but I feel as though I need to move some of the classes around. On closer inspection the PerceptionActivity should not be here as I misread the information on what a perception is so I will move this class to a new hierarchy with two other classes.<br>
+[[image:elementumql.jpg]]<br><br>
+By following a similar pattern for the AbstarctActivity hierarchy I run into the problem of what to do with TaskActivity as it can add Activites to its list but cannot be part of any other activities list.<br>
+<br>[[image:nextactivityuml.jpg|center|frame]]<br><br>
-*[[Large class smell]] / [[Split large classes]]: Some of the visitors have a lot of code in them, especially AccessTighteningVisitor and FieldSeparationVisitor which both deal with the relatively complex task of modifying the parsetrees. EncapsulationAnalysisVisitor on the other hand is also large because it tries to record a lot of different data. As such, it has a large number of instance variables and long but simple methods which record data in these instance variables.
+At Paul's request... I came up with an adjustment to the design. --[[User:Matthew Harward|Matthew Harward]] 01:48, 6 August 2009 (UTC)
-*[[Long method smell]] / [[Reduce the size of methods]]: Some of the methods in the visitors are very long. In EncapsulationAnalysisVisitor, some methods are long but quite simple, consisting of a number of conditional statements to make decisions about what data should be recorded. In AccessTighteningVisitor and FieldSeparationVisitor, some methods are long and relatively complex because they deal with the long-winded process of modifying parsetrees. This requires quite a few different steps, resulting in long methods.
+[[Image:Paulsuml.png|thumb|left|A specialised inhertance hierachy using a [[Decorator]].]][[Image:Paulsuml2.png|thumb|center|Using an interface to avoid no-ops.. Does this work?]]
-*[[Duplicate code smell]] / [[Don't repeat yourself]] / [[Once and only once]]: There are a number of utility methods for modifying parsetrees and walking the model that appear in more than one of the visitors. For some reason, I was too lazy to pull them out into a separate class so I just copied and pasted them. I know that's really bad...
+[[Image:IWrapable_version.png|thumb|left|Yet another option --[[User:Michal Connole|Michal Connole]] 02:41, 6 August 2009 (UTC)]]<br>
-*[[Single responsibility principle]] / [[One responsibility rule]] / [[One key abstraction]]: I feel like some of the visitors are so large because they try to do too much. AccessTighteningVisitor and FieldSeparationVisitor both figure out which parts of the parsetree should be modified and then go on to do the modification. It seems like they have more than one responsibility and as a result, they have become quite large.
+<br><br><br><br><br><br><br><br><br><br><br>Thanks to Michal, Mathew and class for there input here is the final design that I choose I thought I was close to a solution with my AbstractElement solution but by adding the interface wrapable it helped to fix the problem with task activity with it being able to hold other activities but could not actually be wrapped.<br><br>
+[[image:AbstractElement2.jpg]]
-*[[Distribute system intelligence]]: This is closely related to the last point about some of the visitors having too much responsibility. I don't think that I have divided system intelligence properly but instead concentrated it in the large visitor classes.
+==Model-View-Controller==
+The model consists of the MPML3DHeader and MPML3DBody. The view consists of a all encompassing starting GUI that can has a HeaderGui and a BodyGui. For now I will not concentrate on where the heck the MPML3DWriter should be as I am as of yet undecided.
-*[[Sequential coupling]]: This is an anti-pattern that says that a class should not expect its clients to call methods in a particular order. This is currently the case in my design, where Main first creates a visitor, then tells it to start visiting the model, then potentially tells it to resolve accesses and finally tells it to do something with the results. This seems to lead to pretty tight coupling between Main and the visitors and should be removed.
+Will a clear cut between model and viewer i now need to create a controller and decide how to design it. Using it more as a mediator pattern I have decided to break it up in two separate controllers as they control two separate things the header and the body of the script.
-*[[Keep related data and behavior in one place]]: In my design, the behavior for reshaping parsetrees has been separated from the parsetrees and the behavior for visiting the model has been separated from the model. This violates Riel's heuristic or keeping related data and behavior in one place. However, one of the constraints of the project is that I cannot change the JST class so that I cannot put that behavior with the data it acts on so there is little I can do about this heuristics breach.
+[[image:MVC1.png|center|frame]]<br><br>
-==Aims for my first design==
+==Followed design principles==
+*[[Avoid downcasting]] : This was accomplished using the way Wal does it
+*[[Single responsibility principle]] / [[One responsibility rule]] : This is the main reasoning for having a MPML3D header, body and writer. Each one of these classes takes care of the 3 distinct areas of a MPML3D script.
+*[[Keep related data and behavior in one place]]: Also for the above reasonings.
+*[[Avoid no-op overrides]] : By using a variation of the composite design pattern and using a wrapable interface the problem of no ops has been addressed.
+*[[MVC]] : Adding the MVC pattern allows the backend and gui to be totally uncoupled making it easier to create a new front end gui.
+*[[Open closed principle]] : By using inheritance for the activitys and elements it is easy to add new members to these hierarchys.
+==Design patterns used==
+* [[Composite]] : This pattern was used twice with a slight alteration in than all leaf values are placed outside the composite part. It was used for activities and elements
+* [[MVC]] : This pattern was used to totally decouple front and back ends
+==Design conflicts==
+* [[Eliminate irrelevant classes]] - I have created a couple of subclasses selected and parallel that do nothing more than what the super class does. Although this goes against this hieristic I feel for better understanding and separating the activities apart I have decided to keep this classes in the design.
+==Summary==
+I feel the new design has a more pleasing or less smelly feel to it. With the use of the MVC the whole system just looks a lot less coupled. As well as the modified composite design patterns adding extensibility as well as freeing from no ops.
+==Final Layout==
+[[image:finals.png|center|frame]]
-* Break up the large classes and methods so that each class has one clear responsibility and system intelligence is distributed better.
+[[Media:ScriptWriterMPML3D.zip]]
-* Get rid of all the duplicated code and put it somewhere sensible.
-* Try to use subclassing to avoid downcasts in the code. This means somehow handling fields and methods separately.
-* Remove the horrible sequential coupling from the visitors' interfaces to reduce coupling between Main and the visitors.