Looking for proper technique

[darkwater23]

I'm a .Net noob creating a new contact form. I updated this code from a 1.1 technique to a 2.0 technique.

My first question is when the email is sent successfully, is Response.Redirect the "correct" way to move to the thank you page? (The code used to use views, but for SEO, I was asked to drop that technique.)

My second question is related to the try/catch. If I comment out the Response.Redirect in the catch, it shows the Thank You page and I get the email. If I uncomment the Response.Redirect in the catch, it goes to the error page and I still get the email. What's the right way to do the following?

Thanks!

private void processForm()
    {
        String emailTo = ConfigurationSettings.AppSettings["ContactToAddress"];
        String emailFrom = ConfigurationSettings.AppSettings["ContactFromAddress"];
        String emailSubject = "Contact Form Submission";
        String emailBody = "First Name: " + FirstName.Text + "\n" +
                           "Last Name: " + LastName.Text + "\n" +
                           "Company: " + Company.Text + "\n" +
                           "Phone: " + Phone.Text + "\n" +
                           "Email: " + Email.Text + "\n" +
                           "Department: " + ServiceInterest.SelectedItem.Value + "\n\n" +
                           "Comments:\n" + Comments.Text;

        SmtpClient client = new SmtpClient();
        MailMessage email = new MailMessage(emailFrom, emailTo, emailSubject, emailBody);
        email.Priority = MailPriority.High;

        try
        {
            client.Send(email);
            Response.Redirect("thank-you.aspx");
        }
        catch (Exception exc)
        {
            //literalErrorMessage.Text = exc.ToString();
            Response.Redirect("contact-error.aspx");
        }
    }

[webcosmo]

why dont you redirect to the same url with a querystring to indicate success.

[darkwater23]

I could do that, but I'm concerned on why the try/catch isn't behaving as I expect it to. Do yo have any insight on that?

[BrianOConnell]

Response.redirect throws an exception because redirecting aborts the current thread. This is correct behavior. Hence it will always run the code in the catch so you should probably use a boolean variable and set it depending on success then after the try catch check this boolean and redirect accordingly.

[honeymonster]

Just move

Response.Redirect("thank-you.aspx");

out after the try-catch block.

[honeymonster]

Indeed, lose the try-catch all together. You do not need it. Any uncaught exceptions will invoke the standard error/exception behavior of ASP.NET. You configure in web.config which page to display. If the error page is an aspx page it has access to the exception details.

private void processForm()
    {

        ...

        client.Send(email);
        Response.Redirect("thank-you.aspx");
    }

in web.config (example):

		<customErrors mode="On" defaultRedirect="error.aspx">
			<error statusCode="403" redirect="NoAccess.htm"/>
			<error statusCode="404" redirect="FileNotFound.htm"/>
		</customErrors>

[darkwater23]

That's a good idea! I was going to implement custom error pages anyway, so that will work well.

Also, thanks for the insight on the Response.Redirect. That explains a lot.

Thanks guys!

[dhtmlgod]

Just thought I would do a quick refactoring for to:

- Improve Readability
- Sort the Responsiblity of the methods by making smaller tightly focused methods
- Maintainability, aided by the above, each method has only one reason to change

private void CreateAndSendEmail()
        {
            MailMessage mailMessage = CreateMailMessage();
            AddToAddress(mailMessage);
            AddFromAddrress(mailMessage);
            SetSubject(mailMessage, "Contact Form Submission");
            SetBody(mailMessage);
            SetPriority(mailMessage);
 
            SendMailMessage(mailMessage);
        }
 
        private MailMessage CreateMailMessage()
        {
            return new MailMessage();
        }
 
        private void AddToAddress(MailMessage message)
        {
            message.To.Add(new MailAddress(GetAppSettingsValue("ContactToAddress")));
        }
 
        private string GetAppSettingsValue(string key)
        {
            return ConfigurationSettings.AppSettings[key];
        }
 
        private void AddFromAddrress(MailMessage message)
        {
            message.From = new MailAddress(GetAppSettingsValue("ContactFromAddress"));
        }
 
        private void SetSubject(MailMessage message, string subject)
        {
            message.Subject = subject;
        }
 
        private void SetPriority(MailMessage message)
        {
            message.Priority = MailPriority.High;
        }
 
        private void SetBody(MailMessage message)
        {
            message.Body = "First Name: " + FirstName.Text + "\n" +
                           "Last Name: " + LastName.Text + "\n" +
                           "Company: " + Company.Text + "\n" +
                           "Phone: " + Phone.Text + "\n" +
                           "Email: " + Email.Text + "\n" +
                           "Department: " + ServiceInterest.SelectedItem.Value + "\n\n" +
                           "Comments:\n" + Comments.Text;
        }
 
        private void SendMailMessage(MailMessage mailMessage)
        {
            SmtpClient client = new SmtpClient();
            client.Send(mailMessage);
        }

I personally would take this refactroing futher and abstract every as a it's own class and radically change the SetBody to accept a template. With ASP.NET MVC I use View's and the MVC ViewEngine