4
\$\begingroup\$

I'm new to C#, so I'm eager for any and all criticism/advice.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace FileTransfer
{
    class Program
    {
        private static string targetPath;
        public static bool running;

        static void Main(string[] args)
        {
            var function = new Functions();
            running = true;
            int num = 0;

            while (running)
            {

                Console.WriteLine("These files in Customer Orders are new:");
                ModifiedFiles newFiles = new ModifiedFiles();
                num = 0;

                foreach (var file in newFiles.modified())
                {
                    Console.WriteLine("\""+file+"\"");
                    num++;
                }

                Console.WriteLine("Transfer {0} file(s) to Home Office? y/n", num);
                string answer = Console.ReadLine();

                if (answer == "y")
                {
                    targetPath = @"C:\Users\Student\Desktop\Home Office";

                    num = 0;
                    foreach (var file in newFiles.modified())
                    {
                        File.Copy(file.FullName, Path.Combine(targetPath, file.Name), true);
                        num++;
                    }

                    Console.WriteLine("{0} file(s) transfered.", num);
                    function.exit();
                }

                else if (answer == "n")
                {
                    function.exit();
                }

                else
                {
                    function.wrong();
                }
            }
        }
    }   
    class ModifiedFiles
    {
        public string sourceDir;
        public IEnumerable<FileInfo> modified()
        {
            sourceDir = @"C:\Users\Student\Desktop\Customer Orders";    
            var directory = new DirectoryInfo(sourceDir);
            DateTime from_date = DateTime.Now.AddDays(-1);
            DateTime to_date = DateTime.Now;
            var files = directory.GetFiles()
              .Where(file => file.LastWriteTime >= from_date && file.LastWriteTime <= to_date);
            return files.ToList();
        }
    }
    class Functions
    { 
        public void wrong() { Console.WriteLine("---Input must be 'y' or 'n'."); }

        public void exit()
        {
            Console.WriteLine("Would you like to exit the program? y/n");
            string answerExit = Console.ReadLine();
            if (answerExit == "y")
            {
                Console.WriteLine("See yah!");
                Environment.Exit(0);
            }
            else if (answerExit == "n")
            {
                // Loop back to beginning
            }
            else
            {
                this.wrong();
            }

        }               
    }
}
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

