Parsing remote text file and inserting into database table

Question

I did run this code through StyleCop. Goal was to optimize the reading, parsing and inserting of a file on the web and write the rows to a database table using a stored procedure. Contrast this (roughly) to my most previous code review here: C# helper class - mailto Different code but I feel like I'm making progress, for being still fairly new to C#. Thanks.

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;

namespace NewReleases
{
    public class Program
    {
        public static void Main(string[] args)
        {
            const string PremierPublishers = "PREMIER PUBLISHERS";
            const string NewReleasesFor = "New Releases For";

            ReleaseItem releaseItem = new ReleaseItem();
            List<string> premierPublishers = Release.PremierPublishers();
            DateTime releaseDate = new DateTime();

            foreach (var line in Release.GetRelease())
            {
                if (line.Contains(NewReleasesFor))
                {
                    releaseDate = DateTime.Parse(line.Substring(line.Length - 10));
                }
                else
                {
                    if (!line.Any(p => p.ToString().Contains("\t")) & !premierPublishers.Any(p => p == line))
                    {
                        releaseItem.Category = line.Trim();
                    }
                    else if (premierPublishers.Any(p => p == line))
                    {
                        releaseItem.Publisher = line.Trim();
                    }
                    else
                    {
                        string[] lineitem = line.Split('\t');

                        if (lineitem.Count() == 3)
                        {
                            releaseItem.ItemCode = lineitem[0].Trim();
                            releaseItem.Title = lineitem[1].Trim();
                            releaseItem.Price = lineitem[2].Trim();

                            if (releaseItem.Category != PremierPublishers & !premierPublishers.Any(p => p == line))
                            {
                                releaseItem.Publisher = null;
                            }

                            Release.WriteRelease(releaseDate, releaseItem);
                        }
                    }
                }
            }
        }           
    }
}

Release.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.IO;
using System.Data;
using System.Configuration;
using System.Data.SqlClient;
using Dapper;

namespace NewReleases
{
    public static class Release
    {
        private const string RemoteReleaseFile = "http://www.previewsworld.com/shipping/newreleases.txt";

        public static IEnumerable<string> GetRelease()
        {
            using (WebClient webClient = new WebClient())
            {
                Stream stream = webClient.OpenRead(RemoteReleaseFile);
                List<string> lines = new List<string>();

                using (StreamReader streamReader = new StreamReader(stream))
                {
                    while (!streamReader.EndOfStream)
                    {
                        lines.Add(streamReader.ReadLine());
                    }
                }

                return lines.Where(f => !string.IsNullOrWhiteSpace(f));
            }
        }

        public static List<string> PremierPublishers()
        {
            using (IDbConnection connection = new SqlConnection(ConfigurationManager.AppSettings["Previews"]))
            {
                return connection.Query<string>("Select PremierPublisher From PremierPublishers").ToList();
            }
        }

        public static void WriteRelease(DateTime releaseDate, ReleaseItem releaseItem)
        {
            using (IDbConnection connection = new SqlConnection(ConfigurationManager.AppSettings["Previews"]))
            {
                decimal price = 0;
                string note = null;
                bool isDecimal = decimal.TryParse(releaseItem.Price.Replace("$", string.Empty), out price);

                if (!isDecimal)
                {
                    note = releaseItem.Price;
                }
                else
                {
                    price = Convert.ToDecimal(releaseItem.Price.Replace("$", string.Empty));
                }

                connection.Execute(
                    "InsertReleaseItem",
                    new
                    {
                        releaseDate,
                        releaseItem.Category,
                        releaseItem.Publisher,
                        releaseItem.ItemCode,
                        releaseItem.Title,
                        price,
                        note                                                
                    },
                    commandType: CommandType.StoredProcedure);
            }
        }
    }
}

ReleaseItem.cs

namespace NewReleases
{
    public class ReleaseItem
    {
        public string Category { get; set; }
        public string Publisher { get; set; }
        public string ItemCode { get; set; }
        public string Title { get; set; }
        public string Price { get; set; }
    }
}

Answer 1

