Problem
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:usersmyuserDocumentsspellcheck.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());
}
}
}
}
Solution
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:usersmyuserDocumentsspellcheck.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#?)
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
.