6
\$\begingroup\$

I've been writing a program to synchronize two or more folders. I've read that methods should be kept short. While most of mine are, one method is ~50 lines:

public void ModSync(List<mySyncFol> dirf, string logpath)
        {   // Sync Everything to most recent modified
            int a = 0, ac, b=-1, li = 0; // a = current folder marker, ac = current file placeholder, b = loop condition, li = Log counter
            bool isNew, isMissing, delete;
            DateTime mdate; // Last modified date
            string cfile; // Current file name
            var i = new List<int>(new int[dirf.Count]);  // Counter list
            var logf = ImportLog(logpath); // Log file
            DateTime logdate = File.GetLastWriteTime(logpath);

            while (b < 0) //'Continue until all lists are exhaused
            {
                for (int y = 0; y < dirf.Count; y++)
                {
                    if (string.Compare(dirf[y].FSOs[i[y]].RelativePath, dirf[a].FSOs[i[a]].RelativePath) < 0)  a = y;
                }
                cfile = dirf[a].FSOs[i[a]].RelativePath;
                while (string.Compare(logf.FSOs[li].RelativePath, dirf[a].FSOs[i[a]].RelativePath) < 0)  li++;
                isNew = isMissing = delete = false; 
                for (int y = 0; y < dirf.Count; y++)
                {// If file deleted and no file created and modified after, then delete.  Else use most recent modified, even if it was created before the delete
                    if (dirf[y].FSOs[i[y]].RelativePath == cfile)
                    {
                        if (dirf[y].FSOs[i[y]].DateModified > dirf[a].FSOs[i[a]].DateModified)  a = y;
                        if (dirf[y].FSOs[i[y]].DateCreated > logdate && dirf[y].FSOs[i[y]].DateCreated > logdate)  isNew = true;
                    }
                    else isMissing = true;
                }
                if (isMissing && logf.FSOs[li].RelativePath == cfile && !isNew) delete = true;
                mdate = dirf[a].FSOs[i[a]].DateModified;
                ac = i[a];

                for (int y = 0; y < dirf.Count; y++) //delete any outdated files and copy most recent file to all folders
                {  // Copy and replace, delete, or leave.
                    if (!delete && y != a && (dirf[y].FSOs[i[y]].RelativePath != cfile || dirf[y].FSOs[i[y]].DateModified < mdate))  dirf[a].FSOs[ac].CopyTo(dirf[y].Path + dirf[a].FSOs[ac].FullName.Substring(dirf[a].Path.Length));
                    if (dirf[y].FSOs[i[y]].RelativePath == cfile)
                    {
                        if (delete)  dirf[y].FSOs[i[y]].Delete();
                        i[y]++; // Incrementing has to happen last or it will change the file being compared
                    }
                }
                b = 0;
                for (int y = 0; y < dirf.Count; y++) //Cap counters at upperbound and determine whether another cycle is needed (go until at least one counter is >UB and none are <UB). 
                {
                    if (i[y] < dirf[y].FSOs.Count)  b = -dirf.Count;
                    else if (i[y] > dirf[y].FSOs.Count)
                    {
                        i[y] = dirf[y].FSOs.Count;
                        b++;
                    }
                }
            }
        }

I'll include the rest of my code below for context. Here are my two questions:

  1. Should the above method be split into multiple methods, or is it good as is. If it should, then how?

  2. Are there other improvements I should make to improve my code's efficiency, performance, logical structure, extendability, or congruence with best practices?

Classes:

