Image Database Service

Question

I created simply database service with get, insert, get all image names and verify exist method.

All images will be store in blob storage with unique filename + postfix size. Next one I have image model, with simple expression for mapping object select, methods for detection Webp format and existing.

All methods:

• GetAllImageNames() - return enumerable list of imageNames
• Insert(string filename) - insert filename
• site GetImage(string filename) - return image entity, mapped to
• ImageModel ImageExist(string filename) - return true/false
• Delete(string filename) - delete file by filename

I wrote the Image object entity that represents a database object:

public class Image
{
    public int Id { get; set; }

    public string Filename { get; set; }               
}

This is a very simple entity, no comments are needed I think.

ImageModel (object result structure):

public class ImageModel
{
    public string Filename { get; set; }

    public ImageSize Size { get; set; }

    public static Expression<Func<Image, ImageModel>> ToModel()
    {
        return image => new ImageModel
        {
            Filename = image.Filename,
            Size = image.Size
        };
    }

    public bool Exists()
    {
        return !string.IsNullOrEmpty(Filename);
    }

    public bool IsWebp()
    {
       //Now implement for now
    }
}

This model class is very simply again:

ToModel() is a function for mapping to results data.

And finally DatabaseImageService:

public interface IImageDatabaseService : IRepository<Image>
{
    Task<IEnumerable<string>> GetAllImageNames();

    Task Insert(string filename);

    Task Delete(string filename);

    Task<ImageModel> GetImage(string filename);

    Task<bool> ImageExist(string filename);
}

IRepository is design pattern Repository for manipulation with database operations. It's possible to use it as DI of course. But I like to use it as an inherit interface of the current entity.

public class ImageDatabaseService : Repository<Image>, IImageDatabaseService
{
    public ImageDatabaseService(DbContext dbContext, IUnityOfWork unityOfWork) : base(dbContext)
    {
        this.UnityOfWork = unityOfWork;            
    }

    private IUnityOfWork UnityOfWork { get; set; }   

    public async Task<IEnumerable<string>> GetAllImageNames()
    {
        return await this.Select(s => s.Filename).ToListAsync();
    }

    public async Task Insert(string filename)
    {
        this.InsertAsBatch(filename);
        await this.UnityOfWork.SaveAsync();
    }      

    public async Task Delete(string filename)
    {
        var image = await this.FirstOrDefaultAsync(s =>
            s.Filename.Equals(filename, StringComparison.InvariantCultureIgnoreCase));
        if (image != null)
        {
            this.Remove(image);
            await this.UnityOfWork.SaveAsync();
        }
    }

    public async Task<bool> ImageExist(string filename)
    {
        var image = await GetImage(filename);
        return image.Exists();
    }

    public async Task<ImageModel> GetImage(string filename)
    {
        return await this.Where(s => s.Filename.Equals(filename, StringComparison.InvariantCultureIgnoreCase))
            .Select(ImageModel.ToModel()).FirstOrDefaultAsync();
    }

    private void InsertAsBatch(string filename)
    {
        var image = this.Create();
        image.Filename = filename;
        //possibly another image attributes

        this.Add(image);
    }
}

UnityOfWork is a design pattern for saving data to my database, used in combination with the Repository pattern.

Answer 1

First of all the controversial point: there are very good chances that you do not need a Repository. Why?

• Entity Framework already implements Unif of Work. No need to manually handle anything outside very very special cases.
• There are infinitesimal chances that you'll need to switch to another ORM (and if you will do then you will need to rewrite your service layer because EF-specific code pervades those classes).

Honestly: how many times you actually did it? I think I did it three times (in 20 years) and if I have a service layer (and it is well-written) then it's probably the only part you need to rewrite.

Drop all that code and make your code easy to read, programming is already complicate and error-prone without adding useless complexity.

Model

Do you actually need read/write properties? Details vary according to the EF version you're using but at least Image.Id might be read-only.

Validate your inputs! Is an empty file name valid?

public sealed class Image
{
    public int Id { get; }
    public string FileName
    {
        get => _fileName;
        set
        {
            if (String.IsNullOrWhiteSpace(value))
                throw new ArgumentException("...");

            _fileName = value;
        }
    }

    public Image(int id, string fileName)
    {
        _id = id;
        FileName = fileName;
    }

    private readonly int _id;
    private string _fileName;
}

Please do better than this (for example I hate to throw ArgumentException when input is null, ArgumentNullException should be used instead). If file name can be null (in your DB!) then just ignore this. Also note that if not required by your ORM then class should be sealed.

View Model

Same as above, do you need read/write properties? If not then make the relevant ones read-only.

I'm perplex about the ToModel() static method. Is there a good reason you're returning an Expression instead of the object itself? If for any reason you need some kind of lazy creation (and for such simple model without any heavy initialisation I can't imagine the reason) then why you're using an expression instead of a simple Func<T>?

Exists() and IsWebp() are named as properties but declared as methods. If the operation might be expensive then you're right to use methods but then you should name them as verb+noun (for example CheckIfFileExists()). If they're trivial then just use properties:

public bool Exists => !String.IsNullOrWhiteSpace(FileName);

ImageSize is declared here but it's not in the model. Do not post partial code (or, at least, be sure it compiles).

Service

Asynchronous methods should have the Async suffix. It's not required but it's a well-accepted convention and you should follow it.

InsertAsBatch() is misleading. Which batch? I see just one parameter. It might be InsertCore(), InsertImpl() or simply dropped all together because it does not add any value.

In the Delete() method you're doing something extremely dangerous: you're swallowing an error. If trying to delete a non existing image is a soft error you may, at least, return a flag (it's what, for example, .NET collection classes do). I'd prefer a good old exception but that's just my opinion. Also do not repeat your code, to search an image you already have GetImage() method:

public async Task<bool> DeleteAsync(string fileName)
{
    var image = await GetImage(fileName);
    if (image == null)
        return false;

    this.Remove(image);
    await this.UnityOfWork.SaveAsync();

    return true;
}

Whatever you pick be consistent: some methods silently ignore this issue (like DeleteAsync() while others will throw at run-time (like ImageExists()).

public async Task<bool> ImageExistsAsync(string fileName)
    => (await GetImageAsync(fileName))?.Exists() ?? false;

Validate your parameters! I can always pass null for fileName and I don't get any error. Even more serious: decide if null is valid or not because in GetImageAsync() you're calling ImageModel.FileName.Equals() and it'll undoubtedly fail with a tough NullReferenceException if that property is null.

I'm not sure if GetAllImages() should be asynchronous or not. You're reading values from DB and sometimes it might be useful to filter them ("...all images that contain text ABC...") but forcing materialization (even if asynchronous) precludes this possibility. It's also true that you should introduce a separate service method for this then I'd keep it as-is (just adding async in the name).

Are ImageModel.Exists() and ImageModel.IsWebp() used only inside the service? If yes then they should probably be internal instead of public.

Error handling? I do not see a single line to handle DB errors (for example concurrent updates). Is it everything inside Repository base class?

Answer 2

Looks fine to me, I don't agree with BCdotWEB. If you don't follow the patterns you are locking your application to a specific framework. Now you may argue that you're never going to replace the framework however I find that this claim is not true. When you do need to change frameworks (i.e. implementation details) you're really going to love the fact that you abstracted it away like a good software engineer. Additionally if you don't abstract you probably don't create unit tests of your code and for me this is as important as writing good code.

The code itself seems to do it job and obey the patterns.