6
\$\begingroup\$

The code below is the BLL and the DAL logic for the form submission with multiple pages.

A form will have more than one page, will have a button on every page until it reaches the final submission.

For the first page when submitted,the following actions need to be done.

  1. Database table 1 should generate a RefNo
  2. RefNo generated in step 1, should be stored in a session variable
  3. Delete the entries with the RefNo in table 2 (we cannot use update because the type of data we are getting from the UI)
  4. insert into table 2 for every fieldData on the form along with refNo

For the subsequent requests, i.e. from second page onwards, steps 3 and 4 need to be implemented.

The following is the working code but I want someone to review and suggest with best practises or optimize the existing code. I am also concerned whether exception handling is done at every stage.

There are questions in comments where clarification is needed. Would greatly appreciate if anyone could reply. I am more concerned with optimized code and exception handling. Not to worry about business rules.

BLL Layer:

Interface:

public interface IContentService
    {
        string GetRefNo(FormData formData);
    }

Implementation:

public class ContentService: IContentService
    {
        private IFormDataServiceWorker _formDataService;

        public FormServiceWorker(IFormDataServiceWorker formDataService)
        {
            this._formDataService = formDataService;
        }        

        public string GetRefNo(GenericFormData formData)
        {            
            var httpVariables = new HttpRequestVariables(); //the class is given below
            var submissionHeaderDTO = new SubmissionHeaderDTO(); // DTO class is given below
            var formFieldDataList = new List<FormFieldDTO>();
            var i = 0;

            //Q1 : is there any better way of declaration than above?

            if (HttpContextManager.GetSessionValue<SubmissionHeaderDTO>("submissionId") != null)
            {
                submissionHeaderDTO = HttpContextManager.GetSessionValue<SubmissionHeaderDTO>("submissionId");
            }
            else
            {
                submissionHeaderDTO = this._formDataService.GetRefNo(httpVariables); //step 1

                //Q2: what if executenonquery fails , how to handle the return result?

                HttpContextManager.SetSessionValue<SubmissionHeaderDTO>("submissionId", submissionHeaderDTO); //step 2
            }
            //Q3 :Is there any better way of storing in a session?
            foreach(var fieldData in formData.FormFieldData)
            {
            //"value=Male&label=Gender&id=4ad09c1a-dc21-4748-974b-d2a79b36a403_gender||value=Option 1&label=orientation&id=4ad09c1a-dc21-4748-974b-d2a79b36a403_orientation"
                if (fieldData.Value != null)
                {
                    var subFields = fieldData.Value.Split(new string[] { "||" }, StringSplitOptions.RemoveEmptyEntries);

                    if (subFields.Count() > 0)
                    {
                        foreach (var subField in subFields)
                        {
                            var subFieldValue = System.Web.HttpUtility.ParseQueryString(subField).Get("value");
                            var subFieldName = System.Web.HttpUtility.ParseQueryString(subField).Get("label");

                            formFieldDataList.Add(new FormFieldDTO()
                            {
                                FieldId = i + 1,
                                FieldTitle = fieldData.Title,
                                FieldName = fieldData.UniqueKey + "_" + fieldData.Title + "_" + subFieldName,
                                FieldValue = subFieldValue,
                                SubmissionId = submissionHeaderDTO.SubmissionId
                            });
                        }
                    }
                }
            }
            if (formFieldDataList.Count > 0)
            {
                this._formDataService.DeleteFormData(submissionHeaderDTO.SubmissionId);  //step 3

                //Q4: what if executenonquery fails , how to handle the return result?
                foreach (var formFieldData in formFieldDataList)
                {
                    this._formDataService.SubmitFormData(formFieldData);

                    //Q5: what if executenonquery fails , how to handle the return result?
                }
            }
            return submissionHeaderDTO.SubmitSequence;
        }
    }

    public class HttpRequestVariables
    {
        public string User { get { return HttpContext.Current.User.Identity.Name; } }

        public string Url { get { return HttpContext.Current == null || HttpContext.Current.Handler == null ? null : HttpContext.Current.Request.Url.AbsoluteUri; } }

        public string UserAgent { get { return HttpContext.Current.Request.ServerVariables["HTTP_USER_AGENT"]; } }

        public string HttpReferrer { get { return HttpContext.Current.Request.ServerVariables["HTTP_REFERER"]; } }

        public string RemoteHost { get { return HttpContext.Current.Request.ServerVariables["REMOTE_HOST"]; } }

        public string RemoteHostName { get { return System.Net.Dns.GetHostEntry(HttpContext.Current.Request.ServerVariables["REMOTE_ADDR"]).HostName; } }
    }

    public class SubmissionHeaderDTO
    {
        public Guid SubmissionId { get; set;}

        public string SubmitSequence { get; set; }
    }

