To quote Mark Seemann from the following article Small methods are easy to troubleshoot
Write small methods. How small? Small enough that any unhandled exception is easy to troubleshoot.
Break the function up into smaller more manageable chunks that focus on a single concern as much as possible.
Yes, the net effect of this is a lot of additional functions. They are however easier to read and maintain.
Another benefit of the many small methods it that it would help identify areas that can be generalized into reusable services that help reduce repeated code, allowing it (the code) to be more DRY (Don't Repeat Yourself)
Lets start with getting the connection
private SqlConnection createConnection() {
var connection = (SqlConnection) sqlHelper.CreateConnection(); //sqlHelper is a nugget and not in my
connection.Open();
return connection;
}
This can now be wrapped in a using
block that will dispose of the connection once out of scope
using(SqlConnection connection = createConnection()) {
//...
}
While there is nothing wrong with creating the command manually, I personal prefer to let the connection do it rather than newing one. Again wrapping in a using
block, and populating.
//...
using(SqlCommand command = connection.CreateCommand()) {
command.CommandText = Constants.GetAllParkingSlips;
command.CommandType = System.Data.CommandType.StoredProcedure;
command.Parameters.Add("@CreatedBy", SqlDbType.NVarChar).Value = gepservice.GetUserContext().UserId;
command.Parameters.Add("@OffsetRows", SqlDbType.NVarChar).Value = offset;
command.Parameters.Add("@FetchRows", SqlDbType.NVarChar).Value = fetch;
//...
}
That could have also been refactored into its own method, but in my opinion it would have ended up with too many arguments.
This could be fixed with an aggregated value object.
public class ParkingSlipQuery {
public int FetchRows { get; set; }
public int OffsetRows { get; set; }
public string CreatedBy { get; set; }
}
for example
private SqlCommand createCommand(SqlConnection connection, ParkingSlipQuery query) {
SqlCommand command = connection.CreateCommand();
command.CommandText = Constants.GetAllParkingSlips;
command.CommandType = System.Data.CommandType.StoredProcedure;
command.Parameters.Add("@CreatedBy", SqlDbType.NVarChar).Value = query.CreatedBy;
command.Parameters.Add("@OffsetRows", SqlDbType.NVarChar).Value = query.OffsetRows;
command.Parameters.Add("@FetchRows", SqlDbType.NVarChar).Value = query.FetchRows;
return command;
}
and implemented
var query = new ParkingSlipQuery {
CreatedBy = gepservice.GetUserContext().UserId,
OffsetRows = offset,
FetchRows = fetch
};
using(SqlCommand command = createCommand(connection, query)) {
//...
}
The execution of the command and subsequent reader result however can be moved
private SqlDataReader executeReader(SqlCommand command) {
var refCountdr = (RefCountingDataReader) sqlHelper.ExecuteReader(cmd);
return (SqlDataReader) refCountdr.InnerReader;
}
As you have realized by now, you should know what is coming next about building the model to populate the collection.
private ParkingSlipDetails getDetails(SqlDataReader reader) {
ParkingSlipDetails details = new ParkingSlipDetails();
details.Id = Convert.ToInt32(reader[FileUploadConstants.Id]);
details.FileName = Convert.ToString(reader[FileUploadConstants.FileName]);
details.FileUri = Convert.ToString(reader[FileUploadConstants.FileURI]);
details.PONumber = Convert.ToString(reader[FileUploadConstants.DocumentNumber]);
details.UploadDate = Convert.IsDBNull(reader[FileUploadConstants.UploadDate]) ? DateTime.UtcNow: Convert.ToDateTime(reader[FileUploadConstants.UploadDate]);
details.FileStatus = (Status)(Convert.ToInt16(reader[FileUploadConstants.FileStatus]));
details.ReceiptNumber = Convert.ToString(reader[FileUploadConstants.RecieptNumber].ToString());
details.TotalRows = Convert.ToInt32(reader[FileUploadConstants.TotalRows]);
details.CreatedBy = gepservice.GetUserContext().UserId;
return details;
}
private List<ParkingSlipDetails> getDetailsList(SqlDataReader reader, string userId) {
List<ParkingSlipDetails> detailsList = new List<ParkingSlipDetails>();
if (reader != null) {
while (reader.Read()) {
ParkingSlipDetails details = getDetails(reader);
details.CreatedBy = userId;
detailsList.Add(details);
}
}
return detailsList;
}
Finally, to avoid null reference errors, the method should try to avoid return null
. An empty collection is safer to check than a null collection.
This results in refactor looking like
public CustomReturn<List<ParkingSlipDetails>> GetAllParkingSlips(int offset, int fetch) {
List<ParkingSlipDetails> detailsList = new List<ParkingSlipDetails>();
try {
using(SqlConnection connection = createConnection()) {
string userId = gepservice.GetUserContext().UserId;
var query = new ParkingSlipQuery {
CreatedBy = userId,
OffsetRows = offset,
FetchRows = fetch
};
using(SqlCommand command = createCommand(collection, query)) {
using(SqlDataReader reader = executeReader(command)) {
detailsList = getDetailsList(reader, userId);
}
}
}
} catch(Exception ex) {
LogError("ManageParkingSlipDataAccess", "GetAllParkingSlips", "Error", ex);
detailsList = new List<ParkingSlipDetails>();
}
return new CustomReturn<List<ParkingSlipDetails>>(detailsList);
}
The using
block will handle the closing and disposal of the connect and the other disposable members.
If following explicit dependency principle, it could be refactored further down by delegating the argument values to the caller.
public CustomReturn<List<ParkingSlipDetails>> GetAllParkingSlips(ParkingSlipQuery query) {
List<ParkingSlipDetails> detailsList = new List<ParkingSlipDetails>();
try {
using(SqlConnection connection = createConnection()) {
using(SqlCommand command = createCommand(collection, query)) {
using(SqlDataReader reader = executeReader(command)) {
detailsList = getDetailsList(reader, query.CreatedBy);
}
}
}
} catch(Exception ex) {
LogError("ManageParkingSlipDataAccess", "GetAllParkingSlips", "Error", ex);
detailsList = new List<ParkingSlipDetails>();
}
return new CustomReturn<List<ParkingSlipDetails>>(detailsList);
}
with the following support methods
private SqlConnection createConnection() {
var connection = (SqlConnection) sqlHelper.CreateConnection(); //sqlHelper is a nugget and not in my
connection.Open();
return connection;
}
private SqlCommand createCommand(SqlConnection connection, ParkingSlipQuery query) {
SqlCommand command = connection.CreateCommand();
command.CommandText = Constants.GetAllParkingSlips;
command.CommandType = System.Data.CommandType.StoredProcedure;
command.Parameters.Add("@CreatedBy", SqlDbType.NVarChar).Value = query.CreatedBy;
command.Parameters.Add("@OffsetRows", SqlDbType.NVarChar).Value = query.OffsetRows;
command.Parameters.Add("@FetchRows", SqlDbType.NVarChar).Value = query.FetchRows;
return command;
}
private SqlDataReader executeReader(SqlCommand command) {
var refCountdr = (RefCountingDataReader) sqlHelper.ExecuteReader(cmd);
return (SqlDataReader) refCountdr.InnerReader;
}
private List<ParkingSlipDetails> getDetailsList(SqlDataReader reader, string userId) {
List<ParkingSlipDetails> detailsList = new List<ParkingSlipDetails>();
if (reader != null) {
while (reader.Read()) {
ParkingSlipDetails details = getDetails(reader);
details.CreatedBy = userId;
detailsList.Add(details);
}
}
return detailsList;
}
private ParkingSlipDetails getDetails(SqlDataReader reader) {
ParkingSlipDetails details = new ParkingSlipDetails();
details.Id = Convert.ToInt32(reader[FileUploadConstants.Id]);
details.FileName = Convert.ToString(reader[FileUploadConstants.FileName]);
details.FileUri = Convert.ToString(reader[FileUploadConstants.FileURI]);
details.PONumber = Convert.ToString(reader[FileUploadConstants.DocumentNumber]);
details.UploadDate = Convert.IsDBNull(reader[FileUploadConstants.UploadDate]) ? DateTime.UtcNow: Convert.ToDateTime(reader[FileUploadConstants.UploadDate]);
details.FileStatus = (Status)(Convert.ToInt16(reader[FileUploadConstants.FileStatus]));
details.ReceiptNumber = Convert.ToString(reader[FileUploadConstants.RecieptNumber].ToString());
details.TotalRows = Convert.ToInt32(reader[FileUploadConstants.TotalRows]);
return details;
}