HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Lattitude document organizer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
lattitudeorganizerdocument

Problem

I'm not familiar with threads but I am not against them either. A bit of background the original program would have taken about 2 years to run. I had modified it and knocked down that time to around 40 days.

Can I make it even faster by using threads or updating my code?

The reason it is 40 days is because it has to navigate through a directory of about 6 million files. That amount of files slows this program down.

```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Data.SqlClient;
using System.IO;
using System.Data;
using System.Threading;

namespace LattitudeDocumentationOrganizer
{
class Program
{
static string connString = LattitudeDocumentationOrganizer.Properties.Settings.Default.ConnectionString;

static void Main(string[] args)
{
string fileName;
Guid documentID;
string location;
DateTime createdDate;
bool runLoop = true;
int x = 0;
string msg;
int fileCount = 0;
int errorCount = 0;
int number = 0;

while (runLoop)
{
Console.Clear();
// reset variables
fileName = "";
documentID = Guid.Empty;
location = "";
using (SqlConnection connection = new SqlConnection(connString))
{
connection.Open();
// Run through
string sql = @"select top 1 d.UID, d.CreatedDate, d.Location, m.number from master m
inner join documentation_attachments da
on m.number = da.accountid
inner join documentation d
on da.documentid = d.uid
where m.qlevel in (998,999)
and d.lo

Solution

I see a number of things in this code. I'll address all of them, then get to multi-threading. Keep in mind, I'm writing these down as I make changes in the code, so I might suggest something, then change that suggestion.

  • I like your variable names. Very descriptive and I understand what information they are holding. The only exception is connString, I think it should be make readonly and changed to 'ConnString' indicating a private static readonly variable.



  • The declarations of the variables used in the function should be moved closer to where they are being used. This does a couple of things: makes the declaration a little more clear to any other developers, and it ensures the value hasn't been unintentionally altered between declaration and usage.



  • Learn about var. I find that using it really cleans up extra code that just causes clutter.



  • I would make the declaration of sql const, and move it to be a class variable. At the same time, it should be renamed SelectFileInformationSql



  • Good job with the using using.



  • The runLoop variable is set in places it is never needed because of the break statements just below it. You can remove this variable, and replace the main loop to a while(true)



  • The x variable is unused, remove it.



  • The variable rootDir should be declared const, moved to a class variable and renamed RootDir



  • The second declaration of sql should be declared const, moved to a class varible and renamed UpdateDocumentLocationSql



  • The thrid declaration of sql should be declared const, moved to a class varible and renamed LogIssueSql



  • I would like to see document moved out into its own class. This way you can do things like FilenameAndPath = Path.Combine(Location, Filename) only once, instead of 2-3 times in the original code. It also packages all the information needed for processing that file into one nice package.



  • I find the msg variable to be unneeded. You can just put the string right in the function call.



I will get to multithreading when I found out what version of .Net you are using :)

Context

StackExchange Code Review Q#32389, answer score: 9

Revisions (0)

No revisions yet.