1
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ and your question please? \$\endgroup\$
    – iSR5
    Dec 12, 2019 at 19:58
  • \$\begingroup\$ I was just trying to improve working code seeing if there was any potential issues or performance problems \$\endgroup\$
    – Jefferson
    Dec 12, 2019 at 20:02
  • \$\begingroup\$ @iSR5 Posts on CodeReview always have the implied question of "Will you review the syntax, structure, and logic of my code?" \$\endgroup\$
    – Rick Davin
    Dec 12, 2019 at 20:17
  • \$\begingroup\$ This is a terrible idea, the iis logs have all information already. \$\endgroup\$
    – jimjim
    Dec 12, 2019 at 22:47
  • \$\begingroup\$ But I wanted the information in the database \$\endgroup\$
    – Jefferson
    Dec 12, 2019 at 22:52

2 Answers 2

1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ thanks this is doing both Query and Insert how should I access the static variable ? \$\endgroup\$
    – Jefferson
    Dec 12, 2019 at 22:13
  • \$\begingroup\$ You have the variable here: ProcessRequest(HttpContext context)you don't need that static thing. And it's not a query, cause there is nothing coming out. You don't read anything. Don't tell me, make your code tell the story. \$\endgroup\$
    – Holger
    Dec 13, 2019 at 0:02
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Would I need an interface IUserRepository and a model for HttpContext ? \$\endgroup\$
    – Jefferson
    Dec 13, 2019 at 20:25
  • \$\begingroup\$ that would be the most readable version of it. you could use concrete classes but you don't want to leak any implementation details. \$\endgroup\$ Dec 16, 2019 at 16:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.