2
\$\begingroup\$

I want to delete files from folder, which are not stored in DB, and DB records, which no more linked to files. My code:

        public async Task DeleteNonExistingImagesInFolder(string imagesDirectory)
        {
            var images = _unitOfWork.Images.AsQueryable();
            DirectoryInfo d = new DirectoryInfo(imagesDirectory);
            FileInfo[] Files = d.GetFiles();
            await Task.Run(() =>
            {
                foreach (var file in Files)
                {
                    if (!images.Where(i => i.Path == file.FullName).Any())
                        file.Delete();
                }
            });
        }

        public async Task DeleteNonExistingImagesInDB(string imagesDirectory)
        {
            var images = _unitOfWork.Images.AsQueryable();
            DirectoryInfo d = new DirectoryInfo(imagesDirectory);
            FileInfo[] Files = d.GetFiles();
            await Task.Run(() =>
            {
                foreach (var image in images)
                {
                    if (!Files.Where(f => f.FullName == image.Path).Any())
                        _unitOfWork.Images.Remove(image.Id);
                }
            });
            _unitOfWork.Complete();
        }

Is there a better way to do this faster and with less code?

UPD

I call these methods in my web-api controller. Here is code:

        [Authorize(Roles="Admin")]
        [Route("delete")]
        [HttpDelete]
        public async Task<IActionResult> ClearNonExisting()
        {
            try
            {
                _logger.Warn("Deleting redundant images.");
                await _imageService.DeleteNonExistingImagesInFolder(_ImageDirectory);
                _logger.Warn("Deleting empty image links.");
                await _imageService.DeleteNonExistingImagesInDB(_ImageDirectory);
                return Ok();
            }
            catch
            {
                _logger.Fatal("Exception while clearing empty links from DB and deleting redundant images.");
                return StatusCode(500);
            }
        }

I using a generic repository to operate Image records. Repository code:

    public class Repository<TEntity> : IRepository<TEntity> where TEntity : class
    {
        protected readonly DbContext Context;

        public Repository(DbContext context)
        {
            Context = context;
        }
        public void Remove(int id)
        {
            try
            {
                Context.Set<TEntity>().Remove(Get(id));
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot remove {typeof(TEntity)} #{id}.", e);
            }
        }

        public void RemoveRange(IEnumerable<TEntity> entities)
        {
            try
            { 
                Context.Set<TEntity>().RemoveRange(entities);
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot remove range of {typeof(TEntity)}s.", e);
            }
        }
        public IQueryable<TEntity> AsQueryable()
        {
            try
            {
                return Context.Set<TEntity>().AsQueryable();
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot return IQueryable<{typeof(TEntity)}>.", e);
            }
        }


        public void Add(TEntity entity)
        {
            try
            {
                Context.Set<TEntity>().Add(entity);
            }
            catch(Exception e)
            {
                throw new DataAccessException($"Cannot add new {typeof(TEntity)}.", e);
            }
        }

        public void AddRange(IEnumerable<TEntity> entities)
        {
            try
            {
                Context.Set<TEntity>().AddRange(entities);
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot add range of {typeof(TEntity)}s.", e);
            }
        }

        public IEnumerable<TEntity> Find(Expression<Func<TEntity, bool>> predicate)
        {
            try
            {
                return Context.Set<TEntity>().Where(predicate);
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot find the {typeof(TEntity)}s with | {predicate.ToString()} | predicates.", e);
            }
        }

        public TEntity Get(int id)
        {
            try
            {
                return Context.Set<TEntity>().Find(id);
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot get {typeof(TEntity)} #{id}.", e);
            }
        }

        public IEnumerable<TEntity> GetAll()
        {
            try
            {
                return Context.Set<TEntity>().ToList();
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot get all {typeof(TEntity)}s.", e);
            }
        }
        public IEnumerable<TEntity> AsEnumerable()
        {
            try
            {
                return Context.Set<TEntity>().AsEnumerable();
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot return IEnumerable<{typeof(TEntity)}>.", e);
            }
        }

        public void Update(int id, TEntity entity)
        {
            try
            {
                var entry = Context.Entry(Get(id));
                entry.CurrentValues.SetValues(entity);
                entry.State = EntityState.Modified;
            }
            catch (Exception e)
            {
                throw new DataAccessException($"Cannot update values of {typeof(TEntity)} #{id}.", e);
            }
        }
    }

UPD 2

UnitOfWork code:

