3
\$\begingroup\$

I'm writing a console application that will look through a directory and move any log files that have a date modified older than X days (configurable in the app.config file).

The problem I'm having is with the Move method I wrote. It works, but I think there might be a better way to write this method and eliminate the need for the sourceRoothPath parameter. Here's what I have (it's part of a LogManager class I wrote to do the checking and moving of log files):

public void Move(string sourceRootPath, string sourcePath, string targetPath)
{
    if (String.IsNullOrWhiteSpace(sourcePath))
        throw new ArgumentNullException("sourcePath");

    if (String.IsNullOrWhiteSpace(targetPath))
        throw new ArgumentNullException("destinationPath");

    if (!File.Exists(sourcePath))
        throw new FileNotFoundException(sourcePath);

    try
    {
        // trimmedPath becomes the file path with all the subfolders, but without the
        // sourceRootPath that comes in front of it. i.e. it strips the value passed
        // in sourceRootPath from the value passed in sourcePath. The "+ 1" is to include the
        // trailing "\" in the path.
        string trimmedPath = sourcePath.Substring(sourceRootPath.Length + 1);
        string newPath = Path.Combine(targetPath, trimmedPath);
        string fileName = Path.GetFileName(sourcePath);

        // folderStructure is used for creating the subfolder structure I want to preserve.
        // (It is just removing the file name and extension from the newPath.)
        string folderStructure = newPath.Substring(0, (newPath.Length - fileName.Length));

        // Directory.CreateDirectory will create the entire folder structure for me; no need
        // for looping or recursive calls.
        Directory.CreateDirectory(folderStructure);

        // File.Move has no ability to overwrite, so I have to delete the file if it exists in the
        // destination directory so that File.Move doesn't throw an exception.
        if (File.Exists(newPath))
            File.Delete(newPath);

        File.Move(sourcePath, newPath);
        loggingService.LogInfo("Moved file from " + sourcePath + " to " + newPath + ".");
    }
    catch (Exception ex)
    {
        loggingService.LogError(ex.Message);
        throw ex;
    }
}

I included the sourceRootPath parameter because I need this method to duplicate the folder structure of the sourcePath, but I don't want it to include some of the top level folders.

So, for example, if I'm having this application scan a log file directory at C:\someFolder\test\logs, and there are some subfolders like production\someDate or HR\HRSystem\someDate etc. I want to keep the subfolder structure in the production and HR folders, but move them to a new location like C:\test_log_archive. So C:\someFolder\test\logs\production\someDate should become C:\test_log_archive\production\someDate, and the value I would pass for sourceRootPath would be C:\someFolder\test\logs.

Does it make sense to remove the sourceRootPath and how would I do it? I feel like I should be able to accomplish this with just sourcePath and targetPath parameters, but I can't figure out a way to do it other than the code I provided above.

\$\endgroup\$
  • \$\begingroup\$ You should be able to utilize a source and destination, what is the root vs source structure look like? \$\endgroup\$ – Greg May 24 '16 at 13:59
  • \$\begingroup\$ @Greg continuing with the example I posted in my question, sourcePath would be the path to the file it is moving like C:\someFolder\test\logs\production\someDate\someLog.txt. sourceRootPath would be C:\someFolder\test\logs because I would want to preserve the production\someDate folder structure. targetPath would be C:\test_log_archive with the end goal of the log file to be moved from C:\someFolder\test\logs\production\someDate\someLog.txt to C:\test_log_archive\production\someDate\someLog.txt. \$\endgroup\$ – Lews Therin May 24 '16 at 14:05
  • \$\begingroup\$ To the downvoters, please write a comment so I can improve my question. Seriously. @Mat'sMug I've edited the title. Is that acceptable now? \$\endgroup\$ – Lews Therin May 24 '16 at 15:13
  • \$\begingroup\$ CR community expects your code to be working as intended; not my downvote, but my guess is that "the problem" and "how can I do X?" stick out and make the post look more like a "how do I?" question, vs the expected "here's my code, does it make my ass look fat?" open feedback question that reviewers want to see on this site. See How to Ask for more info; in general, if your question reads like a question, you're not asking for feedback on any & all facets of the code; a good CR post is usually off-topic on SO, and vice-versa. \$\endgroup\$ – Mathieu Guindon May 24 '16 at 15:22
4
\$\begingroup\$

No, you do definitely need 3 parameters to do what you are trying to do

Although I would suggest a different 3.

public void Move(string sourceRoot, string targetRoot, string[] relative) {
    /* Precondition checks and try-catch removed for clarity */

    string source = Path.Combine(sourceRoot, relative);
    string target = Path.Combine(targetRoot, relative);
    string folderStructure = Path.GetDirectoryName(target);

    /* etc */
}

This makes clear that there is shared structure under source and target, and there isn't any direct string manipulation on file and folder names. I would even be tempted to factor this out into a seperate class, e.g.

class DirectoryMover {
    private string source;
    private string target;

    private void Move(string[] relative) { ... }
    public void MoveAll() { /* iterate over folders calling Move */ }
}
\$\endgroup\$
  • \$\begingroup\$ Yes! This is what I was looking for. It even gets rid of the string manipulation I really didn't want to have to do (edge cases that could break the code and whatnot). Although why did you make the relative parameter a string array? \$\endgroup\$ – Lews Therin May 24 '16 at 15:07
  • 1
    \$\begingroup\$ @Sylverac The natural implementations of MoveAll would gather a collection of folder/file names (either with an explicit Stack<string> imperatively or recursively on the stack), and string[] matches the signature of Path.Combine, and is easy to get from a collection ( IEnumerable<>.ToArray() ) \$\endgroup\$ – Caleth May 24 '16 at 15:20
  • \$\begingroup\$ So when implementing this solution, it still leaves me with a problem. How do I get the value of relative? My current implementation calls the Move method from within a foreach loop that is iterating through a List<string> collection that contains the "expired" logs. Would I not have to do the same substring gymnastics to get the value to pass in relative? \$\endgroup\$ – Lews Therin May 24 '16 at 15:38
  • 1
    \$\begingroup\$ no, whatever is generating the expired logs is presumably searching from source? As it goes through the directory structure, just keep track of the current relative path, e.g. with a Stack<string>. \$\endgroup\$ – Caleth May 24 '16 at 15:51
  • \$\begingroup\$ Hmm, I seem to be wrong about the values returned by System.IO.Directory.GetFileSystemEntries etc. there doesn't seem to be a nice "directory entries relative to path" function \$\endgroup\$ – Caleth May 24 '16 at 16:07

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.