A few notes, not in order of importance, just as I see them. Keep in mind this is just my opinions but I think they're important especially since you're starting out. You can form bad habits later :)

  • Your classes and methods should always explicitly define their level of accessebility. e.g. class Functions should be public class Functions
  • Always check if a file or directory exists before opening: var directory = new DirectoryInfo(sourceDir); should be:

    DirectoryInfo directory;
    if (Directory.Exists(sourceDir))
        directory = new DirectoryInfo(sourceDir);
    
  • User input should always be compare in one case for example answerExit == "y" should be answerExit.ToLower() == "y" Unless you want to make response case sensitive.

  • System directories should not be explicitly entered. For example : "C:\Users\Student\Desktop\Customer Orders" should be Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.Desktop),"Customer Orders");

  • Your Functions class is superfluous. Those methods directly relate to activities in Main, they can't be used separately or anything else and should definitely be static.

  • Your method modified() return an IEnumerable and I think that this line also return an IEnumerable

    var files = directory.GetFiles()
          .Where(file => file.LastWriteTime >= from_date &&         
          file.LastWriteTime <= to_date);` So why return a .ToList()?
    

I think that's a good start.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Tipp: you can format code in lists by identing it with 8 spaces ;-) instead of just with 4 \$\endgroup\$
    – t3chb0t
    Jun 12, 2016 at 19:49
  • 1
    \$\begingroup\$ answerExit.Equals("y", StringComparison.OrdinalIgnoreCase) would be even better since it saves the extra string allocation - small in this case admittedly.. \$\endgroup\$ Jun 13, 2016 at 12:13
2
\$\begingroup\$

Hard coded paths

Try to avoid putting hard coded paths into your source code. It means that if you want to change your source or destination paths you have to recompile your application. Unless you really want this level of commitment to the paths, then a better approach is either to take the paths as command line arguments, or to put them into the app.config file. For example Config:

<appSettings>
    <add key="SourcePath" value="C:\Users\Student\Desktop\Customer Orders"/>
    <add key="TargetPath" value="C:\Users\Student\Desktop\Home Office"/>
</appSettings>

Then usage:

string sourcePath = ConfigurationManager.AppSettings["SourcePath"];
string targetPath = ConfigurationManager.AppSettings["TargetPath"];

Class Naming

Functions is a terrible name for a class. The class name should express what it is you expect the class to be responsible for. Functions suggests a class that has no actual responsibility but is simply a bucket for functionality.

User Input

You have several places where you're getting input from the user, with y/n questions. If you encapsulate this in a single method you'll be able to reuse the functionality whenever you need it. So for example, you might end up with a class like this (based on your Functions class):

class UserInterogator
{
    public enum Decision {  No, Yes}

    static public Decision GetDecisionFromUser(string prompt)
    {
        while (true)
        {
            Console.WriteLine(prompt);

            string answer = Console.ReadLine();
            if (answer == "y")
            {
                return Decision.Yes;
            }
            else if (answer == "n")
            {
                return Decision.No;
            }

            Console.WriteLine("---Input must be 'y' or 'n'.");
        }
    }
}

Centralise Responsibility

At the moment, your Program is responsible for knowing the target path, but your ModifiedFiles class is responsible for knowing the source path. Since they are related, it's better to have the paths referenced from the same area of code. If you push the knowledge of the source path into your Program class and pass it in then it will be possible to reuse the ModifiedFiles functionality in other classes in future. This leaves the ModifiedFiles class looking like this:

class ModifiedFileFinder
{
    static public IEnumerable<FileInfo> GetFilesModifiedInLast24Hours(string path)
    {
        var directory = new DirectoryInfo(path);
        DateTime from_date = DateTime.Now.AddDays(-1);
        DateTime to_date = DateTime.Now;
        var files = directory.GetFiles()
          .Where(file => file.LastWriteTime >= from_date && file.LastWriteTime <= to_date);
        return files.ToList();
    }
}

Timing

Each time you use your ModifiedFiles class, you go back to the filesystem. You might want to do this, but it means that there is a possibility that the files shown to the user won't be the same ones that were copied (if new files are modified). You could cache the retrieved list of files instead so that the same files are shown to the user as are copied.

Putting it together

Using the above suggestions, your Program class becomes:

class Program
{
    static void Main(string[] args)
    {
        string sourcePath = ConfigurationManager.AppSettings["SourcePath"];
        string targetPath = ConfigurationManager.AppSettings["TargetPath"];

        do
        {
            Console.WriteLine("These files in Customer Orders are new:");
            var modifiedFiles = ModifiedFileFinder.GetFilesModifiedInLast24Hours(sourcePath);
            var num = 0;

            foreach (var file in modifiedFiles)
            {
                Console.WriteLine("\"" + file + "\"");
                num++;
            }

            if (UserInterogator.GetDecisionFromUser($"Transfer {num} file(s) to Home Office? y/n") == UserInterogator.Decision.Yes)
            {
                foreach (var file in modifiedFiles)
                {
                    File.Copy(file.FullName, Path.Combine(targetPath, file.Name), true);
                }

                Console.WriteLine("{0} file(s) transfered.", num);
            }
        }
        while (UserInterogator.GetDecisionFromUser("Would you like to exit the program? y/n") == UserInterogator.Decision.No);

        Console.WriteLine("See yah!");
    }
}

Error Handling

You don't have any error handling in your application. What do you want it to do if it fails to copy one of the files (for example because of a sharing violation, network error, permissions error)? Should it continue, ignoring the file, ask the user, abort? At the moment, the program will just crash.

\$\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.