public class myFSModInfo
    {
        public bool isFile;
        public string BaseDirectory;
        public string RelativePath;
        public string FullName;
        public DateTime DateModified;
        public DateTime DateCreated;
        public myFSModInfo(FileSystemInfo dir, string basedir)
        {
            isFile = dir is FileInfo; 
            BaseDirectory = basedir;
            RelativePath = dir.FullName.Substring(basedir.Length + (basedir.Last() == '\\' ? 0 : 1));
            FullName = dir.FullName;
            DateModified = dir.LastWriteTime;
            DateCreated = dir.CreationTime;
        }
        private myFSModInfo() { }
        /// <summary>
        /// Copies a FileInfo or DirectoryInfo object to the specified path, creating folders and overwriting if necessary.
        /// </summary>
        /// <param name="path"></param>
        public void CopyTo(string path)
        {
            if (isFile)
            {
                Directory.CreateDirectory(path.Substring(0, path.LastIndexOf("\\")));
                File.Copy(FullName,path, true);
            }
            else Directory.CreateDirectory(path);
        }
        public void Delete() 
        {
            if (isFile)  File.Delete(FullName);
            else Directory.Delete(FullName);
        }
    }

    public class mySyncFol
    {
        public string Path;
        public List<myFSModInfo> FSOs = new List<myFSModInfo>();
        public mySyncFol() { }
        public mySyncFol(string Path) 
        {
            this.Path = Path;
            AddRange(new DirectoryInfo(Path).EnumerateFileSystemInfos("*",SearchOption.AllDirectories).OrderBy(x => x.FullName));
        }
        public mySyncFol(string Path, List<myFSModInfo> FSOs) 
        {
            this.Path = Path;
            this.FSOs = FSOs;
        }
        public void Add(FileSystemInfo dir)
        {
            FSOs.Add(new myFSModInfo(dir, Path));
        }
        public void Add(string dirpath)
        {
            if ((File.GetAttributes(dirpath) & FileAttributes.Directory) == FileAttributes.Directory)  Add(new DirectoryInfo(dirpath));
            else Add(new FileInfo(dirpath));
        }
        public void AddRange(IEnumerable<FileSystemInfo> dirs)
        {
            FSOs.AddRange(dirs.Select(x => new myFSModInfo(x, Path)));
        }
        public void AddRange(IEnumerable<string> dirpaths)
        { // May be unnecessary
            FSOs.AddRange(dirpaths.Select(x=>((File.GetAttributes(x) & FileAttributes.Directory) == FileAttributes.Directory)? new myFSModInfo(new DirectoryInfo(x),Path) : new myFSModInfo(new FileInfo(x),Path)));
        }
    }

Methods:

private void SyncBtn_Click(object sender, RoutedEventArgs e)
        {
            // Final Version
            Stopwatch t = Stopwatch.StartNew();
            FolderList.Items.Add(@"C:\Users\e017200\Downloads\Test\plist");
            FolderList.Items.Add(@"C:\Users\e017200\Downloads\Test\plist2");
            LogFileBox.Text = @"C:\Users\e017200\Downloads\Test\Logs\plist - 28-16-2015 - 04_28_38.xml";
            var dirs = FolderList.Items.Cast<String>().ToList();
            var dirf = new List<mySyncFol>(dirs.Select(x => new mySyncFol(x))) ;

            if (FirstFolderRBtn.IsChecked == true)  OneWaySync(dirf);
            else ModSync(dirf,LogFileBox.Text);
            SaveLog("C:\\Users\\e017200\\Downloads\\Test\\Logs\\" + dirs[0].Substring(dirs[0].LastIndexOf('\\')+1) + " - " + DateTime.Now.ToString("mm-dd-yyyy - hh_mm_ss") + ".xml", dirs[0]);
            Debug.Print(Convert.ToString(t.Elapsed));
        }

