Code Review 1 for the Advance Research (Networking) Code

Up Java Resources C++ Resources .NET Resources DevHood Search

AR Group Code Review

Ryan Aipperspach

In general, I’m very impressed with the design of the networking library.  It has a lot of functionality but exposes a very simple interface to the end user.

Flexibility

  • RemotingQueue always has an event associated with an enqueue event, even it it’s a NoEnqueueEvent.  This is good design.  It makes it easy to change the default event.
  •  As of now, we don’t really have a use for the levels of naming indirection (zone/service name), since we’re only making one connection between any pair of objects (e.g., spaces).  However, the decision to put the indirection there was a good one.  I think it will be very useful when we want to enable spaces to do things like statistics reporting.  (For example, the mall could have a SendStatistics service, which would provide a connection for reporting statistics.)
  • It doesn’t look like there’s a way to clear the event handler on IConnections.  If I set the EnqueueEvent, the implementation uses the += operator, which adds an event handler to the list of existing event handlers.  So there’s no way to remove an event handler once I’ve hooked it up to the connection.  It would be a useful function to have, since I might want to change how I’m using a connection after I’ve created it.

 Extensibility

  • The channel and connection factories should work very well.  They make it easy to use different types of connections (pull v. push) and channels (tcp, http, etc.).  It would be useful to be able to specify what type of connection you want to create, instead of just defining a system default.  This would require an option in NetHub.createService that defined a type of connection to use when connecting to that service (or else define it in the constructor to NetHub, and require a given net hub to use one type of connection).
  • It would be nice to have both ConnectionQueue and RemotingQueue inherit from a common base class, since they share some functionality.  To make it work well, however, would require parameterized types.  So see what happens if there’s ever  a C# v 2.0.

 Correctness

  • The code doesn’t actually work yet, but we knew that coming in – so the correctness assessment will look at issues in specific functions that may come up when the code is all in place.  (Will promises that it works now, too!)
  • When the user creates a HubLink, the system attempts to create a NetHub if one isn’t already running.  If this fails for any reason, all of the exceptions raised are re-thrown as a NoHubConnectionException.  This will make it much easier for the model to deal with the networking library, since the model will only create HubLinks, and thus only have to deal with one type of exception.
  • It looks like all of the function calls are correct – E.g., while we can’t test how the code works, a general trace through the code makes sense and seems well designed.  Once all of the calls to the .NET framework are correct, I think the library will work just fine.

 Robustness

  • It’s possible to create two different HubLinks on one machine with the same zone name.  While this is fine as long as there are no duplicate service names on the two zones, you might want to consider enforcing a one-to-one HubLink to zone name mapping.  Otherwise, just make sure that the necessity of keeping unique names is clearly documented.
  • You do, however, check to make sure that there aren’t two services with the same name on one HubLink, among other things.  In general, the name checking is good.
  • If there’s an error in NetHub.setupIncomingService, the call returns null instead of throwing an exception.  This makes catching a problem more subtle, since you have to remember to check for a null return value.  I’d suggest having the system throw an exception if there’s a problem on service creation instead.
  • The RemoteEventHandler class should never be extended by a user (Will says that if you do, the net hub complain – something about the external .dll not being able to find the correct code).  So it should probably be made final.
  • Unit Tests:  Full unit testing would be difficult, since you have multiple processes running on multiple systems.  But it would be nice if there was something implemented – a lot of the pieces could be tested atomically, like queues &c.

 Other

  • NetHub has some protected fields.  Since nothing inherits from the class, should they be private instead?
  • The message box pop-ups should eventually be removed from the code.  They’re good for debugging, but when we release, we don’t want them up.  Possibly put in a flag that determines whether or not to show them?
  • There are lots of classes in the package that shouldn’t be seen from the outside.  Look at using the internal access modifier on the classes (like Java’s package private).

 

Code Review

Travis McPhail  

NetHub

NetHubForm  

I really don’t think that this form is needed.  A taskbar icon would be sufficient.  Otherwise it does exactly what it is supposed to do, create a NetHub.

NetworkHubLib  

ConnectionQueue

The connection queue is solid code ( meaning straightforward. )    

HubLink

There is solid error checking in the HubLink constructor.  Looking at how the code is structured, the AR group might want to throw an exception in the initChannel method if the staticChannel is not null.  Once again the code is straightforward ( which is good. )  You might want to add a string in the NoHubConnectionException with “tcp://localhost:”+p_hubPort.ToString()+”/Rice/Comp410/Remoting/NetworkHub”  ( this is just for quick debugging purposes. )  

IConnection

This is a solid interface.  There really isn’t anymore abstraction that can be added onto this interface.  A note on future implementations of this class will be given at the end.  

NetHub

I like the whole concept of having one Hub per machine and using this Hub as the central connection to the “outside.”  In the setupIncomingService method, shouldn’t some type of setupServiceException be thrown if the zoneNames already contains the service?  

ProcessManager

Good job on the coding for this class.  I’m curious whether it would be a good idea to regulate the starting of processes here ( should there be a maximum number of processes allowed to run concurrently with this particular manager?  If too many processes are running should the manager wait for the other processes to end before starting more?  It just seems like we can customize this to suit our purposes a bit more. )  

