Tuesday, April 28, 2009

Code Review No. N, or Do chimps write code better than us?

Well, this is not actually my first code review, but this is supposed to be a copycat of “Code Review WTF, Number N” series by Ayende Rahien.

I was inspecting an open-source code base which is a medical business application. So why am I reviewing the code base? I had a request to add persian calendar support to this application and I was trying to figure out how much time does this take and if it is possible.

There are very serious issues here, I even don’t know where to start! For a open-source project at its 6th version, I was hoping things to be pretty much solid. Apparantly with all the bells and whistles on developer’s website, it is not.

  • Using a strange naming convension for your project like xCodeBase, xODR is BAD.
  • Having A LOT of commented code in your code base is BAD.
  • Exposing class fields as public is BAD. use properties.
  • Using IFs to check for database type EVERYWHERE is VERY BAD.
  • Obfuscated class and method names are BAD. (as in Lan class which does the string translation job with public methods named g, F and C)!!
  • Wrapping remote method names in an ENUM. What are you thinking?!

After going through some of the classes, I think I’m going crazy! Okay, let’s see how’s the business logic being handled. That’s the major part of the application, right?

Data is accessed by DataCore class in server AND client side (depending if remoting is used), which retrieves the data in an UnTyped dataset. To make things easier(?) there’s one single method to invoke remote BusinessLayer implementation methods:

public static DataSet GetDsByMethod(MethodNameDS methodName, object[] parameters)
{
switch (methodName)
{
default:
throw new ApplicationException("MethodName not found");
case MethodNameDS.AccountModule_GetAll:
return AccountModules.GetAll((int)parameters[0], (bool)parameters[1], (DateTime)parameters[2], (DateTime)parameters[3], (bool)parameters[4]);
case MethodNameDS.Appointment_GetApptEdit:
return Appointments.GetApptEdit((int)parameters[0]);
case MethodNameDS.Cache_Refresh:
return Cache.Refresh((string)parameters[0]);
case MethodNameDS.CovCats_RefreshCache:
return CovCats.RefreshCache();
}
}

public enum MethodNameDS
{
AccountModule_GetAll,
Appointment_GetApptEdit,
Cache_Refresh,
CovCats_RefreshCache,
}

Dude! WTF is that?! and then there’s a similar method to work with DataTables!

On the server where client requests are processed, data is being serialized / deserialzed by a “WorkerClass” class. The code to perform the client’s request is what makes me wonder!

public class WorkerClass
{
private NetworkStream netStream;

// The constructor obtains the state information.
public WorkerClass(NetworkStream stream)
{
netStream = stream;
}

public void DoWork()
{
while(true) {//Each loop gets and returns one message pair.
byte[] data = null;
// Retrieve data from client
try {
data = RemotingClient.ReadDataFromStream(netStream);
}
catch {//if connection was closed by client.
break;
}
DataTransferObject dto=DataTransferObject.Deserialize(data);
//Process and send response to client--------------------------------------
XmlSerializer serializer;
using (MemoryStream memStream = new MemoryStream()) {
try {
Type type = dto.GetType();
if (type == typeof(DtoGetDS)) {
DataSet ds = DataCore.GetDsByMethod(((DtoGetDS)dto).MethodNameDS, ((DtoGetDS)dto).Parameters);
serializer = new XmlSerializer(typeof(DataSet));
serializer.Serialize(memStream, ds);
}
else if (type == typeof(DtoGetTable)) {
DataTable tb = DataCore.GetTableByMethod(((DtoGetTable)dto).MethodNameTable, ((DtoGetTable)dto).Parameters);
serializer = new XmlSerializer(typeof(DataTable));
serializer.Serialize(memStream, tb);
}
else if (type.BaseType == typeof(DtoCommandBase)) {
int result = BusinessLayer.ProcessCommand((DtoCommandBase)dto);
DtoServerAck ack = new DtoServerAck();
ack.IDorRows = result;
serializer = new XmlSerializer(typeof(DtoServerAck));
serializer.Serialize(memStream, ack);
}
else if (type.BaseType == typeof(DtoQueryBase)) {
DataSet ds = BusinessLayer.ProcessQuery((DtoQueryBase)dto);
serializer = new XmlSerializer(typeof(DataSet));
serializer.Serialize(memStream, ds);
}
else if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(FactoryTransferObject<>)) {
// Pass the DTO to the FactoryServer<T>
Type factoryServerType = typeof(FactoryServer<>);
factoryServerType = factoryServerType.MakeGenericType(type.GetGenericArguments());

MethodInfo processCommandMethod = factoryServerType.GetMethod("ProcessCommand", BindingFlags.Public | BindingFlags.Static);
processCommandMethod.Invoke(null, new object[] { memStream, dto });
}
else {
throw new NotSupportedException(string.Format(Resources.DtoNotSupportedException, type.FullName));
}
}
catch (Exception e) {
DtoException exception = new DtoException();
exception.Message = e.Message;
serializer = new XmlSerializer(typeof(DtoException));
serializer.Serialize(memStream, exception);
}
data = memStream.ToArray();
RemotingClient.WriteDataToStream(netStream, data);
}
}//wait for the next message
//connection was lost. Client probably closed program
netStream.Close();
}

The code is so full of anti-patterns that I wonder how it even works. But wait…I just saw a Unit Test project! Something good, finally, eh? But nah….It is a WinForm application and the test code looks like this:

private void FormUnitTests_Load(object sender, EventArgs e)
{
BenefitComputeRenewDate();
ToothFormatRanges();
//LabDueDate();
textResults.Text += "Done.";
textResults.SelectionStart = textResults.Text.Length;
}

private void BenefitComputeRenewDate()
{
DateTime asofDate = new DateTime(2006, 3, 19);
bool isCalendarYear = true;
DateTime insStartDate = new DateTime(2003, 3, 1);
DateTime result = BenefitLogic.ComputeRenewDate(asofDate, isCalendarYear, insStartDate);
if (result != new DateTime(2006, 1, 1))
{
textResults.Text += "BenefitComputeRenewDate 1 failed.\r\n";
}
isCalendarYear = false; //for the remaining tests
//earlier in same month
result = BenefitLogic.ComputeRenewDate(asofDate, isCalendarYear, insStartDate);
if (result != new DateTime(2006, 3, 1))
{
textResults.Text += "BenefitComputeRenewDate 2 failed.\r\n";
}
//earlier month in year
asofDate = new DateTime(2006, 5, 1);
result = BenefitLogic.ComputeRenewDate(asofDate, isCalendarYear, insStartDate);
if (result != new DateTime(2006, 3, 1))
{
textResults.Text += "BenefitComputeRenewDate 3 failed.\r\n";
}
}
I think that’s enough proof for one day. You probably have heard the saying: “Even a chimp can write code”, but sometimes it’s like: “Even chimp writes code better than that”.
Submit this story to DotNetKicks Shout it

3 comments:

wilzad said...

whats is the project's name buddy?

Hadi Eskandari said...

The name is 'OpenDental'. It is an open-source project in SourceForge.net AFAIK

Pictures were removed when I relocated my old hosting to the new one. sorry for the inconvenience, but those are goner!

Anonymous said...
This comment has been removed by a blog administrator.