public void OneWaySync(List<mySyncFol> dirf)
        {   // Given an array of directories frin GetFSInfo, will make all directories match the first one.
            var i = new List<int>(new int[dirf.Count]);
            bool extra = false;

            while (i[0] < dirf[0].FSOs.Count)
            {
                extra = false;
                for (int y = 1; y < dirf.Count; y++)
                {

                    if (i[y] < dirf[y].FSOs.Count && string.Compare(dirf[0].FSOs[i[0]].RelativePath, dirf[y].FSOs[i[y]].RelativePath) > 0)
                    {
                        dirf[y].FSOs[i[y]].Delete();
                        extra = true;
                        i[y]++;
                    }
                    else if (i[y] >= dirf[y].FSOs.Count || dirf[0].FSOs[i[0]].RelativePath != dirf[y].FSOs[i[y]].RelativePath)  dirf[0].FSOs[i[0]].CopyTo(dirf[y].Path + dirf[0].FSOs[i[0]].FullName.Substring(dirf[0].Path.Length));
                    else i[y]++;
                }
                if (!extra)  i[0]++;
            }
        }

        public void SaveLog(string savepath, string dirpath)
        {   // Gets all files from a directory and saves the info to a log file
            var mySerializer = new XmlSerializer(typeof(mySyncFol));
            var myWriter = new StreamWriter(savepath);
            mySerializer.Serialize(myWriter, new mySyncFol(dirpath));
            myWriter.Close();
        }

        public mySyncFol ImportLog(string logpath)
        {   // Reads a log file into a myFSModInfo array
            XmlSerializer mySerializer = new XmlSerializer(typeof(mySyncFol));
            FileStream myFileStream = new FileStream(logpath, FileMode.Open);
            var log = (mySyncFol)mySerializer.Deserialize(myFileStream);
            myFileStream.Close();
            return log;
        }

Edit: Here is my code with changes. Will update as I make changes.

Classes:

public class MyFSInfo
    {
        public bool isFile { get; set; }
        public string BaseDirectory { get; set; }
        public string RelativePath{ get; set; }
        public string FullName{ get; set; }
        public DateTime DateModified{ get; set; }
        public DateTime DateCreated{ get; set; }
        public MyFSInfo(FileSystemInfo dir, string basedir)
        {
            isFile = dir is FileInfo; 
            BaseDirectory = basedir;
            RelativePath = dir.FullName.Substring(basedir.Length + (basedir.Last() == '\\' ? 0 : 1));
            FullName = dir.FullName;
            DateModified = dir.LastWriteTime;
            DateCreated = dir.CreationTime;
        }
        private MyFSInfo() { }
        /// <summary>
        /// Copies a FileInfo or DirectoryInfo object to the specified path, creating folders and overwriting if necessary.
        /// </summary>
        /// <param name="path"></param>
        public void CopyTo(string path)
        {
            if (isFile)
            {
                Directory.CreateDirectory(path.Substring(0, path.LastIndexOf("\\")));
                File.Copy(FullName,path, true);
            }
            else Directory.CreateDirectory(path);
        }
        public void Delete() 
        {
            if (isFile)  File.Delete(FullName);
            else Directory.Delete(FullName);
        }
    }

    public class MySyncFol
    {
        public string Path{ get; set; }
        public List<MyFSInfo> FSOs { get; set; }
        public MySyncFol() { }
        public MySyncFol(string Path) 
        {
            this.Path = Path;
            FSOs = new List<MyFSInfo>();
            AddRange(new DirectoryInfo(Path).EnumerateFileSystemInfos("*",SearchOption.AllDirectories).OrderBy(x => x.FullName));
        }
        public MySyncFol(string Path, List<MyFSInfo> FSOs) 
        {
            this.Path = Path;
            this.FSOs = FSOs;
        }
        public void Add(FileSystemInfo dir)
        {
            FSOs.Add(new MyFSInfo(dir, Path));
        }
        public void Add(string dirpath)
        {
            if ((File.GetAttributes(dirpath) & FileAttributes.Directory) == FileAttributes.Directory)  Add(new DirectoryInfo(dirpath));
            else Add(new FileInfo(dirpath));
        }
        public void AddRange(IEnumerable<FileSystemInfo> dirs)
        {
            FSOs.AddRange(dirs.Select(x => new MyFSInfo(x, Path)));
        }
        public void AddRange(IEnumerable<string> dirpaths)
        { // May be unnecessary
            FSOs.AddRange(dirpaths.Select(x=>((File.GetAttributes(x) & FileAttributes.Directory) == FileAttributes.Directory)? new MyFSInfo(new DirectoryInfo(x),Path) : new MyFSInfo(new FileInfo(x),Path)));
        }
    }