RemotingEvents

I might be totally off base here but I don’t see a need for having both the SubmissionCallback and the InternalSubmissionCallback ( outside of some security issues – that I don’t think we need to worry about. )  The two classes within the file are straightforward ( this is starting to sound like a broken record. )  

RemotingManager

I am not too familiar with all of the code in this file.  It might have been beneficial for me to look at some general remoting material before tackling this one.  The ServiceSpec and ServerSpec classes are straightforward.  The RemotingManager itself is not so straightforward ( this is probably due to my lack of knowledge concerning remoting. )  Does a channel automatically start listening after it is registered?  The RemotingQueueManager and the ClientManager classes are straightforward.  It might have been a descent idea to just make ServerManager extend ClientManager since the only difference is the receiveClientInfo method in ServerManager.  

RemotingQueue

For the most part this code is straightforward.  I just have one gripe – I think the setEnqueueEvent should set the NewEnqueue event not append on to it.  I think a separate method should be added to append events onto the NewEnqueue event.  

ServiceManager

The code is also straightforward.  Should there be any closing events in the Close method?  

Flexibility:

There doesn’t appear to be a way to clear the event handlers in IConnections  

I am pleased with the overall feel of every class.  For non-void methods, a meaningful objects is returned as the value, except when a service is already contained in a set of zonenames in the setupIncomingService method.

Extensibiliity:

The interfaces in the code seem to well planned and provide a nice framework to work with.

Correctness:

Aside from the Process Manager class, I either am not aware that the code in the NetworkingHub is working or I know that it flat out is not working.  But this was not a surprise.  The code seems to be similar to the old RemotingLib code, so it has promise.  

Robustness:

As far as this goes the code is fairly robust.  The concerns I have are already mentioned in the comments on the individual files above.

Overall:  

All in all the NetHub and NetworkingHubLib code is solid ( and for the twentieth time straightforward. )  I’m a little uneasy with the Remoting Manager, but that’s only because I do not know all of the details with remoting.

When an actual class implements the IConnection interface there might need to be a visitor of different event handlers that handle the objects that come through the connection.  ( i.e.  have a “forDoorCase”, “forItemCase”, “forSpaceCase”, etc. )  

But kudos to the AR Group.

Code Review

James McDougall

Overall Comments:  

            The design of this system is excellent.  It has good flexibility and extensibility.  It is abstracted to an appropriate level for a system designed to a single known task in an environment that does not easily allow abstraction.  I am including in this document specific comments on the correctness, extensibility, flexibility, and robustness, as well as notes not specific to a category.  The comments cover good features and practices, as well as bad.  My goal was to understand the process and analyze the structure of the design.

  • Correctness
    • As of the code freeze, it doesn’t work.  This is a problem, but I knew this was the case.  What was important was the architecture of the system.
    • In RemotingQueue the use of the NoEnqueueEvent as default was an excellent design decision.  Very OO.
    • The initialization of HubLink will crash if errors were created when initializing the hub or connection process (setting up queues, etc).  Very good exception handling is used in HubLink to handle this situation.
    • The zone:system implementation specifies that a zone + system combination must be unique, however this is the only stipulation.  You can therefore create multiple zones with the same name.  Though this is not necessarily what the designers intended, it may make the system easier to use.
    • The fact that you cannot close anything (HubLinks, ServiceManagers, etc) is incorrect.  Stub code exists for this, but needs to be implemented.
  • Extensibility
    • The simple implementation of making ServiceSpec a class instead of just a string somewhere allows for a lot of extensibility in the future if needed.
    • Building the end-connection Object as an IConnection (extendable, swappable interface) allows for easy addition of functionality down the road.
  • Robustness
    • For the most part, non-existent.
    • Biggest flaw is the wide range of ways errors are handled.  Sometimes exceptions are thrown.  Sometimes null is returned (NetHub.SetupIncommingService).  Sometimes the thrown exceptions are caught and nothing bad happens, as is the case with RemotingManager.registerChannel.  The fact that there is an error in the channel will probably be caught somewhere down the line in the initialization process, but the proper place to handle this problem is in the method where the exception is thrown.  There are several specific things that should be put in place for easy/robust integration…
      • Well-defined exceptions (not UnknownNetworkException) for when an object is pushed through a connection, but a problem occurs.   An excellent example of this would be if a client machine were to crash and the server tried to push an object through.  Other specific exceptions should be implemented wherever possible.
      • Be very specific with thrown exceptions in the initialization of the Hub as well as connections between Hubs on different machines.  If errors will happen in the remoting this is the first place they will occur and tracking errors to the initialization will be extremely useful in debugging.
  • Flexibility
    • Abstracting ChannelFactories as they are was a very nice design decision.  It allows for easy changes in the future.  The same applies to the types of IConnections used.
    • The code exists for the system to use either a push or pull usage of the queues within the IConnections, but a way to decide which one to use has not been implemented.  Being able to make this decision could be very useful.
  • General Comments
    • In many locations MessageBoxes are poped-up to signal a problem.  These need to be removed before actual implementation.
    • The tops of classes need to be commented with a high-level description of what that class does.

