GUI Code Review

By: Bryan Lipinski

 

 

GUI.Client.Wizard

 

Demographics – Use??  The Client doesn’t need demographic information.  The Server, which is the only entity which can create people, should be the only ones to need access to demographics.  Even in that case the demographics should be directly plugged into the PersonFactory, which can be done fairly easily (We just haven’t gotten around to it).  I can’t even find any references to this class.

 

Products – This should not be needed.  The wizard should configure the factory directly without having to go through this intermediary object.  More in Step 3.

 

Step1 – Good use of properties.  It might be a good idea to have a method called configureSpaceFactory().  This method would take in a factory and then configure it, and hand back the configured factory with its fields set.  This would be even better then worrying about the properties, and keep a nice distance between the information in this panel and what the entire wizard knows.

 

Step2 – The button1 isn’t on Step2.  While it is setup and given event handler’s it doesn’t appear on the Wizard.  Furthermore I suggest a better name then button1 and method names better then button1_Click as there are multiple usage’s of these names/methods throughout the UI code, and it would be less confusing if they were more specifically named things such as loadExistingLayoutButton or such.  Once again a configureSpaceFactory() method could be utilized here for the same reason.

 

Step3 – I think products are not the right way to go.  While they seem like a nice way to give initial values they also just seem like a waste.  That could be their only use, and that doesn’t seem to be very useful.  I think it might be better to find some other way to set initial values.  This could be better accomplished through other means such as simply hardcoding in values such as an array of ProperySpecs, and use that directly.

 I think the actual step should know how to configure the factories.  I think that going through the intermediary Products class is a waste.  I suggest the panel do everything having to do with items, so as to keep a high level of seperation.  There are two possibilities with this.  One way is to have a configureFactories() method which returns ItemFactory[] which you can go through and add the items to the space.  Another way, and probably better way, is just have the panel contain an ItemFactory and then have an addItems() method which takes in a space and sets the ItemFactory and adds the items.

      Another general note is the alphabetical sort can be confusing as to what product goes with what price.  I realize they are at the bottom, but is it possible to put them next to price etc?

      Names like button3 aren’t very useful.  Change it to something descriptive.

      bag_GetValue() and bag_SetValue() do not have any extensibility right now, for the user to add in their own items. If you didn’t use the Product then you wouldn’t need set and get.  All you’d need to do is cycle through the products to set the factory and add the items.  A simple foreach loop could accomplish that.

 

Step4 – I can’t really say anything about this one.

 

Wizard – The only constructor needed should be the Wizard(IspaceFactory, IItemFactory).  The wizard is used to design a new store, there should be no need of the IspaceAdapter in the constructor.  If this was put in to get the Release, we should make steps to change it.  Also the constructor with no parameters won’t be very extensible.  If we want to change the factories, we should just be able to change them in the controller, and then it sets them.  In having the factories initialized by the Wizard, this requires knowledge about what kind of Factories to use which the Wizard shouldn’t know.

      The wizard constructs a store upon closing.  This should be done only when the finished button is pushed, and furthermore only if all fields are filled in.  I suggest methods in each panel which return a boolean, true if enough fields are filed in to produce a store. 

      Furthermore, it instantiates the IspaceFactory right there, instead of getting the spaceFactory from the constructor.  This may be due to lack of setting up how the Wizards and such are initialized in the controller.  Once again it comes to the fact that the panels should know how to do their own thing.  You shouldn’t have to request information and put it in the store, the panels themselves should do that.

      The Wizard should not need to use Model.Entities, that is going against the entire MVC pattern.  It should have access to factories and attributes, but not Entities.  I can’t even find where this is used.  Not needed?

 

GUI.Client

 

Chat – Mostly auto-generated code.  Only thing I don’t like is the lack of good variable naming.  richTextBox1 and richTextBox2 aren’t the most descriptive names.

 

InitForm – Mostly auto-generated code with good variable names.  Could be commented slightly better, but good code.  This should be the main GUI application, it needs to be set up in the controller (Model Team’s fault).

 

Map.gif – A Lovely gif.  Nice and bright. 

 

StoreManager – Variable Names, Variable Names, Variable Names.  Button1, panel1, menuItem1 etc. are not good variable names.

