5
\$\begingroup\$

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; }
    }
}
\$\endgroup\$
3
\$\begingroup\$

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 ;-)

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Why the Cast, SingleOrDefault and StringComparison when this works just as well: ConfigurationManager.ConnectionStrings["Previews"].ConnectionString \$\endgroup\$ – Steve Jun 28 '16 at 11:53
  • \$\begingroup\$ @StevenMpls Oh, you're right. I've fixed it. It was an incomplete copy&paste from one of my projects. I do this because otherwise it will throw a null-reference-exception if the connection string is not found. When I check it myself I can at least put the name of the connection string in the message. \$\endgroup\$ – t3chb0t Jun 28 '16 at 12:02
  • \$\begingroup\$ @t3chb0tI could be totally off base here but this strikes me as overly defensive programming. Why not also check to see if the ConnectionStrings section exists (it isn't required), while you're at it? Don't get me wrong, I am grateful for the help :) \$\endgroup\$ – Steve Jun 28 '16 at 12:16
  • \$\begingroup\$ @StevenMpls I think it will be allways initialized. I admit, it might be too much here but as I always use this snippet I totaly forgot how to get it in the short way so this was the only thing I had in mind at the time of writing the review ;-) \$\endgroup\$ – t3chb0t Jun 28 '16 at 12:20
2
\$\begingroup\$

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);
        }
    }
}
|improve this answer|||||
\$\endgroup\$
0
\$\begingroup\$

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.

  1. 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.

  1. 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.

  1. 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.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ 1. yield return does not appear to work in this context, or not at least with additional changes. \$\endgroup\$ – Steve Jun 28 '16 at 12:05
  • \$\begingroup\$ Do you mean you cannot iterate through lines? Can you compile it? \$\endgroup\$ – psfinaki Jun 28 '16 at 12:25
  • \$\begingroup\$ It does compile but then what was already written to parse the results no longer returns what it used to and so Category and Publisher have no values. \$\endgroup\$ – Steve Jun 28 '16 at 12:34
  • \$\begingroup\$ Well, it is hard to easily recommend a fix, because I do not know exactly how you have applied other solutions. But generally remember not to save the results from GetReleases anywhere, only iterate though them. \$\endgroup\$ – psfinaki Jun 28 '16 at 12:55

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.