4
\$\begingroup\$

I have a console application that basically retrieves XML content, writes to the XML file and sends the file to an SFTP site:

public Main(string[] args)
{
 try
 {
   //code to parse arguments to load into DTO to extract stored proc and report name
  List<ReportInfo> reports = Parse(args);

   foreach(var rpt in reports)
   {
      //retrieve xml content from database 
       XmlDocument doc = new XmlDocument();
       using (SqlConnection con = new SqlConnection(AppConfig.ConnectionString))
        {
            using (SqlCommand cmd = con.CreateCommand())
            {
                cmd.CommandType = CommandType.StoredProcedure;
                cmd.CommandText = rpt.StoredProcedure;
                con.Open();
                using (var reader = cmd.ExecuteXmlReader())
                {
                    doc.Load(reader);
                    reader.Close();
                }
                con.Close();
            }
        }

        //save xml file in a folder
        string filePath = Path.Combine(AppConfig.ReportsFolder, string.Format(rpt.FileName, DateTime.Today.ToString("MMddyyyy")));
        using (var fileStream = new FileStream(filePath, FileMode.Create, FileAccess.Write, FileShare.None))
        {
            using (var xmlWriter = XmlWriter.Create(fileStream,
                      new XmlWriterSettings
                      {
                          OmitXmlDeclaration = false,
                          ConformanceLevel = ConformanceLevel.Document,
                          Encoding = Encoding.UTF8
                      }))
            {
                xmldoc.Save(xmlWriter);
            }
        }

     // third party tool is called to transmit the file 
     SFtpClient client = new SFtpClient("host","user","pwd");
     client.Send(filPpath);
  }
 }
 catch(Exception ex)
 {
    _iLogger.Error(ex);
 }
}

Main() consists of considerable amount of lines. So, I have decided to split functionality into smaller classes similar to SOLID principles like this:

public Main(string[] args)
{
  try
  {
    List<ReportInfo> reports = Parse(args);

    foreach(var rpt in reports)
    {
         XmlDocument xmldoc = DBHelper.GetReport(rpt.sproc);

         var filePath = ReportProcessor.SaveFileAsXml(xmldoc);
         ReportProcessor.SendFileviaSFtp(filePath);         
     }
 }
 catch(Exception ex)
 {
    _iLogger.Error(ex);
 }
}


public static class ParametersParser
{
      public static List<ReportsInfo> Parse(string[] args)
      {
           //parse the args        
      }
}

public static class DBHelper
{
      public static XmlDocument GetReport(string storedprocedure)
      {
        XmlDocument doc = new XmlDocument();
        using (SqlConnection con = new SqlConnection(AppConfig.ConnectionString))
        {
            using (SqlCommand cmd = con.CreateCommand())
            {
                cmd.CommandType = CommandType.StoredProcedure;
                cmd.CommandText = storedprocedure;
                con.Open();
                using (var reader = cmd.ExecuteXmlReader())
                {
                    doc.Load(reader);
                    reader.Close();
                }
                con.Close();
            }
        }
        return doc;
      }
}

public static class ReportProcessor
{
      public static string SaveFileAsXml(string fileName, XmlDocument xmldoc)
      {
         string filePath = Path.Combine(AppConfig.ReportsFolder, string.Format(fileName, DateTime.Today.ToString("MMddyyyy")));

        using (var fileStream = new FileStream(filePath, FileMode.Create, FileAccess.Write, FileShare.None))
        {
            using (var xmlWriter = XmlWriter.Create(fileStream,
                      new XmlWriterSettings
                      {
                          OmitXmlDeclaration = false,
                          ConformanceLevel = ConformanceLevel.Document,
                          Encoding = Encoding.UTF8
                      }))
            {
                xmldoc.Save(xmlWriter);
            }
        }

        return filePath;
      }

     public static void SendFileviaSFtp(string filePath)
     {
        //here third party tool is called to transmit the file 
         SFtpClient client = new SFtpClient("host","user","pwd");
         client.Send(filepath);
     }
}

