5
\$\begingroup\$

I have implemented the Repository pattern in my Web APi 2 + Entity Framework 6.1 + Angular 1.4 SPA site. The application allows management of a martial arts club. One of its functionalities is to allow to handle belt promotions. This involves some validations i.e. You can't add a promotion to a higher belt with a date lower than an existing promotion to a lower belt. Currently i have this validation as part of the repository. This is obviously wrong. I have a couple of questions:

  1. Where should I put this validation? Should it be in the controller?
  2. Should I create a separate class for validation and use it in controller?
  3. How should i return potential validation errors to the front end? Should it be an exception with a message?
  4. Is it a good practice to return results of more complex validations from the API and display those messages in the UI?
  5. Should I have one repository as controler field or instantiate it with each request? Does't instantiating with each request create a risk of issues with database connection?

Here is my current implementation of the repository and controller classes:

public class PromotionsRepository : RepositoryBase, IPromotionsRepository
{
    public PromotionsRepository()
    {
    }

    public async Task<List<Promotion>> GetPromotionsForMember(int memberId)
    {
        using (var context = new BjjClubEntities())
        {
            try
            {
                var promotions = await context.Members.Include("Promotions").Where(member => member.MemberId == memberId).SingleOrDefaultAsync();
                return promotions.Promotions.OrderBy(promotion => promotion.Date).ToList();
            }
            catch (Exception ex)
            {
                LogError(ex);
                return null;
            }
        }
    }

    public async Task AddPromotions(int memberId, List<Promotion> newPromotions)
    {
        using (var context = new BjjClubEntities())
        {
            try
            {
                newPromotions.ForEach(newPromotion =>
                {
                    var editedMember = context.Members.Include("Promotions").Where(member => member.MemberId == memberId).SingleOrDefault();

                    // can't promote to a lower belt or lower number of stripes 
                    var conflictingPromotionsBefore = editedMember.Promotions
                        .Where(promotion => newPromotion.Date > promotion.Date)
                        .Where(promotion => newPromotion.Belt < promotion.Belt || (newPromotion.Belt == promotion.Belt && newPromotion.Stripes < promotion.Stripes)).ToList();

                    // when inserting promotion that occured at a previous date
                    // no promotions after this date should be lower
                    var conflictingPromotionsAfter = editedMember.Promotions
                        .Where(promotion => newPromotion.Date < promotion.Date)
                        .Where(promotion => newPromotion.Belt > promotion.Belt || (newPromotion.Belt == promotion.Belt && newPromotion.Stripes > promotion.Stripes)).ToList();

                    // can't have two promotions on the same day
                    var conflictingPromotions = editedMember.Promotions
                        .Where(promotion => newPromotion.Date.Date == promotion.Date.Date).ToList();

                    conflictingPromotions.AddRange(conflictingPromotionsBefore);
                    conflictingPromotions.AddRange(conflictingPromotionsAfter);

                    if (conflictingPromotions.Count > 0)
                    {
                        var errorMessage = string.Join(", ", conflictingPromotions.Select(promotion => promotion.Date.ToShortDateString()).ToList());
                        throw new ArgumentException($"Promotions from the following dates conflict with the one You are trying to add: {errorMessage}");
                    }

                    editedMember.Promotions.Add(newPromotion);
                });

                await context.SaveChangesAsync();
            }
            catch (Exception ex)
            {
                LogError(ex);
                throw ex;
            }
        }
    }

    public async Task DeletePromotion(int promotionId)
    {
        using (var context = new BjjClubEntities())
        {
            try
            {
                var promotionToRemove = await context.Promotions.Where(promotionEvent => promotionEvent.PromotionId == promotionId).SingleOrDefaultAsync();

                context.Promotions.Remove(promotionToRemove);

                context.Entry(promotionToRemove).State = EntityState.Deleted;

                await context.SaveChangesAsync();
            }
            catch (Exception ex)
            {
                LogError(ex);
                throw ex;
            }
        }
    }
}

Here is the controller:

public class PromotionsController : ApiController
{
    private PromotionsRepository _promotionsRepository;

    public PromotionsController()
    {
        _promotionsRepository = new PromotionsRepository();
    }

    [Authorize]
    [HttpGet]
    [Route("Promotions/{memberId:int}", Name = "PromotionsByMemberId")]
    public async Task<IHttpActionResult> GetPromotionsForMember(int memberId)
    {
        return Ok(await _promotionsRepository.GetPromotionsForMember(memberId));
    }