public class AgencyUnitOfWork:IAgencyUnitOfWork
    {
        private readonly AgencyContext _context;

        private IArticleRepository articleRepository;
        public IArticleRepository Articles => articleRepository ?? 
            (articleRepository = new ArticleRepository(_context));

        private IContactRepository contactRepository;
        public IContactRepository Contacts => contactRepository ?? 
            (contactRepository = new ContactRepository(_context));

        private IRealtorRepository realtorRepository;
        public IRealtorRepository Realtors => realtorRepository ?? 
            (realtorRepository = new RealtorRepository(_context));

        private IRepository<Image> imageRepository;
        public IRepository<Image> Images => imageRepository ??
            (imageRepository = new Repository<Image>(_context));

        /// <summary>
        /// Constructor with DbContext and needed Repositories.
        /// </summary>
        /// <param name="context">Context to work with.</param>
        [Inject]
        public AgencyUnitOfWork(AgencyContext context)
        {
            _context = context;
        }

        public int Complete()
        {
            return _context.SaveChanges();
        }

        public void Dispose()
        {
            _context.Dispose();
        }
    }  
\$\endgroup\$
  • 1
    \$\begingroup\$ Why are you using Task.Run for this? Could you give us more details about your code? It'd be very helpful to know how you call these two methods. \$\endgroup\$ – t3chb0t Aug 3 at 14:28
  • 1
    \$\begingroup\$ I don't know, you're not telling us much about the context where you're using it. \$\endgroup\$ – t3chb0t Aug 3 at 14:32
  • 1
    \$\begingroup\$ They're running synchronously (one after another). Is this what you wanted? We also need to see the _unitOfWork or at least know where you commit the deletion. From your current code it looks like it should not delete any images from the database. \$\endgroup\$ – t3chb0t Aug 3 at 14:37
  • 1
    \$\begingroup\$ @t3chb0t, i just forgot about calling Complete() method. \$\endgroup\$ – Mykhailo Mushynskyi Aug 3 at 14:45
  • 1
    \$\begingroup\$ /*Other repository methods*/ - don't hesitate posting the complete class ;-] Code Review can handle quite a lot. The more the better. I wonder why you are deleting files from the database one by one when you have this API RemoveRange? Doing it with Remove is pretty inefficient. \$\endgroup\$ – t3chb0t Aug 3 at 14:46
4
\$\begingroup\$

In ClearNotExisting, you log a message when something goes wrong, but it would probably make sense to record what went wrong (i.e. spit out some information about the exception).


You have inline documentation in some places, but methods like Complete could really do with it (Will it throw if I call it multiple times? What is the return value? Can I make changes after calling Complete?). Personally, I'm of the opinion that any public API should have at least basic inline documentation, because otherwise I can't be sure that the person who wrote it knows what they were writing (which is usually the case with me).


I'm not convinced that it is DeleteNonExistingImagesInDB's responsibility to call Complete on the unit of work: what if you later decide to perform some other database maintenance task having called it? Then you transaction will be cut in two, and your unit-of-work will have become units-of-work.


You should consider using a HashSet (or similar) to perform these checks:

if (!Files.Where(f => f.FullName == image.Path).Any())

If the number of files is big, this will be slow. Such a replacement is trivial, will generally improve the scalability of this 'out of database' method, and will make the code clearer as well:

HashSet<string> files = new HashSet<string>(d.GetFiles().Select(f => f.FullName));
if (!Files.Contains(image.Path))

You could also achieve this with a Where clause, and then use the RemoveRange method in DeleteNonExistingImagesInDB. This may be more efficient, as t3chb0t has suggested.

_unitOfWork.Images.RemoveRange(images.Where(image => !Files.Contains(image.Path)));

Someone who knows how to use DbContexts seriously can probably suggest a solution that makes use of temporary tables and all that Jazz to avoid transferring the image information about and minimise the duration of the transaction, but I am not such a person.


I find it a little unnerving that public instance method DeleteNonExistingImagesInDB takes the directory as a parameter: it would all too easy to call it with the wrong directory, and delete every record in the database (whereafter the cost would delete all the images, because the database is empty). This just feels wrong: it looks like it is meant to support multiple image directories, and feels like a general-purpose static method, but it is not.


I would like to see a null check in AgencyUnitOfWork..ctor: it's meant to be injected, but that doesn't stop it being misused in some other fashion. Same with Repository..ctor.


Your line-spacing is inconsistent in places (e.g. between RemoveRange and AsQueryable), which just makes the code that little bit more difficult to scan, and has a habit of changing over time, which just clutters commits and messes with the feel of the code.


The exception message in AsQueryable is a bit off: I'm not sure that returning is really the problem, but it's good that the inner exception is there.

throw new DataAccessException($"Cannot return IQueryable<{typeof(TEntity)}>.", e);

If AgencyUnitOfWork is to support disposal of the underlying context, then you should consider having it formally implement IDisposable.

\$\endgroup\$

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.