9
\$\begingroup\$

I wrote a simple console application that reads contents from a CSV file, and using that input, checks a database table to see if the contents are within it. If there is a match, then the program will output the name of the file that the input is included in to a text file.

I feel like I should break this application up into more compartmentalized functions, such as separating the DB connection from DB read.

Any input for this greatly appreciated!

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Data.SqlClient;
using System.Windows.Forms;
using System.IO;
using Microsoft.VisualBasic.FileIO;

namespace MLMSpellingReader
{
    class SpellingReader
    {
        static void Main(string[] args)
        {
            SpellingReader.readCSV();
            MessageBox.Show("Evaluation is complete, press OK to continue");
        }

        public static void readCSV(){

            string filePath = Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments);
            string outputFile = filePath + @"\outputFile.txt";
            string queryString = null;
            using (TextFieldParser parser = new TextFieldParser(@"c:\users\myuser\Documents\spellcheck.csv"))
            {
                parser.TextFieldType = FieldType.Delimited;
                parser.SetDelimiters(",");                
                while(!parser.EndOfData)
                {
                    string[] fields = parser.ReadFields();
                    foreach(string field in fields)
                    {
                        string eval = field;
                        queryString = "select Name from myTable where Logic like '%" + eval + "%' and active = '1'";
                        SpellingReader.connectToDB(outputFile, queryString, eval);
                    }                    
                }             
            }
        }

        public static void connectToDB(string outputFile, string queryString, string eval)
        {
            string connectionString = "Server=myServer;Database=myDBTable;Integrated Security=True;";
            string checkSpellingQuery = queryString;
            SqlConnection conn;            
            conn = new SqlConnection(connectionString);
            try
            {
                SqlCommand command = new SqlCommand(checkSpellingQuery, conn);
                conn.Open();
                SqlDataReader reader = command.ExecuteReader();
                using(StreamWriter file = new StreamWriter(outputFile, true))
                {
                    while(reader.Read())
                    {
                        file.WriteLine("The misspelling " + eval + " was found in: " + reader["Name"]);
                    }
                }
                reader.Close();
                conn.Close();
            }
            catch(Exception ex)
            {
                MessageBox.Show("Cannot open connection: " + ex.ToString());
            }
        }      

    }
}
\$\endgroup\$
14
\$\begingroup\$

readCSV

This function has no reason to be public, make it private. Also its name is misleading, it's not just reading CSV file but also connecting to DB and saving output...

string outputFile = filePath + @"\outputFile.txt";

Do not blindly concatenate paths, use Path.Combine(). Also filename should be moved to configuration (or if immutable to a const member).

string queryString = null;

You do not need to declare a variable at the top of your method, it's usually better to declare as late as possible (best if together with its initialization).

using (TextFieldParser parser = new TextFieldParser(@"c:\users\myuser\Documents\spellcheck.csv"))

Do not hardcode paths. Never. Read it from configuration, command line or asks for them. Here you do not even use Environment.GetFolderPath()!

string eval = field;

It serves no purpose, just drop it.

queryString = "select Name from myTable where Logic like '%" + eval + "%' and active = '1'";

Do not ever ever build SQL commands like that. Use parameters, code may be broken because of deliberate SQL injection attacks or because of unescaped input data.

SpellingReader.connectToDB(outputFile, queryString, eval);

You're connecting to DB multiple times, it's probably a waste: you'd better keep connection open until you processed all input lines.

connectToDB

public static void connectToDB(string outputFile, string queryString, string eval)

This function has no reason to be public, make it private. Also here the name is misleading, you're not just connecting to DB but also querying data and writing output...

eval parameter has not a truly meaningful name, you'd better pick something more close to your domain.

SqlConnection conn;
conn = new SqlConnection(connectionString);

There is no reason to split this code in two lines, moreover you do not dispose connection in case of errors (use using).

SqlDataReader reader = command.ExecuteReader();

You do not dispose this object in case of errors.

using(StreamWriter file...

Some LINQ may make your code shorter and more readable (if you do not have a huge number of lines to write).

Minor issues

In C# usually functions are PascalCase, also you should be consistent with spacing (if ( or while(?)

What to do?

Let's first make clear what your application is doing:

  • Read all words from an input file (each word is separated with comma and each line contains multiple words).
  • For each word check if there is a record in the database where Logic column contains given text.
  • Print Name column of those records.

Let's write some code (let me assume the number of input words is reasonable - it fits in memory):

static IEnumerable<string> ReadAllWords(string path)
{
    var words = new List<string>();
    using (var parser = new TextFieldParser(path))
    {
        parser.TextFieldType = FieldType.Delimited;
        parser.SetDelimiters(",");                

        while (!parser.EndOfData)
            words.AddRange(parser.ReadFields());
    }
}

Now let's query DB:

static IEnumerable<string> FindRecordsByWord(SqlConnection connection, string word)
{
    using (var command = connection.CreateCommand())
    {
        command.CommandText = "SELECT Name FROM myTable WHERE Logic LIKE @word AND active = '1'";
        command.Parameters.AddWithValue("@word","%" + word+ "%");

        using (var reader = command.ExecuteReader())
            yield return reader["Name"];
    }
}

Let's put some pieces together:

var mispelledWords = ReadAllWords(InputFile); // From configuration or cmdline
using (var conn = new SqlConnection("MyConnectionString")) // From configuration!!!
{
    var recordsWithMispelledNames = mispelledWords
        .SelectMany(w => FindRecordsByWord(conn, w).Select(n => $"Found {w} in {n}"));

    File.WriteAllLines(OutputFile, recordsWithMispelledNames);
}

Note that you may now refactor out some simple logic:

static IEnumerable<string> FindRecordsByWordList(IEnumerable<string> words)
{
    using (var conn = new SqlConnection("MyConnectionString")) // From configuration!!!
    {
        return mispelledWords
            .SelectMany(w => FindRecordsByWord(conn, w).Select(n => $"Found {w} in {n}"));
    }
}

Your Main() function will then be as simple as:

File.WriteAllLines(OutputFile,
    FindRecordsByWordList(ReadAllWords(InputFile)));

Few more notes:

  • There is space for more abstractions (input type, output type) but I'd keep code as simple as possible until you actually need it.
  • You should specify input/output file encoding (don't really rely on default settings and magic guesses, you'll be surprised).
  • There are debates around AddWithValue() usage, decide if you like it or not.
  • There is not any error checking (see also Know when to retry or fail when calling SQL Server from C#?)
\$\endgroup\$
  • \$\begingroup\$ Please add 4 spaces in front of your code so that it appears properly formatted \$\endgroup\$ – Caridorc Sep 1 '16 at 12:53
  • \$\begingroup\$ @Caridorc it's OP code, I'm always in doubt if I should format it as citation with > or as code... \$\endgroup\$ – Adriano Repetti Sep 1 '16 at 12:55
  • \$\begingroup\$ @Pete more to come, I'd also wait little bit more before accepting an answer (thank you, anyway) because someone else may want to post another and better answer (code review isn't as fast as few paragraphs) \$\endgroup\$ – Adriano Repetti Sep 1 '16 at 12:57
2
\$\begingroup\$
queryString = "select ... where Logic like '%" + eval + "%' and active = '1'";

active should be a bit column in DB. Therefore you should be checking for a value without '' single quotes around it, to avoid implicitly converting from a string/varchar.

\$\endgroup\$

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.