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. Apparently with all the bells and whistles on developer’s website, it is not.

  • Using a strange naming conversion for your project like xCodeBase, xODR is BAD.
  • Having A LOT of commented code in your code base is a BAD practice.
  • Exposing class fields as public is a BAD practice. Try using properties.
  • Using IF statements to check for database types ALL OVER THE PLACE is VERY BAD.
  • Obfuscated class and method names are considered BAD (unless you’ve used an obfuscator) 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 were 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”. 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 does not looks like a unit test:

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 the saying goes like: “Even chimp writes code better than that”.r than that”.