current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I have been advised to submit my code here by a fellow Stack contributer as it was suggested the code could be further improved; Calling Code and attempt at building session variables:

DataTable dtServerVars = clCommonFunctions.TryAutoLogin("europe\\MrTest");
Session["CablingUserID"]= dtServerVars.Rows[0]["CablingUserID"].ToString();
Session["CablingUseremail"] = dtServerVars.Rows[0]["CablingUseremail"].ToString();
Session["CablingLogin"] = dtServerVars.Rows[0]["CablingLogin"].ToString();
Session["CablingPassword"] = dtServerVars.Rows[0]["CablingPassword"].ToString();
Session["CablingPersonnel"] = dtServerVars.Rows[0]["CablingPersonnel"].ToString();
Session["CablingSurname"] = dtServerVars.Rows[0]["CablingSurname"].ToString();
Session["CablingFirstName"] = dtServerVars.Rows[0]["CablingFirstName"].ToString();
Session["CablingSuperUser"] = dtServerVars.Rows[0]["CablingSuperUser"].ToString();
Session["CablingDateAdded"] = dtServerVars.Rows[0]["CablingDateAdded"].ToString();
Session["CablingContact"] = dtServerVars.Rows[0]["CablingContact"].ToString();
Session["CablingApprovalAuthority"] = dtServerVars.Rows[0]["CablingApprovalAuthority"].ToString();
Session["CablingAdminUser"] = dtServerVars.Rows[0]["CablingAdminUser"].ToString();
Session["SharedInfoID"] = dtServerVars.Rows[0]["SharedInfoID"].ToString();
Session["SharedInfousername"] = dtServerVars.Rows[0]["SharedInfousername"].ToString();
Session["SharedInfopassword"] = dtServerVars.Rows[0]["SharedInfopassword"].ToString();
Session["SharedInfoname"] = dtServerVars.Rows[0]["SharedInfoname"].ToString();
Session["SharedInfoemail"] = dtServerVars.Rows[0]["SharedInfoemail"].ToString();
Session["SharedInfoICLlocation"] = dtServerVars.Rows[0]["SharedInfoID"].ToString();
Session["SharedInfoPhone"] = dtServerVars.Rows[0]["SharedInfoPhone"].ToString();
Session["SharedInfoSecLevel"] = dtServerVars.Rows[0]["SharedInfoSecLevel"].ToString();
Session["IMSUserID"] = dtServerVars.Rows[0]["IMSUserID"].ToString();
Session["IMSUserName"] = dtServerVars.Rows[0]["IMSUserName"].ToString();
Session["IMSIsAnonymous"] = dtServerVars.Rows[0]["IMSIsAnonymous"].ToString();
Session["IMSLastActivityDate"] = dtServerVars.Rows[0]["IMSLastActivityDate"].ToString();
Session["loggedin"] = "unknown";

Code Being Called:

public static DataTable TryAutoLogin(string strREMOTE_USER)
{
    SqlConnection siConnection = new SqlConnection();
    siConnection.ConnectionString = Databases.getDbConnectionString("csSharedInfo");
    siConnection.Open();
    SqlCommand seCmd = new SqlCommand("GetSignOnDetails", siConnection);
    seCmd.CommandType = CommandType.StoredProcedure;
    seCmd.Parameters.Add(new SqlParameter("@DomainAccount", SqlDbType.NVarChar, 300));
    seCmd.Parameters["@DomainAccount"].Value = strREMOTE_USER;
    seCmd.CommandType = CommandType.StoredProcedure;
    SqlDataAdapter sda = new SqlDataAdapter();
    seCmd.Connection = siConnection;
    sda.SelectCommand = seCmd;
    DataTable dtServerVars = new DataTable();
    sda.Fill(dtServerVars);
    siConnection.Close();
    if (dtServerVars != null)
    {
        if (dtServerVars.Rows.Count > 0)
    {
        return dtServerVars;
    }
}
    return null;
}
share|improve this question

2 Answers 2

up vote 1 down vote accepted

First your TryAutoLogin:

  • A SqlConnection is IDisposable. Use it in combination with using.
  • You have some duplicate lines of code, like seCmd.CommandType = CommandType.StoredProcedure;.
  • dtServerVars can never be null.
  • You do not have to open the connection, SqlDataAdapter will do that for you.
  • Since you are using only ONE datarow, just return one datarow.

Resulting in:

 public static DataRow TryAutoLogin(string strREMOTE_USER)
 {
      using(SqlConnection siConnection = new SqlConnection(Databases.getDbConnectionString("csSharedInfo")))
       {
           SqlCommand seCmd = new SqlCommand("GetSignOnDetails", siConnection);
           seCmd.CommandType = CommandType.StoredProcedure;
           seCmd.Parameters.AddWithValue("@DomainAccount", strREMOTE_USER);
           SqlDataAdapter sda = new SqlDataAdapter(seCmd);
           DataTable dtServerVars = new DataTable();
           sda.Fill(dtServerVars);
           if (dtServerVars.Rows.Count > 0)
                return dtServerVars.Rows[0];
           return null;
      }
 }

Having done this, the next piece of code will also be a lot easier:

DataRow drServerVars = clCommonFunctions.TryAutoLogin("europe\\MrTest");
Session["CablingUserID"]= drServerVars["CablingUserID"].ToString();
Session["CablingUseremail"] = drServerVars["CablingUseremail"].ToString();
Session["CablingLogin"] = drServerVars["CablingLogin"].ToString();
...
share|improve this answer
    
You could simplify if (dtServerVars.Rows.Count > 0) { return dtServerVars.Rows[0]; } return null; to just: return dtServerVars.AsEnumerable().FirstOrDefault(). You would need to add a using for System.Data.DataSetExtensions. –  RobH May 1 '13 at 10:31
    
RobH: True! But MoiD101 is still learning a lot of stuff, I was not willing in introducing him into new techniques like LINQ, but just showing him how he can improve his use on the components he is already using. –  Martin Mulder May 1 '13 at 10:34
    
Thank You Martin and thank you RobH for your contribution, as Martin commented though i have gota learn to walk first ;) –  MoiD101 May 1 '13 at 10:49
    
Martin, just one question, the if statement below, is that some sort of short hand can i go read about it somewhere it seems very forein to me thats all... –  MoiD101 May 1 '13 at 11:00
1  
@RobH+MoiD101: RobH is correct. It is a personal chois with pro's and con's. It can lead to mistakes if your coding style (and how you implement it) is not consistent. –  Martin Mulder May 1 '13 at 12:13
up vote 1 down vote

The part where you set session properties could be simplified a lot. If you want to get all columns from the DataTable into Session, then you can use the Columns collection. Something like:

DataTable dtServerVars = clCommonFunctions.TryAutoLogin("europe\\MrTest");

foreach (DataColumn column in dtServerVars.Columns)
{
    Session[column.ColumnName] = dtServerVars.Rows[0][column].ToString();
}

Though it seems some of your column names are different in Session. To do that, you could use a helper method:

private static string TranslateColumnName(string dataTableColumnName)
{
    switch (dataTableColumnName)
    {
    case "SharedInfoID":
        return "SharedInfoICLlocation";
    default:
        return dataTableColumnName;
    }
}

Session[TranslateColumnName(column.ColumnName)] = dtServerVars.Rows[0][column].ToString();

Also, if you know that all the columns are strings, I would use the Field() extension method instead of ToString(). That's because it handles DBNull properly and will throw an exception if the data in the column is actually a different type.

Session[TranslateColumnName(column.ColumnName)] = dtServerVars.Rows[0].Field<string>(column);
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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