2
\$\begingroup\$

Hi I need advice and review, here I have class AutomaticWorker which should constantly running and check if there is any file to process and after processing write data in database. OnDoWork method is inside while loop and started every 5 min to check if there is some job to do.

public override void OnDoWork()
{
    Exception ex = null;

    try
    {
        _automaticWorker  = new AutomaticWorker ();

        Service.Log(this, "Starting...", EventLogEntryType.Information);

        foreach(AutomaticProcesses process in _automaticWorker.GetAutomaticProcesses())
        {
            process.StartTransferProcessing();
        }

        Service.Log(this, "Finished...", EventLogEntryType.Information);
    }
    catch (Exception ex)
    {
        Service.Log(this, "Error: " + ex.Message + Constants.vbNewLine + ex.StackTrace, EventLogEntryType.Information);
    }
}


public class AutomaticWorker
{
    private readonly List<AutomaticProcess> _automaticProcesses;
    public List<AutomatskaObradaOperacije> AutomaticProcesses
    {
        get
        {
            return _automaticProcesses;
        }
    }

    public AutomaticWorker()
    {
        _automaticProcesses = AutomaticWorker.GetAutomaticProcesses();
    }

    private static List<AutomaticProcesses> GetAutomaticProcesses()
    {
        try
        {
            List<AutomaticProcesses> processes = new List<AutomaticProcesses>();

            using (SqlConnection defaultConnection = CNN.DefaultConnection)
            {
                string sqlQuery = string.Empty;

                sqlQuery = <string>...</string>.Value

                using (IDataReader reader = ExecuteReader(sqlQuery, defaultConnection))
                {

                    while (reader.Read())
                    {
                        AutomaticProcess process = new AutomaticProcess()
                        {
                            Code = reader("CODE").ToString.Trim,
                            Market = (MarketType)System.Convert.ToInt32(reader("MARKET")),
                            eMailInfo = dbNullOrNothingEmpty(reader("EMAIL_INFO"), "").ToString.Trim,
                            eMailError = dbNullOrNothingEmpty(reader("EMAIL_ERROR"), "").ToString.Trim,
                            Transfer = (TransferType)System.Convert.ToInt32(reader("TRANSFER_TYPE")),
                            Parameter = dbNullOrNothingEmpty(reader("PARAMETER"), "").ToString.Trim,
                            Path = dbNullOrNothingEmpty(reader("PATH"), "").ToString.Trim,
                            Extension = dbNullOrNothingEmpty(reader("EXTENSION"), "").ToString.Trim
                        };

                        processes.Add(process);
                    }
                }
            }

            return processes;
        }
        catch (Exception ex)
        {
            if (Debugger.IsAttached)
                Debugger.Break();
            throw new Exception("...");
        }
    }
}

Here is AutomaticProcess class which contains method StartTransferProcessing which process transfer based on transfer type, before process it scan specific file from directory and process each file in static method TransferProcesor.Processing(Transfer transfer).

public enum TransferType
{ 
     MC,
     DC,
     DG
}

public class AutomaticProcess
{
    public string Code { get; set; }
    public MarketType Market { get; set; }
    public string eMailInfo { get; set; }
    public string eMailError { get; set; }
    public TransferType Transfer { get; set; }
    public string Parameter { get; set; }
    public string Path { get; set; }
    public string Extension { get; set; }

    private DirectoryData _directoryFiles;

    public AutomaticProcess()
    {
    }

    private DirectoryData GetDirectoryData(string directory, string extension)
    {
        return dataDirectory; //this method scans directory and return all files with specific extension
    }

