1
\$\begingroup\$

I am busy creating a file processor that needs to get some files from an Ftp client download the file and save the data into the database. I have a two part question.

  1. How can i refactor the code to look a bit cleaner? Feels like the method ProcessFiles is doing a bit much. I can extract some of the methods but then i am just sitting with a lot of small methods and not really gaining anything.
  2. What will be the best way the handle exceptions? I need to be able to capture the file that caused an error.

Code Sample

public void Run()
        {
            var ftpFileManager = new FtpFileManager("ftp.someurl.net", "username", "password");   
            var files = ftpFileManager.GetDirectory("somepath");

            try
            {
                ProcessFiles(files, ftpFileManager);    
            }
            catch (AggregateException ae)
            {
                foreach (var ex in ae.InnerExceptions)
                {
                    if (ex is ArgumentException)
                        Console.WriteLine(ex.Message);
                    else
                        throw ex;
                }
            }
        }



private void ProcessFiles(IEnumerable<FtpListItem> files, IFtpFileManager ftpFileManager)
        {
            var exceptions = new ConcurrentQueue<Exception>();
            files.AsParallel().ForAll(async _ =>
            {
                try
                {
                    var dataFileService = _container.GetExportedValue<IDataFileHandler>();
                    var progress = new ProgressReporter(_container.GetExportedValue<IDataFileHandler>());

                var attr = FtpFileAttributes.Parse(_.Name);
                attr.DateOfFile = _.Modified;
                var dataFileId = await dataFileService.AddAsync(attr);

                await progress.Report(Status.Started, dataFileId);

                var file = ftpFileManager.DownloadFile(_.FullName);
                var fileHandler = FileHandlerFactory.Create(attr.FileType, attr);
                var list = fileHandler.ProcessFile(file);

                var handler = CreateHandler(attr.FileType);
                await handler.AddAsync(list, dataFileId);

                var dataFile = await dataFileService.GetAsync(dataFileId);
                dataFile.EndDate = DateTime.Now;
                dataFile.TotalRecords = list.Count();
                await dataFileService.UpdateAsync(dataFile);

                await progress.Report(Status.Completed, dataFileId);
            }
            catch (Exception e)
            {
                exceptions.Enqueue(e);
            }
        });

        if (exceptions.Count > 0) 
            throw new AggregateException(exceptions);
    }
\$\endgroup\$
  • \$\begingroup\$ Is the instance of IDataFileHandler a Singleton? \$\endgroup\$ – JanDotNet Jul 10 '16 at 20:47
1
\$\begingroup\$

Exception order changes processing

This looks really odd to me:

catch (AggregateException ae)
{
    foreach (var ex in ae.InnerExceptions)
    {
        if (ex is ArgumentException)
            Console.WriteLine(ex.Message);
        else
            throw ex;
    }
}

It's basically saying write ArgumentExceptions to the Console, but only until you find an error that isn't an ArgumentException. Then throw that exception.

I don't like it firstly because it could have different results based on order that the exceptions have been added to the queue and secondly because it discards any exceptions after the first one.

Exceptions for program flow

Having ProcessFiles which is a private method return void and throw an aggregate exception, which is immediately caught in the public caller, feels wrong to me. You've already collected the exceptions into a queue, why not just have the method return the queue? The caller can then decide what to do based on the collection being empty or not.

\$\endgroup\$
0
\$\begingroup\$

In reply to question #1; You say you feel the method is doing a bit too much, maybe split the different behaviours out into different methods. Good naming conventions should improve readability and error tracing.

and question #2; I've found when having to maintain state in inputs, creating data structures that contain the instructions lends itself well to encapsulating all relevant information about that instruction. Combining that with a queue would give you the ability to add metadata to each instruction as it is being processed and also attaching any errors along with that. Combined with the refactoring as mentioned earlier you could then accurately process failures at specific points in the workflow.

\$\endgroup\$
  • \$\begingroup\$ Thanks Werner, i will definitely look into breaking the method into smaller pieces. Also i think i was over complicated things a bit with the error logging. You can actually access the file inside the catch. so i know exactly what file failed \$\endgroup\$ – R4nc1d Mar 23 '16 at 9:39

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.