Code Review 1 for the Model

Model Code Review

 

OVERALL

The “heart” of this simulation project is the Model and, thankfully, due to its modeling of real-world objects and hierarchies, it may be the simplest code to review. A good bit of it is simply the structure and abstraction levels, so as long as that’s done well those sections will have little to comment on and little to go wrong. With such a clean object model, though, it’s important to make sure that the actual “doers” (behaviors/attributes) are well-constructed, consistent, and extensible.

 

FLEXIBILITY

• Use of behaviors in a composite-like system allows very simple base objects to perform complex operations depending on context and available resources around them.
• Attributes are a very nice option, but certain things are inconsistent – like being able to compare to objects. Some attributes can be compared, others cannot. While it may not make a lot of sense in the relational- or real-world model, it might be best to at least make all of these Attributes comparable in a very basic way. (Theo mentioned Java’s Comparable/Comparator system at one point.)

 

EXTENSIBILITY

• The high level of abstraction, while resulting in a large number of abstract base classes and interfaces, leaves room for further inheritance if sections of code need extending in the future; the modeling of different types of objects (spaces, entities, etc.) closely models the real world and enables outside or new developers to quickly grasp the model hierarchy.
• The existence of multiple types of the same object from various milestones (e.g. Milestone3Mall class) is confusing – which one should be used? Look into finding a better way to separate old code from new and keep the class names simple and non-specific to a particular milestone or set of milestones.

 

CORRECTNESS

• While lacking a comprehensive “published spec” (aside from inline code documentation and comments), the Model subset of code appears to be “correct” in its operation – quite a bit of the Model is the object hierarchy itself and various getters/setters to support that hierarchy. Behaviors and Attributes appear to be the most complex portion of the Model and this is where “incorrect” code is most likely to occur – both in the behaviors themselves and the code that handles such functionality. The remainder of the model (spaces, entities, etc.) mostly contains groups of other objects and has basic properties – a nice simple system with little to go wrong.

 

ROBUSTNESS

• The use of null-like Singletons in multiple places helps ensure that the system behaves properly and remains stable throughout its operation.
• The getters/setters of the mainly hierarchical objects by their very design have little chance of failing since they perform mostly simple operations and have the benefit of automatic type-checking.

 

SPECIFICS

• Factories:
• IPersonBuilder
  The IPersonBuilder class seems like an interesting idea to create all the attributes. However, I’m not sure I see a clear justification for using something like this at this point. Is it perhaps overkill that could be replaced by passing a list of attributes to the person factory? This might be an example of trying to do too much OO. Do we need an entire class to set the gender of the person?
  
  
• AItemFactory vs. IITemFactory:
  I remember discussing this with model group a while back, but what exactly was the justification for having an abstract AND interface class to represent the same thing. There looks like there is a lot of code duplication in there, so there is most likely a better way to do this.
  
  
• APersonFactory:
  In the same vein of thought as the above paragraph, APersonFactory does not have a corresponding IPersonFactory class like the item and space factory classes do. Whether its needed or not, it should at least be consistent.
  
  
• ItemFactories:
  How do the basic properties (like name, age, etc) work with the IPersonBuilder classes? Will these attributes like name and age eventually be put in as attributes and not have their hard coded getter/setter methods? I assume this is something sort of left over from early development and it should probably be cleaned up.
  
  
• MS3 Stuff:
  There seems to be a reoccurring trend throughout the model code that there are MS3 files (and MS2 and MS1) and you will see comments like “this makes a MS2 space”. These should really be taken out now that we are working toward “production” code, and we should rely on CVS and backups to keep old versions of files around. Every file in the model folder should be designed to work on the most recent milestone, or moved somewhere else.
  
  
• MS3PersonFactory vs. PersonFactory
  The new “MS3” version appears to be a structural improvement over the previous incarnation.
  
  
• Space Factory Issues:
  We need to clean up how spaces determine their boundary of points. This is currently hard coded, and its something the model and UI groups need to work together on. On another note, currently, properties of a space are hard coded in and there doesn’t seem to be an option to easily add more (like people have attributes). Its something worth considering, whether we want to abstract out the ability to add new properties (attributes) to a space like we can a person.
  
  