Data Access Layer

Interface:

public interface IFormDataServiceWorker
    {
        SubmissionHeaderDTO GetRefNo(HttpRequestVariables httpVariables);

        void SubmitFormData(FormFieldDTO formFieldData);

        void DeleteFormData(Guid submissionId);
    }

Implementation:

public SubmissionHeaderDTO GetRefNo(HttpRequestVariables requestVariables)
        {
            using(var sqlConn = new SqlConnection(Configuration.DBConnection))
            {
                sqlConn.Open();                
                    using (var sqlcmd = new SqlCommand("usp_SubmissionHeaderInsert", sqlConn))
                    {
                        sqlcmd.CommandType = CommandType.StoredProcedure;
                        sqlcmd.Parameters.Add("@FormId", SqlDbType.UniqueIdentifier).Value = Guid.NewGuid();
                        sqlcmd.Parameters.Add("@FormCode", SqlDbType.VarChar, 10).Value = "TST";
                        sqlcmd.Parameters.Add("@FormTitle", SqlDbType.VarChar, 200).Value = "Test Form";
                        sqlcmd.Parameters.Add("@User", SqlDbType.VarChar, 50).Value = requestVariables.User;
                        sqlcmd.Parameters.Add("@Host", SqlDbType.VarChar, 50).Value = requestVariables.RemoteHostName;
                        sqlcmd.Parameters.Add("@Url", SqlDbType.VarChar, 255).Value = requestVariables.Url;
                        sqlcmd.Parameters.Add("@UserAgent", SqlDbType.VarChar, 255).Value = requestVariables.UserAgent;

                        SqlParameter outputSubmissionId = new SqlParameter("@SubmissionId", SqlDbType.UniqueIdentifier)
                        {
                            Direction = ParameterDirection.Output
                        };

                        SqlParameter outputSubmissionSequence = new SqlParameter("@SubmitSequence", SqlDbType.NVarChar, 30)
                        {
                            Direction = ParameterDirection.Output
                        };

                        sqlcmd.Parameters.Add(outputSubmissionId);
                        sqlcmd.Parameters.Add(outputSubmissionSequence);

                        try
                        {
                            sqlcmd.ExecuteNonQuery();
                            var submissionHeaderDto = new SubmissionHeaderDTO()
                            {
                                SubmissionId = (Guid)outputSubmissionId.Value,
                                SubmitSequence = outputSubmissionSequence.Value.ToString()
                            };
                            return submissionHeaderDto;
                        }
                        catch(SqlException ex)
                        {
                            Logger.Log(Logging.Options.LoggingLevel.Error, string.Format("Error in getting the refNo {0}.", ex.ToString()), Logging.Options.EventType.Save);  
                            throw;
                            //Q6: do we need to use 'throw' even after logging? What is the best practise?
                        }                                
                    }
            }

        }

        public void SubmitFormData(FormFieldDTO formFieldData)
        {
            using (var sqlConn = new SqlConnection(Configuration.DBConnection))
            {
                sqlConn.Open();
                using (var sqlcmd = new SqlCommand("usp_SubmissionDataInsert", sqlConn))
                {
                    sqlcmd.CommandType = CommandType.StoredProcedure;
                    sqlcmd.Parameters.Add("@SubmissionId", SqlDbType.UniqueIdentifier).Value = formFieldData.SubmissionId;
                    sqlcmd.Parameters.Add("@FieldId", SqlDbType.SmallInt).Value = formFieldData.FieldId;
                    sqlcmd.Parameters.Add("@FieldTitle", SqlDbType.VarChar, 500).Value = formFieldData.FieldTitle;
                    sqlcmd.Parameters.Add("@FieldData", SqlDbType.VarChar, -1).Value = formFieldData.FieldValue;
                    sqlcmd.Parameters.Add("@FieldName", SqlDbType.VarChar, 200).Value = formFieldData.FieldName;                    

                    try
                    {
                        sqlcmd.ExecuteNonQuery();
                        //Q7: I do not need any return value after submitting data, can I leave it without capturing any return value, What is the best practise?what to assert in the unit test if nothing is returned?
                    }
                    catch (SqlException ex)
                    {
                        Logger.Log(Logging.Options.LoggingLevel.Error, string.Format("Error in submitting the form data {0}.", ex.ToString()), Logging.Options.EventType.Save);
                        throw;
                    }
                }
            }

        }

        public void DeleteFormData(Guid submissionId)
        {
            using (var sqlConn = new SqlConnection(Configuration.DBConnection))
            {
                sqlConn.Open();
                using (var sqlcmd = new SqlCommand("usp_SubmissionDataDelete", sqlConn))
                {
                    sqlcmd.CommandType = CommandType.StoredProcedure;
                    sqlcmd.Parameters.Add("@SubmissionId", SqlDbType.UniqueIdentifier).Value = submissionId;

                    try
                    {
                        sqlcmd.ExecuteNonQuery();
                        //Q8: I do not need any return value after submitting data, can I leave it without capturing any return value, What is the best practise?

                    }
                    catch (SqlException ex)
                    {
                        Logger.Log(Logging.Options.LoggingLevel.Error, string.Format("Error in deleting the form data {0}.", ex.ToString()), Logging.Options.EventType.Save);
                        throw;
                    }
                }
            }
        }
