So I have button that sends a POST
request along with two variables, an outbox ID called oid
, and an email
. The controller takes in the variables and performs a method call, the end goal would be to send a JSON
response back depending on whether it fails or succeeds.
I have the following written for the controller:
public ActionResult ResendEmail(int oid, string email = null)
{
try
{
Outbox.ResendEmail(oid, email);
return Json(new { success = true, message = "Email has been queued to be resent", status = (int)HttpStatusCode.OK });
}
catch (Exception)
{
return Json(new { success = false, message = "An error occurred when attempting to resend the email", status = (int)HttpStatusCode.InternalServerError });
}
}
Problem here, is that 1. If the oid
and/or email
is empty, the method call for Outbox.ResendEmail(oid, email)
will still be called and a success json response will be returned. I'm also unsure if this is a proper way to use try/catch.
Feels like I should re-write how i'm handling this. Maybe an if statement to check for an existing oid
and email
followed by a try/catch?
CodePudding user response:
The try/catch is useful for handling exceptions. If Outbox.ResendEmail()
throws an exception, then the try/catch will do its thing.
However, if Outbox.ResendEmail()
will not throw an exception when given an incorrect oid
or an incorrect, empty, or null email
, then you need something else, such as an if-statement.
In addition, it probably is not appropriate to return a 500 error due to invalid parameters. It's not really a server error if the user passes a null email address.
I would recommend something like the following:
public ActionResult ResendEmail(int oid, string email = null)
{
// If you have a way to validate the oid, then use that instead of just checking for 0:
if (oid == 0)
{
return // one of the 400 level responses
}
// Do you have a way to validate the email address?
// If so, probably do that here, depending on how Outbox.ResendEmail() behaves
if (string.IsNullOrWhiteSpace(email))
{
return // one of the 400 level responses
}
try
{
Outbox.ResendEmail(oid, email);
return Json(new { success = true, message = "Email has been queued to be resent", status = (int)HttpStatusCode.OK });
}
catch (Exception)
{
return Json(new { success = false, message = "An error occurred when attempting to resend the email", status = (int)HttpStatusCode.InternalServerError });
}
}
CodePudding user response:
Try Catch always work with exception. As per the below statement in your question, it seems that Outbox.ResendEmail does not throw exception
If the oid and/or email is empty, the method call for Outbox.ResendEmail(oid, email) will still be called and a success json response will be returned. I'm also unsure if this is a proper way to use try/catch
instead of exception, it may be returning response with status false or error or not 200. That why, it is not going to catch and returning json response with success = false.
you can do like this
public ActionResult ResendEmail(int oid, string email = null)
{
if ( oid <=0 || string.IsNullOrWhiteSpace(email) || validation of email if any)
{
return response; // with statuscode among 400 and proper message in response
//that where is the problem in request.
}
try
{
var response= Outbox.ResendEmail(oid, email);
if (response.StatusCode == 200) // condition for check that response is false or success
{
return Json(new { success = true, message = "Email has been queued to be resent", status = (int)HttpStatusCode.OK });
}
return Json(new { success = false, message = "An error occurred when attempting to resend the email", status = (int)HttpStatusCode.InternalServerError });
}
catch (Exception)
{
return Json(new { success = false, message = "An error occurred when attempting to resend the email", status = (int)HttpStatusCode.InternalServerError });
}
}