Methods:

private void SyncBtn_Click(object sender, RoutedEventArgs e)
    {
        // Final Version
        Stopwatch t = Stopwatch.StartNew();
        FolderList.Items.Add(@"C:\Users\e017200\Downloads\Test\plist");
        FolderList.Items.Add(@"C:\Users\e017200\Downloads\Test\plist2");
        LogFileBox.Text = @"C:\Users\e017200\Downloads\Test\Logs\plist - 28-16-2015 - 04_28_38.xml";
        var dirs = FolderList.Items.Cast<String>().ToList();
        var folders = new List<MySyncFol>(dirs.Select(x => new MySyncFol(x)));

        if (FirstFolderRBtn.IsChecked == true) OneWaySync(folders);
        else ModSync(folders, LogFileBox.Text);
        SaveLog(@"C:\Users\e017200\Downloads\Test\Logs\" + dirs[0].Substring(dirs[0].LastIndexOf('\\') + 1) + " - " + DateTime.Now.ToString("mm-dd-yyyy - hh_mm_ss") + ".xml", dirs[0]);
        Debug.Print(Convert.ToString(t.Elapsed));
    }

    public void OneWaySync(List<MySyncFol> folders)
    {   // Given an array of directories, will make all directories match the first one.
        var counter = new List<int>(new int[folders.Count]);
        bool foundExtraFile = false;

        while (counter[0] < folders[0].FSOs.Count)
        {
            foundExtraFile = false;
            for (int y = 1; y < folders.Count; y++)
            {

                if (counter[y] < folders[y].FSOs.Count && 
                    string.Compare(folders[0].FSOs[counter[0]].RelativePath, folders[y].FSOs[counter[y]].RelativePath) > 0)
                {
                    folders[y].FSOs[counter[y]].Delete();
                    foundExtraFile = true;
                    counter[y]++;
                }
                else if (counter[y] >= folders[y].FSOs.Count || 
                    folders[0].FSOs[counter[0]].RelativePath != folders[y].FSOs[counter[y]].RelativePath)
                {
                    folders[0].FSOs[counter[0]].CopyTo(folders[y].Path + folders[0].FSOs[counter[0]].FullName.Substring(folders[0].Path.Length));
                }
                else counter[y]++;
            }
            if (!foundExtraFile)  counter[0]++;
        }
    }

    public void ModSync(List<MySyncFol> folders, string logPath)
    {   // Sync everything to most recent modified
        int cFol = 0,
            cFil,
            finished = -1,
            logCounter = 0;
        bool isNew,
            isMissing,
            delete;
        DateTime modDate;
        string cFilename;
        var counter = new List<int>(new int[folders.Count]);
        var logf = ImportLog(logPath);
        DateTime logdate = File.GetLastWriteTime(logPath);

        while (finished < 0) //'Continue until all lists are exhaused
        {
            for (int y = 0; y < folders.Count; y++)
            {// Find the folder with the first (alphabetically) filename selected
                if (string.Compare(folders[y].FSOs[counter[y]].RelativePath, folders[cFol].FSOs[counter[cFol]].RelativePath) < 0) cFol = y;
            }
            cFilename = folders[cFol].FSOs[counter[cFol]].RelativePath;
            // Move logfile counter to match
            while (string.Compare(logf.FSOs[logCounter].RelativePath, folders[cFol].FSOs[counter[cFol]].RelativePath) < 0) logCounter++;
            isNew = isMissing = delete = false;
            for (int y = 0; y < folders.Count; y++)
            {// If file deleted and no file created and modified after, then delete.  Else use most recent modified, even if it was created before the delete
                if (folders[y].FSOs[counter[y]].RelativePath == cFilename)
                {
                    if (folders[y].FSOs[counter[y]].DateModified > folders[cFol].FSOs[counter[cFol]].DateModified) cFol = y;
                    if (folders[y].FSOs[counter[y]].DateCreated > logdate &&
                        folders[y].FSOs[counter[y]].DateModified > logdate) isNew = true;
                }
                else isMissing = true;
            }
            if (isMissing && logf.FSOs[logCounter].RelativePath == cFilename && !isNew) delete = true;
            modDate = folders[cFol].FSOs[counter[cFol]].DateModified;
            cFil = counter[cFol];

            for (int y = 0; y < folders.Count; y++) //delete any outdated files and copy most recent file to all folders
            {  // Copy and replace, delete, or leave.
                if (!delete && y != cFol &&
                    (folders[y].FSOs[counter[y]].RelativePath != cFilename || folders[y].FSOs[counter[y]].DateModified < modDate))
                {
                    folders[cFol].FSOs[cFil].CopyTo(folders[y].Path + folders[cFol].FSOs[cFil].FullName.Substring(folders[cFol].Path.Length));
                }
                if (folders[y].FSOs[counter[y]].RelativePath == cFilename)
                {
                    if (delete) folders[y].FSOs[counter[y]].Delete();
                    counter[y]++; // Incrementing has to happen last or it will change the file being compared
                }
            }
            finished = 0;
            for (int y = 0; y < folders.Count; y++)
            {// Cap counters at upperbound and determine whether another cycle is needed (go until at least one counter is >UB and none are <UB). 
                if (counter[y] < folders[y].FSOs.Count) finished = -folders.Count;
                else if (counter[y] > folders[y].FSOs.Count)
                {
                    counter[y] = folders[y].FSOs.Count;
                    finished++;
                }
            }
        }
    }

    public void SaveLog(string savePath, string dirPath)
    {   // Gets all files from a directory and saves the info to a log file
        using (var streamWriter = new StreamWriter(savePath))
        {
            var xmlSerializer = new XmlSerializer(typeof(MySyncFol));
            xmlSerializer.Serialize(streamWriter, new MySyncFol(dirPath));
        }
    }

    public MySyncFol ImportLog(string logPath)
    {   // Reads a log file into a MyFSInfo array
        using(var fileStream = new FileStream(logPath,FileMode.Open))
        {
            XmlSerializer xmlSerializer = new XmlSerializer(typeof(MySyncFol));
            return (MySyncFol)xmlSerializer.Deserialize(fileStream);
        }
    }
\$\endgroup\$
10
\$\begingroup\$

I don't have time for a full review at the moment but something filled me with rage jumped out at me...

Naming

The code should be readable - you have 2 ways of making that the case, one is by naming things correctly (the second is formatting).

So when I see things like this:

int a = 0, ac, b=-1, li = 0; // a = current folder marker, ac = current file placeholder, b = loop condition, li = Log counter
string cfile; // Current file name

Well, I won't go in to how it makes me feel.

If ac is the current file why don't you call it currentFile. Then when you're doing things with the current file, I won't have to scroll back to the top to figure out what ac is.

Try to name local variables in camelCase: logpath should be logPath.

What is dirf?!

Classes should be PascalCase like DateTime, FileInfo etc.

Stop abbreviating things because I promise that even you won't remember what they meant when you read them back in a week, let alone a year.

\$\endgroup\$
  • \$\begingroup\$ Apparently I don't have enough rep to upvote this, but thank you for your answer. I did some renaming already (the bool variables), but it could definitely use more. I'll go back through and try to pick more intuitive names. \$\endgroup\$ – Kalev Maricq Jun 18 '15 at 14:54
4
\$\begingroup\$

Don't do this:

int a = 0, ac, b=-1, li = 0;
bool isNew, isMissing, delete;

Sure, it's legal, but it's a coding style that isn't used in C#.


Properly capitalize compound words like logpath.


Comments should inform about the why, not the what. If you need a comment to explain what a certain field is, you've not named it properly:

int a = 0, ac, b=-1, li = 0; // a = current folder marker, ac = current file placeholder, b = loop condition, li = Log counter
DateTime mdate; // Last modified date
string cfile; // Current file name

There is no need for such abbreviations. Properly name your variables, and you don't need such comments. Moreover, proper names would make your code easier to follow. Sure, cfile and mdate aren't that hard to remember, but li? Or even worse, i?

var i = new List<int>(new int[dirf.Count]);  // Counter list

Your method starts of with initializing twelve (12!) variables. That's a lot to keep track of, and it suggests to me that your method does far too much.

Quite frankly, trying to figure out what ModSync does gives me a headache. Even with proper naming I doubt I could make much sense of things like dirf[a].FSOs[i[a]].RelativePath. Perhaps splitting this method into smaller methods would improve things, but considering that everything seems to be linked together I don't know if this is possible.

OneWaySync is barely comprehensible, even though it doesn't do that much.

Maybe these two methods should each be a separate class, which would make it easier to work with global fields and split up the logic into smaller methods? I'd give you an example, but reworking your code looks like a lot of work. Plus I don't have an example of what the current situation is and what is desired, which makes it really hard to attempt anything.


Do not use public fields, use properties with a getter/setter:

public bool isFile;
public string BaseDirectory;
public string RelativePath;
public string FullName;
public DateTime DateModified;
public DateTime DateCreated;

public string Path;
public List<myFSModInfo> FSOs = new List<myFSModInfo>();

Class names should be PascalCase, and shouldn't contain acronyms that aren't well-known:

public class myFSModInfo
public class mySyncFol

The same rule about acronyms also applies to properties and fields etc.:

public List<myFSModInfo> FSOs = new List<myFSModInfo>();

A lot of your code uses "single liners", e.g.

if (/*something*/) DoSomething();
while (/*something*/) DoSomething();

I would advise against this, except for simple cases like this:

if (/*something*/) continue;

And even in that case I would advise to use brackets and use "proper" formatting:

if (/*something*/)
{
  continue;
}

You have lines that are 200+ characters long. I don't like to do much vertical scrolling when reading code, and horizontal scrolling is a big NO altogether. I have a hard enough time figuring out what this does:

if (!delete 
    && y != a 
    && (dirf[y].FSOs[i[y]].RelativePath != cfile 
        || dirf[y].FSOs[i[y]].DateModified < mdate))  
{
    dirf[a].FSOs[ac].CopyTo(dirf[y].Path + dirf[a].FSOs[ac].FullName.Substring(dirf[a].Path.Length));
}

But when it is all on one line I just get lost:

if (!delete && y != a && (dirf[y].FSOs[i[y]].RelativePath != cfile || dirf[y].FSOs[i[y]].DateModified < mdate))  dirf[a].FSOs[ac].CopyTo(dirf[y].Path + dirf[a].FSOs[ac].FullName.Substring(dirf[a].Path.Length));

I'm baffled to see these hard-coded values:

FolderList.Items.Add(@"C:\Users\e017200\Downloads\Test\plist");
FolderList.Items.Add(@"C:\Users\e017200\Downloads\Test\plist2");
LogFileBox.Text = @"C:\Users\e017200\Downloads\Test\Logs\plist - 28-16-2015 - 04_28_38.xml";

Shouldn't these come from user input?


Don't do this, but let Path.Combine handle this, please:

"C:\\Users\\e017200\\Downloads\\Test\\Logs\\" + dirs[0].Substring(dirs[0].LastIndexOf('\\')+1) + " - " + DateTime.Now.ToString("mm-dd-yyyy - hh_mm_ss") + ".xml"

Also, use String.Format to construct the filename. And this really shouldn't be hardcoded: "C:\\Users\\e017200\\Downloads\\Test\\Logs\\".


Encapsulate classes that implement IDisposable in a using block (if/when appropriate):

var mySerializer = new XmlSerializer(typeof(mySyncFol));
var myWriter = new StreamWriter(savepath);
mySerializer.Serialize(myWriter, new mySyncFol(dirpath));
myWriter.Close();

Would become:

using (var streamWriter = new StreamWriter(savepath))
{
    var xmlSerializer = new XmlSerializer(typeof(mySyncFol));
    xmlSerializer.Serialize(streamWriter, new mySyncFol(dirpath));
}
\$\endgroup\$
  • \$\begingroup\$ Wow, there is a lot here. Thank you! I have some questions about your suggestions: 1. What should I use for counters? I thought "i" was pretty standard for that. 2. Should I prioritize making variable names self-explanatory or making lines shorter? 3. Is continuing a horizontal line on the next line a good solution? 4. What's the difference/advantage between public fields and properties with a get/set? 5. Comment: Hardcoded values are just for testing. 6. If I followed these, would ModSync be understandable or is it a structure issue? \$\endgroup\$ – Kalev Maricq Jun 18 '15 at 15:05
  • \$\begingroup\$ @KalevMaricq i is only "allowed" for things like for (var i = 0; i< max; i++), not to name List<int> etc. Variable names should be self-explanatory; I've given an example of how to reduce the length of a line by splitting it in some places. Public fields vs properties. "Hardcoded values are just for testing." I suspected that, but even then I wouldn't do it, since you'll need to change it later anyway. \$\endgroup\$ – BCdotWEB Jun 18 '15 at 15:16
  • \$\begingroup\$ @KalevMaricq "If I followed these, would ModSync be understandable or is it a structure issue?" I suspect it would increase the legibility of your code, but ModSync would still do too much. Even if it's a mere "50 lines", it does a lot of things. Like I said: perhaps it would be better as a class of its own with a number of smaller, specialized methods (e.g. Move(), Delete(), ...). \$\endgroup\$ – BCdotWEB Jun 18 '15 at 15:20
  • \$\begingroup\$ I made some updates to my code. A couple questions: 1. Does putting "return" in a using block stop the fileStream from closing? 2. You suggested having ModSync be it's own class. I'm not sure what this would look like. Is it understandable enough for you to give an example now that I made some of your changes? 3. I'm not sure how to fix the log filepath. Path.Combine doesn't seem to do much, since I have only two pieces (folder and file). I'm not sure how String.Format would differ from the String.ToString formatting. \$\endgroup\$ – Kalev Maricq Jun 18 '15 at 19:23
4
\$\begingroup\$

When you say "method should be kept short" it doesn't mean to put everything in one line. It means to separate into methods, each having it's own specific function.

Example, this line.

for (int y = 0; y < dirf.Count; y++)
{
    if (string.Compare(dirf[y].FSOs[i[y]].RelativePath, dirf[a].FSOs[i[a]].RelativePath) < 0)  a = y;
}

Could be a function

a = GetSomeValue(dirf, y, i, a);
\$\endgroup\$
  • \$\begingroup\$ So, you're suggesting I add a FirstFile method that contains the loop and then set a = FirstFile(dirf, i)? Or are you suggesting I keep the loop and move just the if statement to a new method? \$\endgroup\$ – Kalev Maricq Jun 18 '15 at 19:45
  • \$\begingroup\$ To be honest, there's no way I can understand your code unless I spend a lot of time analysing it... It's a bit hard for me to give you concrete example. If the job of that loop is to find the first file, than you can put that in a function called FindFirstFile. It's just a way to reduce the size of your function. \$\endgroup\$ – the_lotus Jun 19 '15 at 12:50

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.