1
\$\begingroup\$

I'm not really sure what the preferred way is to deal with records not found in the database. Is it better to write a Find method which returns null or a Get method which raises a RecordNotFoundException?

[AuthenticateFilter(UsernameAndSecretKey)]
[Route("api/v1/activities/emails/{id}")]
[HttpGet]
public IHttpActionResult GetEmailActivity(int id)
{
  try
  {
    // business logic service, could use only db service but this way we can do unit tests (just fill bl service method with fake objects)
    var service = new EmailActivityBlService();

    // 1. use Find method which returns null in case record with provided id does not exist in db
    var model = service.FindActivity(id);
    if( model != null )
      return Ok(model);
    return NotFound();  

   // 2. or is this approach better
   // throws RecordNotFoundException in case row by id is not found in database
   return Ok(service.GetActivity(id));
  }
  catch(RecordNotFoundException e) { return NotFound(); }
  catch(Exception e) { return InternalServerError(e); }
}

EmailActivityBlService has next code in case anyone interested (showing only the important part):

private EmailActivityDbService _dbService;

public EmailActivityModel GetActivity(int id)
{
  var model = this._dbService.GetActivity(id);
  if( model == null )
    throw new RecordNotFoundException(); // I suppose System.Data.ObjectNotFound is also suitable
  return model;
}

public EmailActivityModel FindActivity(int id)
{
  // typical entity framework query
  // using(var context = new ..) { return contect.EmailActivity.Where()..SingleOrDefault().ConvertToModel();
  return this._dbService.GetActivity(id);
}
\$\endgroup\$
5
  • \$\begingroup\$ Is this an unintended error that you're getting? \$\endgroup\$
    – Jamal
    Commented Aug 27, 2016 at 19:19
  • \$\begingroup\$ No intended of course. This is correct behaviour. Api has to return 404 if resource is not found, just not sure which code is better - Find returning null or Get throwing exception. \$\endgroup\$
    – broadband
    Commented Aug 27, 2016 at 19:28
  • \$\begingroup\$ I need to make sure because if this code is broken, it's off-topic. \$\endgroup\$
    – Jamal
    Commented Aug 27, 2016 at 19:35
  • \$\begingroup\$ I see. The code works as it should. Well, I could change the title so it would say "it doesn't work'. \$\endgroup\$
    – broadband
    Commented Aug 27, 2016 at 19:42
  • \$\begingroup\$ Welcome to CodeReview; that's a good first question, enjoy your stay! \$\endgroup\$
    – ferada
    Commented Aug 27, 2016 at 21:24

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Browse other questions tagged or ask your own question.