    [Authorize]
    [HttpPost]
    [Route("Promotions", Name = "AddPromotionForMember")]
    public async Task<IHttpActionResult> AddPromotionsForMember([FromBody] List<Promotion> newPromotions)
    {
        int memberId = newPromotions[0].MemberId;
        PromotionValidator validator = new PromotionValidator(memberId, new PromotionsRepository());

        if (newPromotions.Count == 0)
        {
            throw new ArgumentException("No promotions to add");
        }

        Common.ValidationResult result = await validator.ValidateNewPromotions(newPromotions);

        if (!result.Result)
        {
            var message = new HttpResponseMessage(HttpStatusCode.Conflict)
            {
                Content = new StringContent(result.ValidationMessage)
            };
            throw new HttpResponseException(message);
        }

        await _promotionsRepository.AddPromotions(memberId, newPromotions);

        return Ok();
    }

    [Authorize]
    [HttpDelete]
    [Route("Promotions/{id:int}", Name = "DeletePromotion")]
    public async Task<IHttpActionResult> DeletePromotion(int id)
    {
        try
        {
            await _promotionsRepository.DeletePromotion(id);

            return Ok();
        }
        catch (Exception ex)
        {
            var message = new HttpResponseMessage(HttpStatusCode.Conflict)
            {
                Content = new StringContent(ex.Message)
            };
            throw new HttpResponseException(message);
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Just some comments. Way too much usage of Try/Catch. Just create a single global exception filter that handles/logs all exceptions. Also, don't use throw ex, just do throw. Also, you should learn how to use dependency injection in order to instantiate instances of your classes, such as the Repository class/DbContext. DI can scope instances to the lifetime of the request, and will automatically dispose when finished. Lastly, your repository class isn't so much a repository because it contains a lot of business logic - just call it a service class. \$\endgroup\$ – Brad M Jan 15 '17 at 18:32
  • 1
    \$\begingroup\$ Personally i dislike to use async\await if there is nothing parallel at all. \$\endgroup\$ – Disappointed Jan 16 '17 at 7:42
  • \$\begingroup\$ @Disappointed: Which part do You mean? I guess using async/await when nothing is parallel is not smart but I can't find such a case in the code I posted. \$\endgroup\$ – BodzioSamolot Jan 16 '17 at 8:58
  • \$\begingroup\$ @BradM: Thank You for valuable suggestions :) \$\endgroup\$ – BodzioSamolot Jan 16 '17 at 8:58
  • 1
    \$\begingroup\$ He SHOULD continue to use async/await in the API methods: stackoverflow.com/questions/26158789/… \$\endgroup\$ – Bryan Boettcher Jan 19 '17 at 21:21
3
\$\begingroup\$

First, some overall considerations:

        catch (Exception ex)
        {
            LogError(ex);
            return null;
        }

It is usually a bad practice to return null when a exception happens. The consumer of your method will think that there are no results (in this case, no promotions for member), when what actually happened is that the connection has time-outed and he need to retry...

    await _promotionsRepository.AddPromotions(memberId, newPromotions);

As @Disappointed commented, if there is nothing in parallel, there is no reason to use async/await.

Now, to your questions:

  1. Where should I put this validation? Should it be in the controller?

It is usually a good idea to separate your business logic from everything else, so changes on it does't spread throught all your application. So, the validation should be on your business layer. The controller gets the data from the repository, send it to the appropriate class on the business layer, which execute the rules on that data, and return the results to the controller.

  1. Should I create a separate class for validation and use it in controller?

Yes, as per answer 1.

  1. How should i return potential validation errors to the front end? Should it be an exception with a message?

That depends on your rule. If your method on the business layer is called "AddPromotions", it should either add a promotion or throw a exception. If your method is called "CanBePromoted" it should return a bool indicating if your user can be promoted, and throw if it is unable to determine whether the user can be promoted or not. In the end, it is all about the contract you are setting.

But when you say "validations", I'm pretty sure that you mean the "AddPromotions" situation. On this case, it is usually better to just throw. If you need to validade multiple rules and return all the issues at once, take a look at the class AggregateException, it is a exception that contains a list of exceptions inside.

  1. Is it a good practice to return results of more complex validations from the API and display those messages in the UI?

Well, sure, your user must know what he did wrong, right? But be sure to show the user just meaningful business errors: No null references, no database issues, no nothing that means "computer stuff went wrong". You are building a martial arts management program, so only messages that a martial arts teacher would understand should be displayed. A good way to do it is to have a specific exception type that you use to throw your validations, and make a global catch that return the text of this kind of exceptions to the UI. For all the other exceptions that this filter catch, return a generic error message and log the actual exception.

  1. Should I have one repository as controller field or instantiate it with each request? Does't instantiating with each request create a risk of issues with database connection?

On each request, you are correct on your assumption that it will create issues with the DB connection. Probably a lot of them.

\$\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.