\$\endgroup\$
  • \$\begingroup\$ As a complete aside: practise is the verb - practice is the noun. Therefore, it's "best practice". \$\endgroup\$ – RobH Jul 12 '16 at 9:16
2
\$\begingroup\$

Answer to Q1: These declarations look OK.

Many of your questions revolve around what to do when an exception occurs while executing a query against the database. Logging them is a good idea. If you want to 'bubble' your exception and handle it then yes this throw; is necessary as it will preserve the stack. If you went this route you can wrap the exception higher up in the callstack with something that exposes less potentially sensitive information (since this appears to be a call close to the database) in the response. The logging call will still allow you to preserve the most details server-side without having to worry about exposing them too much to consumers as well as recognize the anomalous state.

I'm not sure if you are planning on running your tests against a standard database. If you are you may want to consider additional refactoring so you can mock this database layer in your tests and avoid needing a live database for your tests.

Data Access Layer:

I think FormDataServiceWorker.GetRefNo(HttpRequestVariables requestVariables) should be renamed and refactored. This method is confusing since ExecuteNonQuery() is intended for INSERT, UPDATE and DELETE queries, yet the method appears to be getting a value. Nowhere in the method is there a refNo retrieved. Perhaps GetSubmissionHeader() or something similar would be more appropriate. Consumers of this function will know more precisely what will happen when it is executed.

This method as well as SubmitFormData and DeleteFormData are pretty long and does a few different things that could be broken into other methods. Perhaps consider building up the SqlCommand Parameters in a separate method or class that takes in and uses the HttpRequestVariables object. If you would rather not have the connection object outside of this method (even though still in the using block) you can create the SqlCommand object without it in your new method/class where building up the parameters. Then assign it in GetRefNo and the other methods prior to executing the query. Doing this will help isolate functionality for testing as well as readability by keeping this code closer to the Single Responsibility Principle.

  • You could also consider moving this database logic into a new class of its own entirely. I see hardcoded values like the sqlcommand parameter names and lengths. Pulling these out of the method and into private constants (if they are not changing) would help mitigate any potential errors of omission when refactoring. Creating an object to hold the sql parameter values themselves so that adding more is easier in the future if/when necessary will help maintainability and testability too.