I am very happy with the design of this system.  Perhaps its largest advantage is that the user us shielded from practically ALL .Net remoting issues. They are hidden behind a few easy to use interfaces.  Very little low-level setup is required of the user.  One would almost say it is somewhat “plug-and-play.”

In this review I did not discuss or analyze the low-level code.  I do not know how the .Net remoting library works, nor was it necessary for me to analyze these specific details.  My concern was how their design affected its use in the larger system.  It seems to me that the NetworkingHubLib will work exactly as discussed before its implementation and that it will fit very nicely into the model/view system.

Jeff Segars

General Comments

Overall, I felt like the NetHubLib was very well done.  The code was well documented and seemed as if it would be easily extensible.  My understanding of remoting is very limited and this was my first exposure to any AR code, so my viewpoint is very surface level but what I see looks good.  The high-level documentation was especially helpful and without it I would have been completely lost. 

There is one minor issue that I see when looking at the NetHubLib from the UI point of view, though.  From the high-level documentation, it appears that the current NetHub executable will be going away to be replaced by a system try icon of some sort.  There are probably good reasons for keeping some kind of control interface for the hub, but looking at it strictly from the end user standpoint it seems like the hub should be as transparent as possible, with the user having no knowledge that it even exists.

 NetHubForm
Everything in here is pretty basic and clear to me.  The General Comments apply here once you get ready to redesign though.

ConnectionQueue
I was a little confused by the use of events within the queue, specifically the OnNewEnqueue event.  This was called each time a new connection was added to the queue, but it doesn’t appear the event handler actually does anything.  Maybe this is just a hook for some added extensibility if something ever needed to happen on an enqueue but I can’t see why it would be necessary.  No harm, though.

HubLink
My comments on HubLink might be better placed in the General Comments section because the deal with some very basic hub issues but this is where I first encountered them so I’ll leave my comments here.  The way services are currently setup, a client must provide a unique zone name, service name (not necessarily unique), and an unused port.  Should the burden for determining these unique names and ports be placed on the client or should the client rely on the NetHub to give it a unique name and an available port?  The latter seems preferable to me because these naming conflicts are NetHub issues and NetHub should solve them, not force the client to provide correct data.  Maybe both methods could be supported?

IConnection
Not the much to say here.  The interface looks pretty good and I assume its working fairly well too since we were using it for doors in the last milestone.

NetHub
Considering your use of exceptions throughout the class, I’m sure there’s some reason for returning null if you tried to add a service into a zone where another service of that name already exists.  I couldn’t go without mentioning it though….

ProcessManager
Currently it appears that stopProcess() kills every process with a certain name rather than a single instance of a process.  Is there a reason it would be preferable to always kill every instance of a process rather than referring to them by a process ID and killing a single process at a time?

RemotingEvents
Everything here seems clear enough.

RemotingManager
I’m not sure how valuable any comments I have in this section will be.  The code here was much more technical than anything else and I wasn’t able to get a very good grasp on what was going on.  This was probably more of an indication of the complexity of the code (and my ability to figure was going on) than any problems on your part.

RemotingQueue
This looks to be a basic queue, very similar to ConnectionQueue.  I don’t see any issues that need to be addressed here.

ServiceManager
Everything here seems pretty straightforward as well.  Obviously, something will need to happen eventually when a service is closed but no functionality there currently.

 

Ali Ongun

Flexibility:

  • The code is very well written and incredibly adaptable. It seems that the code was written in such a way that aside from instantiation there is always some valid input stored in the object.  For example the assigning of paths in HubLink.
  • The custom exceptions are very good ideas, they can be used to troubleshoot specific problems that happen in several places in the execution of the code.
  • It is very impressive that the code incorporates both push and pull scenarios within IConnection, I am assuming this will come in handy when a user closes a connection abruptly or the user is unable to send information on his own.

 

Extensibility:

  • Hiding things in factories was a good way to localize changes that would affect the rest of the system.
  • It seems that Remoting and Connection Queue are very similar and perhaps could inherit benefit from some sort of inheritance, while allowing the inner EnqueueEvent classes to have their own file.
  • Remoting Manager is written simply enough but there are so many internal classes that it might be better to move them out to simplify future changes to the code.

Correctness:

  • The code is written quite simply and is easy to step through.  It seems that the structure will not be a hindrance for making it work.
  • I did have the concern that when exceptions are thrown, sometimes they are not caught, as in the instance of InitChannel in HubLink, if it receives a value of –1 it will throw an exception and break the system.

Robustness:

  • There seem to be an appropriate number of error checking statements throughout the program.  While it is obvious that care has been taken to enable that there are valid parameters and limited use of typecasting, an excellent framework for catching where things could go wrong is also included.

Other Comments:

The AR group has done a very good job with a difficult portion.  The framework is well planned and implemented.  I want to point out that in my personal thinking the inner classes should be moved somewhere else, this just seems that it would make things simple.  Also there were a number of commented and non-commented debug statements.  Rather than doing this it might be better to implement a flag system that displays these debug statements when we are working on code, but able to be turned off for production.