I’m confused as to what happens when they want to change their store.  Does a new storeManager appear, or is there a panel associated with the store that is replaced or what?  If it’s a whole new manager or a panel then there is no point for the SpaceAdapter property.  If, however, you are just swapping out IspaceAdapters when you change then that would work. 

Again, I don’t think that there really needs to be a StoreWizard property. No one else should need access to the StoreWizard, and the StoreWizard shouldn’t need to be changed at any point.

Constructor should also set the StoreWizard.

Like the comments say setSpaceAdapter should be removed.  The StoreManager should always be handed an IspaceAdapter from the initForm either a previously created one, or a newly created one through the wizard.  Talk with model people (ie. Me and we’ll set it up nicely).

For the newStore event there are issues with WizardCreation.  Wizards once created must not be disposed, because they are needed to get access to their store field to get the ISpaceAdapter they create.  However if too many are created they could just be floating around.  They either need to be disposed of after they are finished, or there should only be one, which is cleared each time through.

For updates method there is a good use of small methods to isolate how things are done.  Very nicely done.  The only thing to change would be to only update what is needed, and that won’t be done until the model group has finished the dirty stuff.  We’ll get on it.

 

 

      Only images and previously defined files and dlls.  Nothing that the UI group could mess with.

 

 

      GUI.Manager.Wizard

 

Demographics – An empty class.  Should not be needed.  The Wizard should set the person factory directly.  Is this even being used by anything?

 

Step1 – Mostly computer generated code.  Once again I would not have the get methods to access the fields, but have the panel have a setMallFactory, which takes in the SpaceFactory which creates the Mall, and sets the fields.  This adds to the extensibility and flexibility of the Step.

 

Step2 – button1?!  Mostly unimplemented so not much to evaluate.  Once again have a method to set the factory.  Note:  Maybe the model should create an IMallFactory, or at least a MallFactory concrete class of off ISpaceFactory. 

 

Step3 – label1?  I think the “Add This Person” button is appropriate.  While the server manager should possibly be able to add people, it shouldn’t be from the beginning.  One possibility is for him to be able to add names to the list of names being considered for a randomly generated person, however the GUI should not make people ever!  I like this setup for the hardcoding of the demographics.  A lot of code is unimplemented or commented out, but there is no explanation as to what commented out code  did, or what unwritten code will do.

 

Step4 – label1.

 

Wizard – Consider renaming this one as to distinguish it from the other wizard.  Could possibly abstract something from between the two, and create an AWizard class.

There should only be the one constructor which takes in a PersonFactory and a SpaceFactory.  Along the same lines, the SpaceFactory and PersonFactory properties are useless.   The factories should never change with the wizard, and there really should only be one wizard.  New instances seem frivolous.  Either way it should be handed the appropriate factories upon creation.

The same stuff as the previous Wizard for closing.  There is no checking to make sure all the appropriate fields have been filled out.  While there are defaults, having a bunch of Malls called Your Mall etc. could get ugly.  Just add a method to each panel to make sure that the stuff they had to get done is done, or don’t allow access to the next button until all panels are completed.  Also the wizard closing shouldn’t create the object.  What happens when they press cancel?  There shouldn’t be an instantiation of spaceFactory here either.  The constructor should set the one to use.

                        The other thing is that the panels should add their own stuff to the factories and such using methods.  You should not be getting the fields from the panels and then adding them.  The panels should know what to do. 

There should be a method through the IspaceAdapter (for the mall) to add the PersonFactory.   You should not be adding people to the rooms here.  I’ll get on that soon.  I’m finding more work for myself in reviewing your code… yuck.    

I didn’t find a single bad variable name, good job.

 

      GUI.Manager

 

Chat – Variable Names.  Not implemented yet.

 

DataSet1 – Previously generated code from somewhere?  No commenting.  If you are using this, please comment at least the top part as to what you are using it for.  Looking at it, I have no idea what it is for or what it does.

 

EditWindow – Variable Names.  Offhand there needs to be a way for the owner to create points to move.  Furthermore there needs to be an ok button, and ways to define store shapes in the overall Mall Structure.  Right now they are limited to the number of points given, and furthermore with only being able to create the perimeter of the mall.

            An ok, button could be nice as well.

 