    public void StartTransferProcessing()
    {
    TransferResult result;

    try
    {
        switch (Transfer)
        {
            case TransferType.DC:
                {
                    TransferInfo transferInfo;

                    transferInfo =  GetAllTransferInfo()
                                                   .Where(f => f.TransferCode.ToUpper().Trim() == Parameter.ToUpper().Trim())
                                                   .FirstOrDefault();

                    _directoryFiles = GetFilesFromDirectory(transfer.Directory, Extension);

                    if (!_directoryFiles == null && !_directoryFiles.Files.Count == 0)
                    {

                        foreach (DataFile file in _directoryFiles.Files)
                        {
                            DCTransfer transfer = new DCTransfer()
                            {
                                Market = this.Market,
                                TransferData = transferInfo,
                                FileData = file,
                                TransferType = this.TransferType
                            };

                            result = TransferProcesor.Processing(transfer);

                            TransferProcesor.SaveResult(result); //save result in database, method with insert data in database

                        }
                    }

                    break;
                }

            case TransferType.MC:
                {
                    TransferInfo transferInfo;

                    transferInfo = GetAllTransferInfo()
                                                   .Where(f => f.TransferCode.ToUpper().Trim() == Parameter.ToUpper().Trim())
                                                   .FirstOrDefault();

                    _directoryFiles = GetFilesFromDirectory(transfer.Directory, Extension);

                    if (!_directoryFiles == null && !_directoryFiles.Files.Count == 0)
                    {                      

                        foreach (DataFile file in _directoryFiles.Files)
                        {
                            MCTransfer transfer = new MCTransfer()
                            {
                                Market = this.Market,
                                TransferData = transferInfo,
                                FileData = file,
                                TransferType = this.TransferType
                            };

                            result = TransferProcesor.Processing(transfer);

                            TransferProcesor.SaveResult(result); //save result in database, method with insert data in database

                        }
                    }

                    break;
                }

            case TransferType.DG:
                {

                    _directoryFiles = GetFilesFromDirectory(transfer.Directory, Extension);

                    if (!_directoryFiles == null && !_directoryFiles.Files.Count == 0)
                    {

                        foreach (DataFile file in _directoryFiles.Files)
                        {
                            DGTransfer transfer = new DGTransfer()
                            {
                                FileData = file,
                                TransferType = this.TransferType
                            };

                            result = TransferProcesor.Processing(transfer);

                            TransferProcesor.SaveResult(result); //save result in database, method with insert data in database
                        }
                    }

                    break;
                }

        }
    }
    catch (Exception ex)
    {
        if (Debugger.IsAttached)
            Debugger.Break();
        throw new Exception("...!", ex);
    }
}
}

Processing method creates specific object for processing each transfer depending on type.

public class TransferResult
{
    public MarketType Market { get; set; }
    public TransferType Type { get; set; }
    public TransferStatus Status { get; set; }
    public bool Success { get; set; }
    public string FileName { get; set; }
    public string Info { get; set; }
    public List<Errors> TransferErrors { get; set; }
}

public abstract class Transfer
{
    public FileData fileData { get; set; }
    public TransferType transferType  { get; set; }
}

public class DCTransfer : Transfer
{
    public TransferInfo TransferInfo { get; set; }
}

public class MCTransfer : Transfer
{
    public TransferInfo TransferInfo { get; set; }
}

public class DGTransfer : Transfer
{

}

public class TransferProcesor
{
    public static TransferResult Processing(Transfer transfer)
    {
        TransferResult result;

        switch (transfer.TransferType)
        {
            case TransferType.DC:
                {
                    DCTransferProcessing dcProcessing = new DCTransferProcessing(transfer);

                    result = dcProcessing.ProcessData();
                    break;
                }

            case TransferType.DG:
                {
                    DGTransferProcessing dgProcessing = new DGTransferProcessing(transfer);

                    result = dgProcessing.ProcessData();
                    break;
                }

            case TransferType.MC:
                {
                    MCTransferProcessing mcProcessing = new MCTransferProcessing(transfer);

                    result = mcProcessing.ProcessData();
                    break;
                }

            default:
                {
                    throw new NotImplementedException("...");
                    break;
                }
        }

        return result;
    }

}

For me this code looks a little bit ugly, is it better approach to use some design pattern? If you need further explanation for this classes or methods feel free to ask.

\$\endgroup\$
  • \$\begingroup\$ Does this code compile? 1) is RezultatTransfera actually TransferResult? 2) Transfer inheritors (e.g. MDCTransfer) are not used. 3) I don't see a TransferType implementation. 4) TransferType seems to be both a property and a class within the same scope - AutomaticProcess class. \$\endgroup\$ – radarbob Apr 25 at 20:25
  • \$\begingroup\$ Hi @radarbob, I translated from my mother language on some places I missed to translate on English, I edited my post and added enum definition for TransferType. \$\endgroup\$ – Anve Apr 25 at 20:33
2
\$\begingroup\$

This code needs better abstraction and Single Responsibility Principle re-structuring which will naturally facilitate pattern use. Right off the bat I see "factory pattern" as one result.


Factory Pattern

When abstract Transfer defines ALL members needed to do stuff then we can use factory pattern to build all the derived types. Same for those three xxTransferProcessing types.

// EVERYTHING needed to instantiate a Transfer must be in here.
public static class TransferFactory {

  // maybe no switch, but overloaded `Create` for different parameters. Or maybe use `params`.

