2
\$\begingroup\$

I'm new to C#, and want to conform as much as possible to good design patterns. This code is the beginning of a Microservice, running as Asp.Net. It is based on a Microsoft tutorial doing similar work.

It has three functional components currently:

  1. Converts a CSV file to JSON, for return via REST
  2. A REST controller to test the conversion of the CSV to JSON return
  3. A background service which monitors a directory looking for changed files. The file attributes (Path, Date, Checksum) are stored in a MongoDB database via a Repository.

Opinions/Recommendations regarding the design patterns welcomed.

The full code can be found at: https://github.com/BioComSoftware/unite-radimaging-source-n2m2.git

BONUS: Notice in FileSearchHostedService.cs, I instantiate the Repository objects explicitly. I think it would be more appropriate to have them as Dependency Injection - but I couldn't get it to work. Notice the commented-out lines that would have done this. My understanding is; I would need to do this with IScopedProcessingService - but I have no idea how to do this with this specific code.

Structure

[Startup.cs]

using unite.radimaging.source.n2m2.Data;
using unite.radimaging.source.n2m2.Repositories;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
//using Microsoft.OpenApi.Models;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

namespace unite.radimaging.source.n2m2 {
    public class Startup {
        public Startup(IConfiguration configuration) {
            Configuration = configuration;
        }

        public IConfiguration Configuration { get; }

        // This method gets called by the runtime.
        // Use this method to add services to the container.
        public void ConfigureServices(IServiceCollection services) {

            services.AddControllers();

            services.AddScoped<IFoundFileContext, FoundFileContext>();
            services.AddScoped<IFoundFileRepository, FoundFileRepository>();
        }

        // This method gets called by the runtime.
        // Use this method to configure the HTTP request pipeline.
        public void Configure(IApplicationBuilder app, IWebHostEnvironment env) {
            if (env.IsDevelopment()) {
                app.UseDeveloperExceptionPage();
            }
            app.UseExceptionHandler("/error");
            app.UseRouting();
            app.UseAuthorization();
            app.UseEndpoints(endpoints => { 
                endpoints.MapControllers(); 
            });
        }
    }
}

[FoundFileRepository.cs]

using unite.radimaging.source.n2m2.Data;
using unite.radimaging.source.n2m2.Entities;
using MongoDB.Driver;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

namespace unite.radimaging.source.n2m2.Repositories {
   public class FoundFileRepository : IFoundFileRepository {
       private readonly IFoundFileContext _context;
       public FoundFileRepository(IFoundFileContext context) {
           _context = context ?? throw new ArgumentNullException(nameof(context));
       }

       public async Task<IEnumerable<FoundFile>> GetFiles() {
       return await _context
           .FoundFiles
           .Find(p => true)
           .ToListAsync();
       }

       public async Task<FoundFile> GetFile(string id) {
           return await _context
                          .FoundFiles
                          .Find(p => p.Id == id)
                          .FirstOrDefaultAsync();
       }

       public async Task<FoundFile> GetFileByPath(string path) {
           return await _context
                          .FoundFiles
                          .Find(p => p.Path == path)
                          .FirstOrDefaultAsync();
       }

       public async Task<FoundFile> GetFileByChecksum(string checksum) {
           return await _context
                          .FoundFiles
                          .Find(p => p.Checksum == checksum)
                          .FirstOrDefaultAsync();
       }

       //public async Task<IEnumerable<FoundFile>> GetFileByMtime(string mtime) {
       //    FilterDefinition<FoundFile> filter = Builders<FoundFile>.Filter.ElemMatch<DateTime>(p => p.Mtime, mtime);

       //    return await _context
       //                    .FoundFiles
       //                    .Find(filter)
       //                    .ToListAsync();
       //}

       //public async Task<IEnumerable<FoundFile>> GetFileBySize(long size) {
       //    FilterDefinition<FoundFile> filter = Builders<FoundFile>.Filter.ElemMatch<long>(p => p.Size, size);

       //    return await _context
       //                    .FoundFiles
       //                    .Find(filter)
       //                    .ToListAsync();
       //}

       public async Task CreateFile(FoundFile foundFile) {
           await _context.FoundFiles.InsertOneAsync(foundFile);
       }

       public async Task<bool> UpdateFile(FoundFile foundFile) {
           var updateResult = await _context.FoundFiles.ReplaceOneAsync(
               filter: g => g.Path == foundFile.Path, replacement: foundFile);
           return updateResult.IsAcknowledged && updateResult.ModifiedCount > 0;
       }