Manager – Once again variable names: panel1, tabControl2 etc.

            There is duplicity in the setSpaceAdapter() method and the SpaceAdapter field.  If you are doing the hot swap, as in the Client then you’d just need a setting capability.  There should be no need for the classes outside the Manager to access the IspaceAdapter directly.

            I suggest takes a Wizard in as a constructor as well.  That should not be changing.  There need only be one wizard, and since we aren’t going to be changing wizards during run-time the property is frivolous. 

            InitDefaultMall should be declared private.  While it is nice to setup the mall, there should not be any need for anything else to use this method.  In this method there are creations of RenderableMallSpaces.  To me this seems that it should be creations of IspaceAdapters, now that they implement IRenderable.  The model needs to come up with a better Mall object, and you’ll have to add that to it. 

            In newMallEvent the code is making a new instance of a Wizard each time.  This defeats the purpose of the early Wizard field as well as bogging down the system.  Go with one Wizard, and clear it each time after it’s called.

 

            The update tabs can be made more efficient, that probably needs to wait until the dirty bit is being used. 

            The rest is just computer-generated or un-implemented. 

 

 

      GUI.Renderer

 

Animation -  I don’t like multiple classes in files, but it doesn’t really matter. 

      The Animation class looks fairly straightforward.  It is not properly documented though.  There are good uses of loops, and the code looks good.  It would be nice to know what the unimplemented methods do, so again documentation.  I haven’t done anything with C# Hashtable class, but it’s probably safe to assume the comment is correct.

            Both of the Key classes aren’t documented.  They are really straightforward being just container classes for the most part, but I’m not sure of the difference.  The StringKey class is used in the Animation class, but I can’t find where the Key class is used.  Is it obsolete code?

            AnimationNode is too simple to have bad code.  Interfaces are good though, so kudos to the UI group for using them.

            PersonNode is the concrete AnimationNode.  Upon looking at PersonNode I wonder if AnimationNode is being underutilized.  There are a lot more public methods in personNode then animationNode.  This makes me believe that maybe the interface isn’t being used to it’s fullest.   This is further followed, by the fact that in Animation there are typecasts to personNode, when those typecasts should be to an animationNode.  Extend animation node to encompass everything you need.  The rest is graphics carp that I don’t need or want to understand.

 

RenderableMallSpace – The should be obsolete now.  The IspaceAdapter should be implementing the IRenderableSpace interface now, and be the primary implementation of the interface.  Any references directed toward a RenderableMallSpace, should be made towards IrenderableSpaces.  I don’t want to search through the code so I’ll just say it here.

 