  public static Transfer Create( TransferType thisGuy ) {
    Transfer aTransfer;

    switch ( thisGuy ) {

         case TransferType.DC: {
           aTransfer = new DCTransfer( thisGuy );
           break;
         }

         case TransferType.DG: {
           aTransfer = new DGTransfer( thisGuy );
           break;
         }

         case TransferType.MC: {
           aTransfer = new MCTransfer( thisGuy );
           break;
         }

         case TransferType.Undefined: { ... }  // see below

         default: { ... }
    }

    return aTransfer;
  }
}

TransferType enum

public enum TransferType { undefined, MC, DC, DG }

enum default value is zero, so might as well give it a name. Assigning it explicitly is saying "yeah, this guy is 'undefined' on purpose". The next time you have a 60 value enum used across dozens of classes you'll say "thanks for the tip, Bob." Yeah, you will. ;-)

In a way it makes a "nothing / null-like" value type safe with respect to TransferType variables.


Now the massive TransferProcess.StartTransferProcessing() total code is cut by at least 2/3.

transferInfo =  GetAllTransferInfo()
                .Where(f => f.TransferCode.ToUpper().Trim() == Parameter.ToUpper().Trim())
                .FirstOrDefault();

 _directoryFiles = GetFilesFromDirectory(transfer.Directory, Extension);

 if (!_directoryFiles == null && !_directoryFiles.Files.Count == 0)
 {
   foreach (DataFile file in _directoryFiles.Files)
   {
 // ******* LOOK! NO SWITCH *******
     Transfer = TranferFactory.Create( Transfer, this.Market,
                                       transferInfo, file )
   };

   result = TransferProcesor.Processing( transfer );

   TransferProcesor.SaveResult(result); 

// yeah, this should be in try/catch.
{
    if ( Debugger.IsAttached )
        Debugger.Break();
    throw new Exception("...!", ex);
}

abstract class Transfer

Move public TransferInfo TransferInfo into the abstract class. Make its implementation required and have some kind of "nothing" or "null-ish" implementation for classes like DCTransfer.

public abstract class Transfer
{
  public FileData fileData { get; set; }
  public TransferType transferType  { get; set; }
  public abstract TransferInfo TransferInfo ();
}

public class DCTransfer : Transfer  {     }

public class MCTransfer : Transfer  {     }

public class DGTransfer : Transfer  
{ public TransferInfo get{ return null; }  }

When all base-type object are consistent then client code gets much simpler. If client code must, for itself, differentiate derived types that's possibly a design defect (aka "code smell").

Resulting OO goodness:

public static TransferResult Processing(Transfer transfer)

becomes dead simple when we treat all derived objects the same way.

{
   Transfer Transfer = TransferFactory.Create(transfer.TransferType);
   TransferResult result = Transfer.ProcessData();
   return result;
}

abstract class TransferProcessing

Make a base for xxxTransferProcessing objects.

Resulting OO goodness:

public abstract class TransferProcesor {
{
  public static TransferResult Processing(Transfer transfer)
  {
     TransferResult result;
     TransferProcessing xxProcessing = ProcessFactory.Create( transfer )
     result = xxProcessing.ProcessData();
     return result;
  }
}

Collection classes

I sigh whenever I see this kind of thing:

private readonly List<AutomaticProcess> processes;

Do this instead:

public class AutoProcesses { 
  protected List<AutomaticProcess> processes { get; set; }

  public AutoProcesses() { processes = new List<AutomaticProcesses>() }

  public void Add(AutomaticProcess thisGuy) {
     if ( thisGuy == null ) return;

     processes.Add(thisGy);
  }

  pubic AutoProcesses AllDC() {
     AutoProcesses DcProcesses = new AutoProcesses();

     forEach (var process in processes) {
        DcProcesses.Add(this.processes.Find(process.Type == TransferType.DC);
     }

     return DcProcesses;   
  }
}

The OO goodness:

  • Look and feel of a business domain object
  • All List public members are hidden. Does only what you intend. This is why there's no inheritance.
  • Proper SRP. The collection knows how to find its own members, thank you very much.
  • Client code is vastly simplified because needed collection functionality is encapsulated in the right kind of class.
  • Client code becomes far more readable, understandable, and amazingly simplified.
  • Principle of least knowledge. Client does not do-it-yourself with internal access to the collection ( nor the contained objects ).

Which Design Pattern?

Heck, I don't know. Often a certain pattern may inspire some design aspect but without precisely implementing the pattern. IN ALL CASES, good OO design of your basic classes is essential. I hope I have shown this in getting to the factory pattern.

Factory is a construction pattern. Its purpose is to encapsulate details and complexities of building objects, giving clients a simple interface (method calls) to use it. However complex the construction process may be, the client is blissfully ignorant - all it knows is myFactory.Create( thisGuy ).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you @radarbob for your advice and review \$\endgroup\$ – Anve Apr 26 at 19:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.