This would allow you to just send in this object to a method that builds up sqlcommand objects. A primary method in a new database logic-centric class could be:

public class DbHelper 
{

public SqlCommand BuildParameterizedStoredProcedureCommand(string commandName, DataParameters parameters)
{
    var sqlcmd = BuildParameterizedSqlCommand(commandName, parameters);

    sqlcmd.CommandType = CommandType.StoredProcedure;
    return sqlcmd;
}

public SqlCommand BuildParameterizedSqlCommand(string commandName, DataParameters parameters)
{
    if(parameters == null)
    {throw new ArgumentNullException("Data parameters cannot be null."}

    var sqlcmd = new SqlCommand(commandName);
    sqlcmd.AddRange(parameters.ParametersAsArray()); 

    return sqlcmd;
}

public int ExecuteNonQuery(SqlCommand command)
{
    int rowsModified;


    try{
        rowsModified = command.ExecuteNonQuery();
    }catch(SqlException e)
    {
        Logger.Log("Error while executing query: " + e.ToString(), Logging.Options.EventType.Save);
        throw;
    }

    return rowsModified;
}
}

Then adding additional parameters can be done at the FormParameter level just once without changing the BuildSqlCommand method. This can be expanded to other areas where this pattern seems to exist (SubmitFormData potentially).

Something like:

public abstract class DataParameters
{
IDictionary<string, SqlParameter> Parameters {get;}

protected DataParameters()
{
    Parameters = new Dictionary<string, SqlParameter>();
}

public SqlParameter[] ParametersAsArray()
{
    return Parameters.ToArray();
}

}

   public class SubmissionDataParameters : DataParameters
{   
public SubmissionDataParameters()
{
        //Pull out constants as necessary

        Parameters.Add("SubmissionId", new SqlParameter("@SubmissionId", SqlDbType.UniqueIdentifier));

        //additional parameters..
    }
}

}

public class FormDataParameters{

public FormParameters()
{   
    //Pull out constants as necessary

   Parameters.Add("FormId", new SqlParameter("@FormId", SqlDbType.UniqueIdentifier);
   Parameters.Add("FormCode", new SqlParameter("@FormCode", SqlDbType.VarChar, 10));
   Parameters.Add("SubmitSequence", new SqlParameter("@SubmitSequence", SqlDbType.NVarChar, 30){Direction = ParameterDirection.Output});

   //additional parameters..
}
}

When you create the SubmissionHeaderDTO in GetRefNo you will get a more valuable exception for a bad outputSubmissionId.Value if you assign SubmissionId to a new Guid(outputSubmissionId.Value). As it is now, you will get anInvalidCastException for an invalid guid rather than a reason why the cast failed.

Answer to Q7 and Q8: If you are not capturing the amount of modified records then yes you can probably ignore the return value here. If this is the case your test could execute an additional query to verify the expected data was added. This is another area I would recommend refactoring a bit so that the query logic is a little more separated.

If the number of modified records were returned then that value could potentially be a testable value, but I would consider making sure the modified records are correct as well in your test.

At the very least, you may want to consider some form of logging the return value of ExecuteNonQuery()calls. A return value of -1 can mean a rollback occurred. C# will not throw for errors with a severity level less than 16. If any of your stored procedures use the RAISEERROR function they'll need to specify a severity level of 16 in order cause .NET to throw an exception (at least, for the Sql Server DB Engine). Example of this.

Finally, if you are using C# 6 the properties in HttpRequestVariables can be shortened using Expression Bodies: - public string User => HttpContext.Current.User.Identity.Name;

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