Renderer – Once again multiple classes in one file, can be annoying.  I’m going to say right now that I don’t know jack about Graphics so bear with me.

            Interfaces are good.  Some commenting as to what Renderer will do would be nice. Looking ahead at MallRenderer, it appears that there is a lot more that could be extracted up to the interfaces.  I’m not sure what are internal to the MallRenderable, but from the number of public methods, it looks like the interfaces could cover more ground.

            For Viewport, and in general, there needs to be private classes.  There are a lot of instances where the default is used (I’m not sure what C# defaults to, but I assume it’s something like package).    Since the viewport is changing each time with the space the sets are good, but do you really need the gets, or are they there just because?

            The MallRenderer looks good.  The number of public methods worries me that it will limit the usage of the interfaces in the future.  Make sure that nothing needs a MallRenderer, just IRenderers or  SpaceRenderers.  Looks like clean code.  Private methods are good.  I like the smaller methods instead of super method.

 

StoreRenderer – No commenting.  Is editMode used currently?  Not much I can tell from the code.  Why is there a constructor with no arguments?!  That should be taken out, and it is to be kept, then there needs to be some checks to make sure the store != NULL before saying store.getColor().  I don’t think that constructor should exist.  Other then that pretty good code.  The Hack will be fixed, but that will be made when the Attribute for shape? is better implemented.

 

 

     All in all good coding from the UI team.  A few general statements.  Private methods are good.  Even though certain things are hacked make sure they have good variable names, its just one field.  Finally, documentation is good.  While some classes where documented very well, others were very lacking. This may or may not be correlated to the coders.  I’ll let you bicker amongst yourselves as to who is responsible.

     A representative and I need to meet for 2-4 hours sometime this weekend or such, and actually set up the damn controller correctly.  A lot of the methods have hacks in them were you just instantiate things when they are needed.  You really should have already had those things on you.  Again, not the UI team’s fault. 

     A lot of the classes are not needed any more and those really should be looked at.  All the “temporary” stuff and “hacks” need to be worked out, and double/tripled checked to make sure that there are no references to them.

     I wasn’t entirely sure on the interfaces.  A lot of the classes which implement the interfaces have lots of public methods.  This makes me think that you actually need to typecast to an instance of that particular class, when you should really only need an interface.  Just a thought.  That’s all.  Nicely Done.

UI Code Review

by Robby Morgan

 

          Overall I believe the UI group has done a good job of developing an intuitive, flexible UI that integrates well with the rest of the project.  During my code review, I didn’t spend much time verifying that things like rendering calculations were correct.  Instead I focused on potential limitations of the current design and some other minor design details.  Here now are the results of my code review:

 

·       IRenderableEntity has a method getType() that returns a string which “tells the renderer what kind of graphic to display”.  The comments in the code mention that eventually this string will map to some icon that should be displayed.  I would advise you to consider using a object that is inherently capable of drawing or displaying the correct icon on the screen for two reasons.  First, you would be encapsulating the current string-to-color or string-to-icon mapping, which would free you from requiring this mapping for all entities in the future.  Second, you would be able to ‘decorate’ the default icon fairly easily with different colors/outlines in the event that you wish to portray different states for an entity throughout the simulation (hot v. cold pizza, satisfied v. angry customers).  On a separate note, you need to consider how future users might need to add new items to the simulation by specifying new icons in addition to new behaviors.

·       SpaceRenderer should be an abstract class that implements IRenderer.  There are two classes, MallRenderer and StoreRenderer, that implement both of these interfaces, and both classes have the exact same code for the IRenderer.render() method, in addition to sharing some of the same fields.  These common fields should be protected fields of the abstract superclass, and the render() method should do its delegation to abstract versions of the renderLayout() and renderContents() methods in the superclass.  As it stands these classes are flagrantly violating the basic principles of object-oriented programming.

·       Viewport is designed to convert from one coordinate system to another for the purpose of displaying graphics on the screen.  I’m wondering whether the viewport class could be generalized to handle the computations necessary to allow stores and entities in stores to work with the stores’ local coordinate systems while transparently querying and manipulating objects on the global coordinate system.  This would provide the encapsulation desired for entities in a store while permitting the simultaneous viewing of entities inside and outside stores that users may desire.  Also, the Viewport class might consider supporting vertical flips that would allow objects to move in a normal xy-plane rather than having the y-axis inverted as GDI uses.

·       The Animation class looks about half-finished, and it also doesn’t appear to be used in any of the rest of the code.  For this reason, I didn’t look at it too closely, but I did find faults with the hashing-related classes and comments in the file.  First, you define the classes Key and StringKey to encapsulate ints and strings that are to be used for hashing.  With C#’s unified type system, I would advise you to simply use the built-in ints and strings for hashing, as I see no additional benefits you have gained.  Also, comments in the code suggest writing your own hashtable.  If you are unhappy with the .NET hashtable, I’m sure others have been as well, and I would advise that you look to the internet for fully-tested and working versions of C# hashtables.  This should save you time and headaches.

·       Looking at the GUI, I’m curious how you plan to update, grow or shrink the legend for the mall manager interface once you have more than four different types of stores.  It’s not so much the implementation issues I’m concerned about (although that could be difficult); I’m more worried about the real estate issues involved.  You might eventually consider popping up a separate floating dialog that the user could position near the map and reference should he/she desire.

·       Users will need the ability to load/save mall layouts between simulations.  This functionality would be useful now too for testing purposes.  I strongly encourage the UI group to get this hammered out soon, as it will be critical to the usability of our application.

·       Finally when I close the application window(s) after starting the program through VS, the process continues to run.  This is likely the result of a stray thread that did not terminate when the window was closed.  There are a number of ways to remedy this, but the easiest is adding a handler for the window close event that simply calls Application or System.exit(0).  This instructs Windows to kill the process even if there are threads still running.  It’s definitely not the most elegant or most robust way, but it provides a quick fix until the stray threads are located.