Technical documentation of the code/project seem like a worthy goal.
After all if we aren't living in a vacuum someone will need to
understand our code/design and maintain it, use it to develop new stuff
etc.
On the other hand writing documentation can get tedious,
boring, hard to maintain and whatnot - so you get all sorts of ways to
deal with it. Consider, for example, the following:
- On a recent article in IBM's DeveloperWorks
Paul Duvall suggests you use automation to generate various
documentation artifacts like UML diagrams based on the source code
(using ANT tasks and UMLGraph), ERDs (with SchemaSpy) etc.
- Another example, is a .Net tool called GhostDoc. This tool automagically generates C# XML documentation comments ("///") e.g. (all comments below are ghostdoc work):
/// <summary>
/// Appends the HTML text.
/// </summary>
/// <param name="htmlProvider">The HTML provider.</param>
public void AppendHtmlText( IHtmlProvider htmlProvider )
/// <summary>
/// Adds the specified item.
/// </summary>
/// <param name="item">The item.</param>
public void Add( string item )
/// <summary>
/// Determines the size of the page buffer.
/// </summary>
/// <param name="initialPageBufferSize">Initial size of the page buffer.</param>
/// <returns></returns>
public int DeterminePageBufferSize( int initialPageBufferSize )
[from Introduction to GhostDoc]
What
I think is that while both of these efforts can help satisfy a customer
specific requirement for "comprehansive documentation"* they have
very little value
in making anyone understand anything about your code. UML diagrams can
only help if they are created at a higher level of abstraction than the
code (which means they'd be hand-crafted) and if GhostDoc can
understand your code enough to create anything useful it means that
your method and parameter names are self-descriptive anyway.
In a previous post I
mentioned that I prefer to rely on tests, short methods and meaningful
names for readability. I'll talk about tests in another post, for this
installment lets look at the other two. I think it would be better
demonstrated by an example.
Consider the following horror of a method:
public void HandleWithFrame(FrameProp Frame)
{
int FreeProcessNum = 0;
int FreeProcessId = 0;
if (Frame != null)
{
rwl.EnterWriteLock();
if (m_WaitingFrame.ContainsKey(Frame.m_SessionId))
m_WaitingFrame[Frame.m_SessionId].m_Frame = Frame;
else
m_WaitingFrame.Add(Frame.m_SessionId, new WaitingFrame(Frame));
IncrementPriority();
rwl.ExitWriteLock();
}
rwl.EnterUpgradeableReadLock();
foreach (var keyValuePair in m_ProcessMap)
{
if (keyValuePair.Value.m_Busy == false)
{
FreeProcessId = keyValuePair.Key;
++FreeProcessNum;
if (FreeProcessNum > 1)
break;
}
}
if (FreeProcessNum == 0)
{
rwl.ExitUpgradeableReadLock();
return;
}
if (FreeProcessNum >= 2 )
{
rwl.EnterWriteLock();
m_ProcessMap[FreeProcessId].SendFrame2CV(Frame);
m_WaitingFrame[Frame.m_SessionId].m_NumProcess += 1;
m_WaitingFrame[Frame.m_SessionId].m_Priority = 0;
m_WaitingFrame[Frame.m_SessionId].m_Frame = null;
m_ProcessMap[FreeProcessId].m_Busy = true;
rwl.ExitWriteLock();
rwl.ExitUpgradeableReadLock();
return ;
}
WaitingFrame MaxPriority = new WaitingFrame();
MaxPriority.m_NumProcess = 1000;
MaxPriority.m_Priority = -1;
foreach (var Item in m_WaitingFrame)
{
if (Item.Value.m_NumProcess < MaxPriority.m_NumProcess)
if (Item.Value.m_Priority > MaxPriority.m_Priority)
MaxPriority.m_Frame = Item.Value.m_Frame;
}
rwl.EnterWriteLock();
// Console.WriteLine("ProcessId={0} Assign", FreeProcessId);
m_ProcessMap[FreeProcessId].SendFrame2CV(MaxPriority.m_Frame);
m_ProcessMap[FreeProcessId].m_Busy = true;
m_WaitingFrame[MaxPriority.m_Frame.m_SessionId].m_NumProcess += 1;
m_WaitingFrame[MaxPriority.m_Frame.m_SessionId].m_Priority = 0;
m_WaitingFrame[MaxPriority.m_Frame.m_SessionId].m_Frame = null;
rwl.ExitWriteLock();
rwl.ExitUpgradeableReadLock();
return ;
}
This
class needs a lot of explanations if you want to understand what
exactly going on here. So you can set your self up to writing a lot of
comments and trying to figure things our - or assuming this class was
fully tested (which it wasn't, but that's another story) try to
refactor it until we get something meaningful (I will omit the added
tests though for brevity)
Step1 - First If
It seems that when we have a first frame we want to keep it aside, so let's
extract method all the if code to EnqueueFrame
if (Frame != null)
EnqueueFrame(Frame);
Ok
so now we look at EnqueueFrame, The code we see here talks with the
m_WaitingFrame private member (which is a Dictionary of <Guid,
WaitingFrame>();. The first thing we'll do is to
rename it
to FramesQueue. Now the more interesting thing is that the code here
has to do with managing this FramesQueue and isn't directly related to
the containing class.
We can either subclass the Dictionary class or
we can add an extnention method to Dictionary<Guid,WaitingFrame>
to handle this for us.
Do we'll do that and then refactor the If again:
if (Frame != null)
{
rwl.EnterWriteLock();
FramesQueue.Enqueue(Frame);
rwl.ExitWriteLock();
}
and Enqueue looks like (in a separate interanal static class)
public static void Enqueue(this Dictionary<Guid, WaitingFrame> queue, FrameProp Frame)
{
if (queue.ContainsKey(Frame.m_SessionId))
queue[Frame.m_SessionId].m_Frame = Frame;
else
queue.Add(Frame.m_SessionId, new WaitingFrame(Frame));
queue.Prioritize();
}
The
advantage of what we've achieved thus far is both better OO design
(separation of concerns) and enhanced readability by using intention
revealing names and notation.
Step 2 - The foreach loop
Again we'll start with
Extract Method, we can now remove the definition of FreeProcessNum from the beginging and we get
var FreeProcessNum = GetFreeProcessNum(ref FreeProcessId);
but
this code is not really clear, for one we have to rename FreeProcessNum
to FreeProcessesCount to make it more legible. and we have an ugly and
hard to follow ref variable. It is probably better to apply the Single
Responsibility principle and seperate this into two distinct methods,
so we'd get
var FreeProcessesCount = ProcessesList.CountFree();
var FreeProcessId = ProcessesList.GetNextFreeId(); // we don't really need/want the ID but we'll fix that later
(as
in the previous example we add extension methods to ProcessesList to
make the code more intention revealing and for better seperation of
concerns)
All we want to do in CountFree is count how many proccesses are not marked as busy so we can rewrite
var FreeProcessNum = 0;
foreach (var keyValuePair in ProcessesList)
{
if (keyValuePair.Value.m_Busy == false)
{
FreeProcessId = keyValuePair.Key;
++FreeProcessNum;
if (FreeProcessNum > 1)
break;
}
}
return FreeProcessNum;
into
public static int CountFree(this Dictionary<int, ProcessStatus> processesList)
{
return processesList.Count(item => item.Value.m_Busy == false);
}
Thank you MS for adding Linq and Lambda expressions :). The same can be done for GetNextFreeId
Step 3 the two Ifs and the rest
Taking
a deep look at the code we can see that the rest of the method tries to
find a free processor and if there are enough processors send the
frame, otherwise it should send the top prioritized. We can also spot a
bug here that two different threads can get the same Processor and then
try to send a message to it one after the other. Another potential bug
comes from the way the maximal priority is found. There's an assumption
there that the max priority would be 1000. While it isn't likely to
happen it is still a hard coded assumption.
Anyway, if we continue
and apply the same principles that got us here (Single Responsibility
Principle, Don't Repeat Yourself, Intention revealing methods,
coherence and opening classes to add specific functionaliy) we get the
original method to look like the following:
public void ProcessFrame(FrameProp nextFrame) //was HandleWithFrame
{
rwl.EnterWriteLock();
try
{
if (nextFrame != null)
FramesQueue.Enqueue(nextFrame);
TryDispatchTopFrame();
}
finally
{
rwl.ExitWriteLock();
}
}
Compare this with the original method....
Also
note that it isn't that the functionality disappeared - it is just
neatly distributed and grouped in short related methods in related
classes e.g.
internal static class FramesQueueExtnesions
{
public static void Enqueue(this Dictionary<Guid, WaitingFrame> queue, FrameProp Frame)
{
if (queue.ContainsKey(Frame.m_SessionId))
queue[Frame.m_SessionId].m_Frame = Frame;
else
queue.Add(Frame.m_SessionId, new WaitingFrame(Frame));
queue.UpdatePriorities();
}
public static void ResetSlot(this Dictionary<Guid, WaitingFrame> queue,Guid slotId)
{
queue[slotId].m_NumProcess += 1;
queue[slotId].m_Priority = 0;
queue[slotId].m_Frame = null;
}
public static void UpdatePriorities(this Dictionary<Guid, WaitingFrame> queue)
{
foreach (var Item in queue)
{
if (Item.Value.m_Frame != null)
Item.Value.m_Priority += 1;
}
}
public static FrameProp FindTopPrioritized(this Dictionary<Guid,WaitingFrame> queue)
{
var maxPriority= queue.Max(item => item.Value.m_Priority);
return queue.First(item => item.Value.m_Priority == maxPriority).Value.m_Frame;
}
}
You should note that this is not the end of the refactoring (e.g. we should still handle the WaitingFrame, FrameQueue and the ProcessesList which we are called here) we just took a look at a single method.
While
there might still be a need for an occasional explanatory remark , I
think this little exercise demonstrate that we can gain a lot in the
way of clarity by refacroting code and keeping up a few simple
principles. Oh yea, and what we got at the end of the process is not
just readable code, but also a more maintainable, better designed code
that can move forward and evolve further as the system changes.
* I don't underestimate the value of
generating full documentation
when there's
such a requirement from a customer. I would prefer to convince a
customer that having such a Write-Only document is a complete waste of
time and trees but sometimes you can't help it. Generating documents
in these situations can be a life-saver.