• Run:
  Are we happy with the system we have setup to run each cycle with the run method? This started off I think as the model group’s idea for milestone 1, and we may need to check if this is ok for the long term. There may be some issues down the road, possibly something for the AR group to research.
  
  
• Attributes:
• AttributeResult - Clear, basic abstract class to facilitate the comparisons of attributes.
  
  
• Attributes – IAttribute or AAttribute? Are they the same thing (or just a naming error in the comments)? Since it’s an abstract class, I guess it would be AAttribute. The comment for the getter for _name states that the name is unique to the attribute and may possibly be made equal to GetType().FullName. If this is a better way to go, what’s holding you back? (Seems to be a good choice…) The comparison between another attribute and the current one is done pretty cleanly. Everything’s fairly straightforward.
  
  
• BoxAttribute, PositionAttribute, PriceAttribute, StockAttribute – Again, straightforward and clear. At first glance it seems a tiny bit excessive, but then the beauty of OO shines through…
  
  
• BoxAttributes:
• EthnicityBoxAttribute, IconTypeAttribute, NameAttribute, UniqueIDAttribute – Ok. Not much code-wise, but it seems like a solid framework.
  
  
• TimeInMallBoxAttribute – Clean. Out of curiosity, what do we do with the information about shopper’s duration besides kill them off after a while? Is there any other use for this?
  
  
• AttributeComparators:
• DefaultComparator – Seems clear.
  
  
• IAttComparator – Right now, there’s only one concrete comparator, DefaultComparator and I guess the motivation behind having an interface class for comparators is that you envision implementing all types of other comparators. Will they be needed? Good job of thinking ahead and making this extensible into the unforeseen future.

 

• Behaviors:
• BuyBehavior – The Execute method *should* probably use the Visitor pattern, but as it is now, it’s still pretty straightforward.
  
  
• HelloWorldBehavior – Neat-o, but I’m not sure what it’s really used for.
  
  
• RandomInteractBehavior – I assume that MS3RandomInteractBehavior is what is being used (or will be used very soon). It seems as if the main difference between this class and the MS3RandomInteractBehavior is that the ‘execute’ method is a little smarter (in that it uses a MS3MoveBehavior to move towards another entity if it isn’t close enough to begin with). This seems like a plausible choice, though I’m curious what motivated the design change.
  
  
• MS3MoveBehavior – Why was a max step size of 5 chosen? Perhaps this “magic number” could be made into a global variable (that can be easily changed for testing).
  
  
• MS3GetOlderBehavior – Ah, the euthanasia method. Why do people have to die when they get old – why can’t they just leave the room?
  
  
• MS3DecideBehavior – Straight forward again.

 

• Entities: (Very simple object structure and little to implement, hence few notes.)
• AEntity._room
  This private variable should be initialized in the default- and specialized constructors or as a part of its member declaration itself.
  
  
• AEntity._initRoomName
  First, this member seems poorly named; it appears to be used to hold the name of the space in which the entity currently resides, but the member name suggests it has something to do with initializing the room, or an initial name for the entity when it’s in a room, etc. Second, and more importantly, it’s not needed. There is already a reference to the current space/room (see _room member) so getting the current space name should simply involve a method call on that space itself. Having a separate variable in the Entity is just an extra point of failure that could eventually get out-of-sync; again, just reference the name from the existing ‘_room’ member.
  
  
• AEntity.calculateKey()
  This method’s purpose appears to be to generate a “unique string key” but it does not appear to be very strong; it uses only the room name and some integer number. This isn’t incorrect, per se, but if true uniqueness is to be guaranteed, perhaps something stronger like a hash code should be involved somewhere? (And I don’t mean hash the current string result, I mean include a hash IN the string result.)
  
  