       public async Task<bool> DeleteFile(string path) {
           FilterDefinition<FoundFile> filter = Builders<FoundFile>.Filter.Eq(p => p.Path, path);

           DeleteResult deleteResult = await _context
                                               .FoundFiles
                                               .DeleteOneAsync(filter);

           return deleteResult.IsAcknowledged
               && deleteResult.DeletedCount > 0;
       }
   }
}

[FileSearchHostedService.cs]

using unite.radimaging.source.n2m2.Repositories;
using unite.radimaging.source.n2m2.Entities;
using unite.radimaging.source.n2m2.Data;
using Serilog;
using Microsoft.Extensions.Configuration;
using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Hosting;

namespace unite.radimaging.source.n2m2.HostedServices.FileSearchHostedService {
    public class FileSearchHostedService : BackgroundService {
        private readonly IConfiguration _configuration;
        //private IFoundFileRepository _repository;

        public FileSearchHostedService(
            //IFoundFileRepository repository,
            IConfiguration configuration
            ) {
            //_repository = repository ?? throw new ArgumentNullException(nameof(repository));
            _configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
        }

        protected override async Task ExecuteAsync(CancellationToken cancellationToken) {
            Log.Information("FileSearchHostedService: File searching started.");

            cancellationToken.Register(() => Log.Information("Processing service stopped"));

            //string checksum;
            string _msg; 
            string    _current_path;
            FileInfo  _current_fileinfo;
            FoundFile _foundFile;
            FoundFile _existingFile;
            // Convert these two lines to Dependency Injections if possible
            FoundFileContext FoundfileContext = new FoundFileContext(_configuration); // Normal instantiation, in leiu of injection
            FoundFileRepository _repository = new FoundFileRepository(FoundfileContext);  // Normal instantiation, in leiu of injection
            string dir = _configuration.GetValue<string>("FileSearchSettings:SearchDir");

            while (!cancellationToken.IsCancellationRequested) {
                string[] files = Directory.GetFiles(dir);

                foreach (string filename in files) {
                    _msg = $"'{filename}' ";
                    _current_fileinfo = new FileInfo(filename);
                    _current_path = Path.GetFullPath(Path.Combine(_current_fileinfo.Directory.ToString(), _current_fileinfo.Name));
                    _existingFile = await _repository.GetFileByPath (_current_path);

                    _foundFile = new FoundFile() {
                        Path = _current_path,
                        Size = _current_fileinfo.Length,
                        Mtime = _current_fileinfo.LastWriteTime,
                        Checksum = FileChecksum.getChecksum(filename)
                    };

                    if (_existingFile == null) {
                        _msg += $"does not exist in MongoDB. Creating new entry ";
                        // Creating a new entry in MongoDB should prolly be it's own class
                        //  but until I figure out dependency injection for the repo, it creates more 
                        //  processing than its worth
                        try {
                            await _repository.CreateFile(_foundFile);
                            _msg += "(OK)";
                        }
                        catch (Exception e) {
                            _msg += $"(FAILED with '{e.Message}'";
                        }
                    }
                    else if (_foundFile.Equals(_existingFile)) {
                        _msg += "is already in the database. No further processing needed.";
                    }
                    
                    else {
                        _msg += $"has changed since being added to MongoDB. Updating entry ";

                        try {
                            var _result = await _repository.UpdateFile(_foundFile);
                            if (!_result == true) {
                                throw new ApplicationException("FoundFileRepository.UpdateFile did not return 'true'. Unknown MongoDB error updating the document.");
                            }
                            _msg += "(OK)";
                        }
                        catch (Exception e) {
                            _msg += $"(FAILED with '{e.Message}')";
                        }
                    };

                    Log.Debug(_msg);
                }
                await Task.Delay(_configuration.GetValue<int>("FileSearchSettings:Delay"),cancellationToken);
            }
        }
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ Welcome to CodeReview! From what aspect(s) are you looking for a review? \$\endgroup\$ – Peter Csala Apr 19 at 12:13
  • \$\begingroup\$ @PeterCsala Thanks. Since I'm new to C#, there's three things I'd like a review on. 1) The general class structure/layout (see the image of VS's Solution Explorer.) 2) Whether my classes need to be more subdivided (See FileSearchHostedService.cs... It will be searching, checking, and creating new MongoDB entries. It does a lot) and 3) How/If I can instantiate the FoundFileRepository as a Dependency Injection instead of a explicit new(). Thanks again! \$\endgroup\$ – RightmireM Apr 19 at 12:20
  • 3
    \$\begingroup\$ Welcome to the Code Review Community. We may not be able to help with 3) How/Ifsince we only review working code and can't write new code. Other than that, excellent question. \$\endgroup\$ – pacmaninbw Apr 19 at 13:55
  • 1
    \$\begingroup\$ Not gonna do a review, but: 1) no not use underscores at the start of local variables. 2) use PascalCase/camelCase where necessary and don't use underscores to separate parts of a compound word. 3) Fix typos ("leiu"). 4) The _msg += thing is deeply annoying. Just do a Log.Debug each time, that way your code is clearer. 4) Decrease your nesting: at several points you have a try inside an if (or else) inside a foreach inside a while. \$\endgroup\$ – BCdotWEB Apr 20 at 7:41
  • 1
    \$\begingroup\$ @RightmireM google.com/search?q=visual+studio+spell+checker \$\endgroup\$ – BCdotWEB Apr 20 at 10:06
