I'm not really sure what is the prefered way when dealing with record not found in the database. Is it better to write Find method which returns null or Get method which returns 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);
}
UPDATE
Talked with my colleagues, we decided to go with this solution. As why GetActivity returns null instead of throwing Exception, I prefer answer from rboe:
So return null if it is can happen in your domain, that records do not exist (in my experience this is most often the case). If you expect a record to exist and it is not there, then it is valid to throw an exception.
[AuthenticateFilter(UsernameAndSecretKey)]
[Route("api/v1/activities/emails/{id}")]
[HttpGet]
public IHttpActionResult GetEmailActivity(int id)
{
var service = new EmailActivityBlService();
var model = service.GetActivity(id); // returns null in case activity is not found
if( model != null )
return Ok(model);
return NotFound();
}
We avoided any try-catch in the methods and put global filter when Exception occurs:
File: App_Start\WebApiConfig.cs
public class WebApiExceptionFilter : ExceptionFilterAttribute
{
public override void OnException(HttpActionExecutedContext actionExecutedContext)
{
actionExecutedContext.Response = actionExecutedContext.Request.CreateErrorResponse(HttpStatusCode.InternalServerError, actionExecutedContext.Exception.Message, actionExecutedContext.Exception);
}
}
Both ways are valid ones.
It is a different emphasis whether you use exceptions or the return value null
to indicate non existing records.
Exceptions exist to signal an error state (something happened that is abnormal). The code in the catch
-handler is focused on how to deal with an error and not to contain business logic.
If you return null
then it will be a normal and 'non exceptional' state in your model.
So return null
if it is can happen in your domain, that records do not exist (in my experience this is most often the case). If you expect a record to exist and it is not there, then it is valid to throw an exception
.
I disagree with the other answer. In the case of a GetyById method, I wouldn't say to return null instead of throwing because you could argue it was "expected" that there might not be a record with the requested id. This "exceptions for exceptional situations," while often stated, I don't really think is the best way to think about the method's contracts. APIs should make semantic sense, ideally.
Instead, I suggest to throw exceptions whenever the method cannot do what it was told to do. So GetById methods should thrown an exception in the event there is no such record with the requested id in the system. The Find method should probably return an enumerable, which of course could be empty in the event no records match the criteria given.
An API which has a FindById method strikes me as odd; if you are giving the API an ID, that implies the caller could somehow have learned the ID in a previous API call, and so the API shouldn't need to "find" an already known to exist record. It should provide a way to get the record directly by its id. Instead Find should be for locating records when you aren't sure they exist, and using some other criteria.
Given the web service call, I would go with the service calling the GetById method, as the web service caller also learned the id somehow. If the id turns out not to exist, the library can throw the RecordNotFoundException, which causes the service call to return 404.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With