IPAddress Range access

Question

I want to make this code better and easier to read. It works now but I think it can be better. When there is an Inbound request I check to see if that accessKey is setup for IP address Range that will return a bool flag. If its true I then need to get the IPStartAddress and IPEndAddress from the database. With that data and the inbound IP Address I can see if its in Range. Would be great if anyone can help me with this.

public static bool IsAuthenticated(string accessKey, string ipAddress)
 {
 bool IsRangeEnable = false;
 IsRangeEnable = IsAccessKeyRangeEnable(accessKey);
 if (IsRangeEnable == false)
 {
 bool canAccess = false;
 string conn = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
 SQLHelper sql = new SQLHelper(conn);
 SqlParameter[] parms = new SqlParameter[] {
 new SqlParameter("@AccessKey", SqlDbType.VarChar,36),
 new SqlParameter("@IPAddress",SqlDbType.VarChar,15)
 };
 parms[0].Value = accessKey;
 parms[1].Value = ipAddress;
 SqlDataReader dr = sql.ExecuteReaderStoreProcedure("dbo.[usp_Select_LoginAPI_AccessKey_IPAddress]", parms);
 while (dr.Read())
 {
 canAccess = Convert.ToBoolean(dr["IsValid"]);
 
 }
 dr.Close();
 return canAccess;
 }
 else
 {
 return CheckIPRange(accessKey, ipAddress);
 }
 }
 public static bool IsAccessKeyRangeEnable(string accessKey)
 {
 bool IsRangeEnable = false;
 string conn = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
 SQLHelper sql = new SQLHelper(conn);
 SqlParameter[] parms = new SqlParameter[] {
 new SqlParameter("@AccessKey", SqlDbType.VarChar,36),
 };
 parms[0].Value = accessKey;
 SqlDataReader dr = sql.ExecuteReaderStoreProcedure("dbo.[usp_Admin_Select_AccessKeyRangeEnable]", parms);
 while (dr.Read())
 {
 IsRangeEnable = Convert.ToBoolean(dr["ReturnValue"]);
 }
 dr.Close();
 return IsRangeEnable;
 }
 public static bool CheckIPRange(string accessKey, string ipAddress)
 {
 bool InRange = false;
 string conn = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
 SQLHelper sql = new SQLHelper(conn);
 SqlParameter[] parms = new SqlParameter[] {
 new SqlParameter("@AccessKey", SqlDbType.VarChar,36),
 };
 parms[0].Value = accessKey;
 SqlDataReader dr = sql.ExecuteReaderStoreProcedure("dbo.[usp_Admin_Select_IPRange]", parms);
 while (dr.Read())
 {
 InRange = IsInRange(Convert.ToString(dr["IPStartAddress"]), Convert.ToString(dr["IPEndAddress"]), ipAddress);
 }
 dr.Close();
 return InRange;
 }
 public static bool IsInRange(string startIpAddr, string endIpAddr, string address)
 {
 long ipStart = BitConverter.ToInt32(IPAddress.Parse(startIpAddr).GetAddressBytes().Reverse().ToArray(), 0);
 long ipEnd = BitConverter.ToInt32(IPAddress.Parse(endIpAddr).GetAddressBytes().Reverse().ToArray(), 0);
 long ip = BitConverter.ToInt32(IPAddress.Parse(address).GetAddressBytes().Reverse().ToArray(), 0);
 return ip >= ipStart && ip <= ipEnd;
 }

Answer 1

Quick remarks:

• There is no point in defining a variable and assigning it a value, only to override that value in the next line (IsRangeEnable in IsAuthenticated).

• Follow the Microsoft naming guidelines. Variable names should be camelCase (InRange, IsRangeEnable, ...).

• Don't use ADO.NET. Use something like Dapper. Using it would have made your code significantly shorter and easier to read. (I don't know what SQLHelper is; a Google search seems to suggest some ancient class that you definitely should not use.)

• Looking at your code you don't seem to properly dispose of your database connection.

• Don't pointlessly abbreviate. Look at conn: I'd expect a connection, instead it is a connection string. I also never would have guess that sql is a SQLHelper from its name.

• Why is the code inside if (IsRangeEnable == false) not in a method of its own?

Answer 2

Let me add some details

• It's better to keep your methods private unless you really need them to be public.
• You can put you connection string into a class field, that will reduce redundancy.
• You can remove redundancy in your IsInRange method if you extract parts containing BitConverter.ToInt32( into a new method.
• You don't need while (dr.Read()) if you want to return single result from your SP. Use if(dr.Read()) instead.
• Instead of using Convert.ToBoolean(dr["ReturnValue"]) you can use dr.GetBoolean("ReturnValue").
• Avoid writing a lot of static methods. Such code is less testable and usually harder to support (remember about possible DI usage in the future). You can find some thoughts about static methods in this question.
• You have two SP calls in your IsAuthenticated method. It could be not a very good design in terms of performance at least for web applications in case this method can be called pretty frequently.
• An approach with SPs here itself can lead to questions. At least in new applications, though it can be OK for legacy ones (in my humble opinion).

• Jefferson: Thanks how would I remove the two sp calls in the IsAuthenticated?