1
\$\begingroup\$

In this post let me focus on the project structure.

Project vs Projects

Most of the time when we implement a n-tier application we separate each tier into its own project. We also separate implementation and abstraction. Some like this:

  • {Project}.Presentation
  • {Project}.Service.Abstractions
  • {Project}.Service
  • {Project}.Repository.Abstractions
  • {Project}.Repository

Each tier has its own responsibility and scope. For example the Presentation (or sometimes called Web) tier will handle the incoming requests (via Middlewares and Controllers) by

  • performing preliminary checks (like: is xyz present, is xyz an int, etc.)
  • calling the business layer (service tier)
  • transforming the result (to align to the presentation framework)

In a simple project like this the above showed separation might be an overkill, but as your application scope grows you might need to perform this kind of restructuring.

I also want to mention that for more complex Microservices n-tier architecture might not be the best choice. Onion or Hexagonal architecture might be a better fit.

Controller

  • DefaultController: Try to name your controllers based on the resources on which they are performing some kind of operations. In case of REST APIs the primary building blocks are the resources. So, organizing your code around them is the suggested way.
  • ErrorController: Without knowing the implementation it is kinda hard to make any suggestions. But nowadays we usually handle application errors via a custom error handler middleware in WebApis.
  • TestCSVController: It is really weird to use verbs inside the controller names. Resources are usually nouns.

I also want to mention that there is an assessment process to evaluate the maturity of your REST API. It called Ricardson's maturity model (1).

CSVParsers

I can see two implementation classes here and no abstraction. Based on the image it is impossible to tell how they relate to each other. Do they provide the same API (then define an ICSVParser)? Are they in a parent-child relationship? ...

Data

Personally I don't like the following naming conventions: XYZManager, Utils, Data, Helpers, etc. They simply do not provide any value. Based on these names you still don't know anything.

Naming is hard. So, I'm not saying that Data is bad rather I would like to suggest to evaluate your naming whenever you add/remove/alter some file in this directory.

Nowadays refactoring tools make the renaming quite painless.

Entities

In your Data folder you have Context class definitions. In your Entities folder you have (most probably) POCO definitions to hold data. As you can see the naming is kinda weird from this aspect.

Databases and DataModels might be better names in my opinion.

Repositories

Without the concrete implementation it is hard to tell but based on the naming the FileChecksum is not a repository. I do think it should not belong here.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the detailed response. I'm making some changes based on your suggestions. I'm interested in input on other parts of the code (not really related to this answer.) Is it more appropriate to update this question and code, or should I accept an answer and repost a new question? \$\endgroup\$ – RightmireM Apr 20 at 9:56
  • 1
    \$\begingroup\$ @RightmireM Please follow the tour, and read "What topics can I ask about here?", "How do I ask a good question?" and "What types of questions should I avoid asking?". \$\endgroup\$ – BCdotWEB Apr 20 at 10:07
  • 1
    \$\begingroup\$ @RightmireM Before any review is posted you can amend your code in your question. But after that you should not. If you have further questions or want to receive further reviews for the revised code then you advised to create a new question and refer to the original one. \$\endgroup\$ – Peter Csala Apr 20 at 12:44

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.