C# Recording Traffic Data

Question

I would like to record client information when they go into a location on the web site so I created this ProcessRequest class that will collect data and insert it into the database. This will collect HttpContext and send that data to a Store Procedure that will then insert into a database table.

namespace DataCollection
{
    public class siteDataCollection
    {
        public void ProcessRequest(HttpContext context)
        {
            string conn = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
            string filename = Path.GetFileName(context.Request.Url.AbsolutePath);

            string customerGuid = context.Request.Cookies["customerguid"].Value.ToString();
            customerGuid = customerGuid.Replace("%2D", "-");

            if(customerGuid is null) {

                customerGuid = HttpContext.Current.Session.SessionID;
            }

            SQLHelper sqlh = new SQLHelper(conn);
            SqlDataReader dr = null;

            SqlParameter[] parms = new SqlParameter[] {
                            new SqlParameter("@CustomerGUID", SqlDbType.NVarChar, 50),
                            new SqlParameter("@FileName", SqlDbType.NVarChar, 255),
                            new SqlParameter("@REMOTE_ADDR", SqlDbType.NVarChar,15),
                            new SqlParameter("@LOCAL_ADDR", SqlDbType.NVarChar,15),
                            new SqlParameter("@HTTP_USER_AGENT", SqlDbType.NVarChar,255),
                            new SqlParameter("@HTTP_REFERER", SqlDbType.NVarChar,255)
                            };


            parms[0].Value = customerGuid;
            parms[1].Value = filename;
            parms[2].Value = HttpContext.Current.Request.ServerVariables["REMOTE_ADDR"];
            parms[3].Value = HttpContext.Current.Request.ServerVariables["LOCAL_ADDR"];
            parms[4].Value = HttpContext.Current.Request.ServerVariables["HTTP_USER_AGENT"];
            parms[5].Value = HttpContext.Current.Request.ServerVariables["HTTP_REFERER"];



            dr = sqlh.ExecuteReaderStoreProcedure("usp_InsertFileLocation", parms);

            context.Response.Clear();
            context.Response.ClearContent();
            context.Response.ClearHeaders();
            context.Response.AddHeader("Pragma", "no-cache");
            context.Response.AddHeader("Expires", "-1");

            context.Response.ContentType = "application/pdf";
            byte[] bytePDF = File.ReadAllBytes(filename);
            context.Response.BinaryWrite(bytePDF);
            context.Response.End();
        }
    }
}

Answer 1

The parms array could be initialized like this:

new SqlParameter("@REMOTE_ADDR", SqlDbType.NVarChar,15) { Value = HttpContext.Current.Request.ServerVariables["REMOTE_ADDR"] }

It gives minimal performance gain, cause you don't need to access the array members, and it becomes more robust against the order of parameters. You could add a new one on top, without the need to change the indexes of all the other ones.

This is useless

 SqlDataReader dr = null;

it can be done right here:

 var dr =  sqlh.ExecuteReaderStoreProcedure("usp_InsertFileLocation", parms);

It's still useless, you can omit the entire dr variable.

In general I cannot see what this routine is doing. Does it a Query or an Insert on the Database. It Executes a Reader, but there's something called "InsertFileLocation". So this need at least some comment. For a clear design you might divide Taking the Request and Storing something in two different routines. Cause you should be able to test it separately.

If you have a HttpContext as a Parameter, why do you access the static variable HttpContext.Current. It is probably the same reference, but it's not good habit to use a static workaround if you already have a local parameter with the same content.

Answer 2

for cleaner code, and to keep things dry and single responsibility. I would break it into various parts.

Settings HttpContextParser SqlPersistor ResponseWriter (this could be debatable depending on how it looks)

• Have a class that returns a parsed object/class from the the HttpContext with the data you require.
• Save it to the database.
• Return response

Actually i think it's a bad idea to process the response here. it should be processed in the Controller method or someplace else. the responsibility of this class should only be dealing with saving the user traffic data.

The response should be processed by another mechanism.

namespace DataCollection 
{
     public class SiteDataCollection
     {
        private IContextParser _contextParser;
        private IUserRepository _userRepository;

        public SiteDataCollection(
            IContextParser contextParser,
            IUserRepository userRepository)
        {
            _contextParser      = contextParser;
            _userRepository     = userRepository;
        }

        public async Task<FileInfo> ProcessRequest(HttpContext context)
        {
            var userTrafficInfo = _contextParser.GetUserTrafficInfo(context);

            await _userRepository.SaveTrafficInfoAsync(userTrafficInfo);

            return new FileInfo(userTrafficInfo.FileName);
        }
    }
}

Also if you are having some issues writing ado.net code, consider using something like dapper, you can still call stored procedures, but also direct insert statements and just pass a clr object and it should map correctly to the parameters. There are loads of examples online, and i think it would make your code a little more readable.