Let's refactor it a little bit toward SOLID. It's not yet perfect but is should give you an idea how to start (you should take @Heslacher's advices into account)

---

Add the release date to the release item:

public class ReleaseItem
{
    public string Category { get; set; }
    public string Publisher { get; set; }
    public string ItemCode { get; set; }
    public string Title { get; set; }
    public string Price { get; set; }
    public DateTime ReleasedOn { get; set; }
}

Create a release downloader to get the lines:

public class ReleaseDownloader
{
    public IEnumerable<string> GetRelease(string remoteReleaseFile)
    {
        using (WebClient webClient = new WebClient())
        {
            Stream stream = webClient.OpenRead(remoteReleaseFile);
            List<string> lines = new List<string>();

            using (StreamReader streamReader = new StreamReader(stream))
            {
                while (!streamReader.EndOfStream)
                {
                    lines.Add(streamReader.ReadLine());
                }
            }

            return lines.Where(f => !string.IsNullOrWhiteSpace(f));
        }
    }    
}

Create a release repository to take care of the database interaction:

public class ReleaseRepository
{    
    public ReleaseRepository(string connectionString)
    {
        ConnectionString = connectionString;
    }

    public string ConnectionString { get; }

    public List<string> PremierPublishers()
    {
        using (IDbConnection connection = new SqlConnection(ConnectionString))
        {
            return connection.Query<string>("Select PremierPublisher From PremierPublishers").ToList();
        }
    }

    public void SaveRelease(ReleaseItem releaseItem)
    {
        using (var connection = new SqlConnection(ConnectionString))
        {
            decimal price = 0;
            string note = null;
            bool isDecimal = decimal.TryParse(releaseItem.Price.Replace("$", string.Empty), out price);

            if (!isDecimal)
            {
                note = releaseItem.Price;
            }
            else
            {
                price = Convert.ToDecimal(releaseItem.Price.Replace("$", string.Empty));
            }

            connection.Execute(
                "InsertReleaseItem",
                new
                {
                    releaseItem.ReleasedOn,
                    releaseItem.Category,
                    releaseItem.Publisher,
                    releaseItem.ItemCode,
                    releaseItem.Title,
                    price,
                    note
                },
                commandType: CommandType.StoredProcedure);
        }
    }
}

It would also be a good idea to remove these lines from the SaveRelease method as they are not related to saving and put them in the ReleaseItem class as properties with a getter only:

bool isDecimal = decimal.TryParse(releaseItem.Price.Replace("$", string.Empty), out price);

if (!isDecimal)
{
    note = releaseItem.Price;
}
else
{
   price = Convert.ToDecimal(releaseItem.Price.Replace("$", string.Empty));
}

Create a release parser that's specialization is to parse the lines only:

public class ReleaseParser
{
    const string PremierPublishers = "PREMIER PUBLISHERS";
    const string NewReleasesFor = "New Releases For";

    public static IEnumerable<ReleaseItem> Parse(IEnumerable<string> lines, IEnumerable<string> premierPublishers)
    {
        var releaseItem = new ReleaseItem
        {
            ReleasedOn = DateTime.Now
        };

        foreach (var line in lines)
        {
            if (line.Contains(NewReleasesFor))
            {
                releaseItem.ReleasedOn = DateTime.Parse(line.Substring(line.Length - 10));
            }
            else
            {
                if (!line.Any(p => p.ToString().Contains("\t")) & !premierPublishers.Any(p => p == line))
                {
                    releaseItem.Category = line.Trim();
                }
                else if (premierPublishers.Any(p => p == line))
                {
                    releaseItem.Publisher = line.Trim();
                }
                else
                {
                    string[] lineitem = line.Split('\t');

                    if (lineitem.Count() == 3)
                    {
                        releaseItem.ItemCode = lineitem[0].Trim();
                        releaseItem.Title = lineitem[1].Trim();
                        releaseItem.Price = lineitem[2].Trim();

                        if (releaseItem.Category != PremierPublishers & !premierPublishers.Any(p => p == line))
                        {
                            releaseItem.Publisher = null;
                        }

                        yield return releaseItem;
                    }
                }
            }
        }
    }
}

Refactor the main method and get the connection string from the connections-string section and not the app-settings. Create the downloader, the parser, the repository and let them to their job.

private const string RemoteReleaseFile = "http://www.previewsworld.com/shipping/newreleases.txt";

public static void Main(string[] args)
{
    var connectionStringName = "foo";
    var connectionStringSetting =
                ConfigurationManager
                .ConnectionStrings
                .Cast<ConnectionStringSettings>()
                .SingleOrDefault(x => x.Name.Equals(connectionStringName , StringComparison.OrdinalIgnoreCase));

    if(connectionStringSetting == null || 
        string.IsNullOrEmpty(connectionStringSetting.ConnectionString))
    {
        throw new Exception($"Connection string '{connectionStringName}' not found");
    }

    var releaseDownloader = new ReleaseDownloader();
    var releaseRepository = new ReleaseRepository(connectionStringSetting.ConnectionString);

    var releases = releaseDownloader.GetRelease(RemoteReleaseFile);
    var premierPublishers = releaseRepository.PremierPublishers();

    foreach (var releaseItem in ReleaseParser.Parse(releases, premierPublishers))
    {
        releaseRepository.SaveRelease(releaseItem);
    }
}

Now you can test and work with each module separately without affecting the others ;-)