Here are my doubts with these modifications:

  1. As observed, the classes are static. I felt they are more of utility classes and placed them all in Helpers folder. So, is it advisable to use static classes this way or shall I shift to normal classes?
  2. By having static classes and methods, do I run into threading issues as there might be chances multiple instances of this exe might execute concurrently?
  3. Even after splitting into different classes, I have the exception handling and logging inside Main(). Is it ok to have it handle at single location or shall I also include in individual methods and throw specific exception?

    public static class ParametersParser
    {
        public static List<ReportsInfo> Parse(string[] args)
        {
            try
            {
                //parse the args  
            }
            catch(Exception ex)
            {
                throw new Exception("ParametersParser.Parse", ex);
            }
        }
    }
    
\$\endgroup\$
6
\$\begingroup\$

I would start by looking at functional decomposition. Here's crack #1 at it:

internal static class Solid
{
    private static readonly XmlWriterSettings settings = new XmlWriterSettings
    {
        OmitXmlDeclaration = false,
        ConformanceLevel = ConformanceLevel.Document,
        Encoding = Encoding.UTF8
    };

    public static void Main(string[] args)
    {
        try
        {
            ProcessReports(args, AppConfig.ConnectionString, AppConfig.ReportsFolder);
        }
        catch (Exception ex)
        {
            _iLogger.Error(ex);
        }
    }

    private static void ProcessReports(string[] args, string connectionString, string reportsFolder)
    {
        // code to parse arguments to load into DTO to extract stored proc and report name
        foreach (var rpt in Parse(args))
        {
            ProcessSingleReport(connectionString, reportsFolder, rpt);
        }
    }

    private static IEnumerable<ReportInfo> Parse(string[] args)
    {
        // Do actual argument parsing here...
        return Enumerable.Empty<ReportInfo>();
    }

    private static void ProcessSingleReport(string connectionString, string reportsFolder, ReportInfo rpt)
    {
        var doc = LoadReportXml(connectionString, rpt);
        var filePath = Path.Combine(reportsFolder, string.Format(rpt.FileName, DateTime.Today.ToString("MMddyyyy")));

        WriteFile(filePath, doc);
        TransmitFile(filePath);
    }

    private static XmlDocument LoadReportXml(string connectionString, ReportInfo rpt)
    {
        using (var con = new SqlConnection(connectionString))
        using (var cmd = con.CreateCommand())
        {
            cmd.CommandType = CommandType.StoredProcedure;
            cmd.CommandText = rpt.StoredProcedure;
            con.Open();
            using (var reader = cmd.ExecuteXmlReader())
            {
                var doc = new XmlDocument();

                doc.Load(reader);
                return doc;
            }
        }
    }

    private static void WriteFile(string filePath, XmlDocument doc)
    {
        using (var fileStream = new FileStream(filePath, FileMode.Create, FileAccess.Write, FileShare.None))
        using (var xmlWriter = XmlWriter.Create(fileStream, settings))
        {
            doc.Save(xmlWriter);
        }
    }

    private static void TransmitFile(string filePath)
    {
        // third party tool is called to transmit the file
        SFtpClient client = new SFtpClient("host", "user", "pwd");

        client.Send(filePath);
    }
}

Then, you can see there's a definite separation of concerns, and therefore separate classes:

internal static class Solid
{
    public static void Main(string[] args)
    {
        try
        {
            var argumentParser = new ArgumentParser(args);
            var reportLoader = new ReportLoader(AppConfig.ConnectionString);
            var reportProcessor = new ReportProcessor(AppConfig.ReportsFolder, reportLoader);
            var reportsProcessor = new ReportsProcessor(argumentParser, reportProcessor);

            reportsProcessor.ProcessReports();
        }
        catch (Exception ex)
        {
            _iLogger.Error(ex);
        }
    }
}

/// <summary>
/// code to parse arguments to load into DTO to extract stored proc and report name
/// </summary>
internal sealed class ArgumentParser
{
    private readonly string[] args;

    public ArgumentParser(string[] args)
    {
        this.args = args;
    }

    public IEnumerable<ReportInfo> Parse()
    {
        // Do actual argument parsing here...
        return Enumerable.Empty<ReportInfo>();
    }
}

internal sealed class ReportLoader
{
    private readonly string connectionString;

    public ReportLoader(string connectionString)
    {
        this.connectionString = connectionString;
    }