• NullEntity._behaviors
  This member of the NullEntity singleton is initialized to: new Stack(); is this necessary? I couldn’t find where this stack was actually used since all the NullEntity methods return null or perform no operation. The definition of the member might be required by the abstract base class, but you can initialize it to Null and save a miniscule amount of memory and a LOT of confusion for further developers.
  
  
• Item.Name Property (as well as a few other objects’ Name properties)
  This property returns the result from the object’s ToString() method; the ToString() method does all the work of calculating the item’s name. This is poor design; theoretically, someone who extends Item could override or shadow the ToString method and you lose all name functionality. Define the name logic in the Name property itself and if you want the ToString() method to return the Item’s name, have the ToString() method call the Name property instead of the other way around. In short, put the functionality where it’s expected.
  
  
• Milestone3Mall – consider future support for multiple “entrances.”

Code Review 1 by Justin Nixon

 

Overall, I thought that the Model Group’s code was very well organized and very flexible.  It should be very easy to extend this code.  There are lots of Interfaces and Abstract classes so it takes a while to get to the concrete implementations.  At some points I had to look through a lot of OO fluff to find one line of “meat” code.  This isn’t a bad thing since we are aiming for extensibility, but we need to make use of the structure that is already implemented.  Most of the code is fairly robust although I would like to guarantee that some of the casts work in certain areas.  It is nice that the model group has paid close attention to serialization issues since these issues could cause such big problems with remoting.  As far as I can tell the code works correctly and when it does not there is a documented reason usually with a tag that says something needs to be implemented.

I really like how Properties are used whenever possible.  Most variables and classes are named very descriptively which added to the code readability.  There are great comments throughout the code, but there are a few areas that are still uncommented especially in Entities.  There are some areas where regions are used very well and add to the readability of the code, while it is unused in other areas.  I think regions are great and should be used wherever possible.  The code is in general very nicely formatted and uniform with a good use of space.

 

All of the little issues I had with the code are listed below:

 

Behaviors

• I noticed that there is lots of casting and assuming that the casts work.  Maybe this is a valid assumption, but it would be nice to check for errors here if bad objects are passed. 
• It seems like there is a lot of CompositeBehavior code floating around in interfaces and abstract classes that only works in CompositeBehavior.  Maybe all behaviors should extend CompositeBehavior in some way so each Behavior gets this additional functionality.
• RandomInteractBehavior removes the host object from the its interact list rhough a search when it should be using the remove(object) method.
• In decide behavior do you really need to check if the caller is in the same room as the host? Isn’t this taken care of with timing and ticks?
• Checking for a null stock attribute in BuyBehavior doesn’t really make sense. When would it ever be null?
• It’s a little confusing how behaviors work(ie buy behavior allows an item to be bought, the move behavior allows people to move toward the object with the behavior).
• Treating of NullBehavior seems weird sometimes.  For instance in MoveBehavior a null position attribute catch sets the position to 0,0.  Is this what we want?  Should it be caught earlier?
• What is num in ValueResult?  It could be named better and I am not sure what it does or how it is different than pref.

 

 

Attributes

• Some of the documentation in Attributes refers to IAttributes which do not seem to exist.
• I don’t understand some of the attribute comparison code.  Attributes are only compared with like attributes but should we have some kind of cross attribute comparison?  Items might have any of the same attributes.  It also loos like there is a lot of structure surrounding the attribute comparison when the code really only does one small calculation for the result.
• BoxAttribute is a good idea but is possibly poorly named.
• EthnicityBoxAttribute constructor is unimplemented because there is no way to  compare ethnicity?

 

Entities

• ReceiveFirstObject in Milestone3Mall hard codes an ISpaceAdapter as the first object sent.  This may be unavoidable with our system, but we should check for other types in case something else is sent.
• Catching a general Exception in Milestone3Mall with no specifics does not provide information on the type of exception.  This may be the AR groups fault since their code is not very robust yet but it is something that needs to be looked into.
• I was confused between Person and IPerson.  Milestone3 person extends both
• I hope threading in spaces works, I’m not familiar with C# threading and it’s undocumented in the code.

 

Factories

• Very nice clean code with lots of space and regions!