Business logic to interact with Data access layer with exceptional handling

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.

• Database table 1 should generate a RefNo

• RefNo generated in step 1, should be stored in a session variable

• Delete the entries with the RefNo in table 2 (we cannot

use update because the type of data we are getting from the UI)

• 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();
var i = 0;

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

if (HttpContextManager.GetSessionValue("submissionId") != null)
{

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 Parameters {get;}

protected DataParameters()
{
    Parameters = new Dictionary();
}

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});

//add

StackExchange Code Review Q#134577, answer score: 2