    public XmlDocument LoadReportXml(ReportInfo rpt)
    {
        using (var con = new SqlConnection(this.connectionString))
        using (var cmd = con.CreateCommand())
        {
            cmd.CommandType = CommandType.StoredProcedure;
            cmd.CommandText = rpt.StoredProcedure;
            con.Open();
            using (var reader = cmd.ExecuteXmlReader())
            {
                var doc = new XmlDocument();

                doc.Load(reader);
                return doc;
            }
        }
    }
}

internal sealed class ReportProcessor
{
    private readonly string reportsFolder;

    private readonly ReportLoader reportLoader;

    public ReportProcessor(string reportsFolder, ReportLoader reportLoader)
    {
        this.reportsFolder = reportsFolder;
        this.reportLoader = reportLoader;
    }

    public void ProcessSingleReport(ReportInfo rpt)
    {
        var doc = this.reportLoader.LoadReportXml(rpt);
        var filePath = Path.Combine(this.reportsFolder, string.Format(rpt.FileName, DateTime.Today.ToString("MMddyyyy")));

        new ReportWriter(filePath, doc).WriteFile();
        new Transmitter(filePath).TransmitFile();
    }
}

internal sealed class ReportsProcessor
{
    private readonly ArgumentParser parser;

    private readonly ReportProcessor reportProcessor;

    public ReportsProcessor(ArgumentParser parser, ReportProcessor reportProcessor)
    {
        this.parser = parser;
        this.reportProcessor = reportProcessor;
    }

    public void ProcessReports()
    {
        foreach (var rpt in this.parser.Parse())
        {
            this.reportProcessor.ProcessSingleReport(rpt);
        }
    }
}

internal sealed class ReportWriter
{
    private static readonly XmlWriterSettings settings = new XmlWriterSettings
    {
        OmitXmlDeclaration = false,
        ConformanceLevel = ConformanceLevel.Document,
        Encoding = Encoding.UTF8
    };

    private readonly string filePath;

    private readonly XmlDocument doc;

    public ReportWriter(string filePath, XmlDocument doc)
    {
        this.filePath = filePath;
        this.doc = doc;
    }

    public void WriteFile()
    {
        using (var fileStream = new FileStream(this.filePath, FileMode.Create, FileAccess.Write, FileShare.None))
        using (var xmlWriter = XmlWriter.Create(fileStream, settings))
        {
            this.doc.Save(xmlWriter);
        }
    }
}

internal sealed class Transmitter
{
    private readonly string filePath;

    public Transmitter(string filePath)
    {
        this.filePath = filePath;
    }

    public void TransmitFile()
    {
        // third party tool is called to transmit the file
        SFtpClient client = new SFtpClient("host", "user", "pwd");

        client.Send(this.filePath);
    }
}

Extract out some interfaces for each of these classes, inject some more of the smaller dependencies here and there, and you'll have a pretty SOLID class structure.

\$\endgroup\$
  • \$\begingroup\$ Thanks Jesse for spending time on this. Much appreciated. Do you have any links specifically which deals with binding of classes to use SOLID principles. \$\endgroup\$ – Sunny Aug 15 '13 at 17:14
  • \$\begingroup\$ Also, the error handling in the Main() should be ok right? Instead of writing try and catch in individual classes? \$\endgroup\$ – Sunny Aug 15 '13 at 17:15
  • \$\begingroup\$ Best practice for exception handling is to handle the specific exception you know could happen, if you can. Otherwise, let it bubble up the call stack until something can. A blanket catch (Exception ex) in every method is a bad, bad idea as it hides potential logic errors. \$\endgroup\$ – Jesse C. Slicer Aug 15 '13 at 17:38
  • \$\begingroup\$ Last one, is it advisable to instantiate ReportWriter and Transmitter directly in the ProcessSingleReport method. \$\endgroup\$ – Sunny Aug 15 '13 at 18:36
  • 1
    \$\begingroup\$ I'm going to break out a time-honored development mantra: "It depends" :) Personally, if it were me, I'd probably leave those two classes as static (or make them singletons) since their function is cut-and-dried utility. \$\endgroup\$ – Jesse C. Slicer Aug 15 '13 at 19:22

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.