Answer 2

Program

• If you use an else if instead of an else you can reduce the indentation of the code by one level.

• by using short circuit evaluation you will save some time. Replace & with &&. Using && means that the second expression is evaluated only if the first returns true, whereas using & will evaluate always the second regardless ovf the result of the first expression.

• if you switch the order of if statements to first check premierPublishers.Any(p => p == line) you won't do this that often.

• the string[] has a Length property to tell you how many items are contained in the array. Using IEnumerable<T>.Count() involves some casts which aren't neccesary.

• line.Substring(line.Length - 10) why 10 ? Why not 11 or 16. Extract this magic number to a meaningful constant so it is easier to understand.

• you have releaseItem using camel case casing but you have lineitem which is very hard to read. You should always be consistent in the style you use. In addition, lineitem reffers to a collection of strings, so using the plural form lineItems is more natural.

• the DateTime.Parse can become dangerous if the returned SubString() won't contain a regular date expression. You should consider to use DateTime.TryParse() instead.

Implementing the mentioned points lead to

const string PremierPublishers = "PREMIER PUBLISHERS";
const string NewReleasesFor = "New Releases For";

ReleaseItem releaseItem = new ReleaseItem();
List<string> premierPublishers = Release.PremierPublishers();
DateTime releaseDate = new DateTime();

const int SomeConstForSubstring = 10;

foreach (var line in Release.GetRelease())
{
    if (line.Contains(NewReleasesFor))
    {
        releaseDate = DateTime.Parse(line.Substring(line.Length - SomeConstForSubstring));
    }
    else if (premierPublishers.Any(p => p == line))
    {
        releaseItem.Publisher = line.Trim();
    }
    else if (!line.Any(p => p.ToString().Contains("\t")))
    {
        releaseItem.Category = line.Trim();
    }
    else
    {
        string[] lineIems = line.Split('\t');

        if (lineIems.Length == 3)
        {
            releaseItem.ItemCode = lineIems[0].Trim();
            releaseItem.Title = lineIems[1].Trim();
            releaseItem.Price = lineIems[2].Trim();

            if (releaseItem.Category != PremierPublishers)
            {
                releaseItem.Publisher = null;
            }

            Release.WriteRelease(releaseDate, releaseItem);
        }
    }
}

Answer 3

So, above other answers.

1. C# has a cool feature - yield keyword. Your case looks perfect to use its benefits:

   public static IEnumerable<string> GetRelease()
   {
       using (WebClient webClient = new WebClient())
       {
           Stream stream = webClient.OpenRead(RemoteReleaseFile);
           using (StreamReader streamReader = new StreamReader(stream))
           {
               while (!streamReader.EndOfStream)
               {
                   var line = streamReader.ReadLine();
                   if (!string.IsNullOrWhiteSpace(line))
                   {
                       yield return line;
                   }
               }
           }
       }
   }
   

2. Consider using IEnumerable instead of List whenever suitable. Here

   public static List<string> PremierPublishers()
   {
       using (IDbConnection connection = new SqlConnection(ConfigurationManager.AppSettings["Previews"]))
       {
           return connection.Query<string>("Select PremierPublisher From PremierPublishers").ToList();
       }
   }
   

you do not really need a list as you do not use its features (e.g. indexing) later and need it just once in your code. So you can simplify and perhaps accelerate it:

    public static IEnumerable<string> PremierPublishers()
    {
        using (IDbConnection connection = new SqlConnection(ConfigurationManager.AppSettings["Previews"]))
        {
            return connection.Query<string>("Select PremierPublisher From PremierPublishers");
        }
    }

After changing the type of premierPublishers in the caller everything remains working.

3. Actually, this line looks weird to me:

   var premierPublishers = Release.PremierPublishers();
   

PremierPublishers() appears to be a method, thus it should start with a verb in its name, smth like GetPremierPublishers(). Otherwise it looks like a new object instantiation.

4. Here

   if (lineitem.Count() == 3)
   

3 is a kind of maaagic. Better extract it to a private static readonly int naming it smth like NumberOfPartsInComicsLine. Yes, the name is not the best but this is really what you hope to get here.

5. I would also do something with the algorithm itself. Look at this logic:

   if (line.Contains(NewReleasesFor))
   {
       releaseDate = DateTime.Parse(line.Substring(line.Length - 10));
   }
   

The thing is that effectively this condition will be true just once, when parsing the first line. However, it will be tested for each of the zillion lines you get. So perhaps you should take and handle the first line separately in the beginning and then iterate through all the remaining lines.