Question Retrieve a value from DB based on the user login variable value

[andrewmanuja]

Hi,

Thanks for the above respond.

Now, my requirement is to retrieve a single string value from the database based on the variable value I collected at the user login point.

Basically, I want to get the "costCentreCode" which relates to individual user and pass it to a textbox.

My class looks as below;
public class UserData
{
public static string userName;
public static string costCentreCode;
}

I have written a function to get the "costCentreCode" as below;
private void CallCostCentre()
{
SqlCommand cmd = new SqlCommand("SELECT costCentreCode FROM tbl_UserData WHERE userName= UserData.userName ", con);
con.Open();
SqlDataReader dr = cmd.ExecuteReader();
string costCentreCode = null;
if(dr.Read ())
{
UserData.costCentreCode = (string)dr[0];
}
dr.Close();
con.Close();
}

Note
con refers to connection string
Please find my original post of the question on below link
"Question - Store user login data in to a Class and reuse within the application"

I need two favors;
1. Whether the function I have written on the respective window form is correct?
2. How to call the "costCentreCode" value using the function elsewhere of the program/ or at least on the same window form?

Thank you very much in advance for your favorable comments.

Look forward to hearing from you.

Kind regards,

Andrew

 

[Skydiver]

There are several things at play here, and I'm not quite sure where to start, so please forgive the rambling form that this response is going to take.

First, this isn't going to work:

"SELECT costCentreCode FROM tbl_UserData WHERE userName= UserData.userName "

because the SQL server (which likely is running on another machine) has no way of being able determine what the value of your UserData.userName variable. Please take time to read about the proper use of SQL Parameters. (Yes, you could use something like $"SELECT costCentreCode FROM tbl_UserData WHERE userName= {UserData.userName}". Do not do this. This sets you up for a SQL injection attack.)

In general, in object oriented programming, the use of singletons and global data should be avoided. A major reason for this is for the sake of testability of code, but other reasons are that it is much easier to reason with code when you are assured that nobody else is going to change the values underneath you. In your case, the members of your UserData class are essentially global variables, because anybody can access the public static members of that class at anytime. Imagine what would happen if there are two near simultaneous calls to your CallCostCentre() with UserName set to "John Doe" and another with UserData.UserName set to "Tarzan". There will be multiple permutations of which order the lines of code would be executed and most of those permutations will result in one or both of the callers getting unexpected results due to the use of globals and race conditions.

The method names should be descriptive. A better name for CallCostCentre() would be GetCostCentreForUser().

So to fix the various race conditions, you would re-write the method above as:

string GetCostCentreForUser(string user)
{
    string costCentreCode = null;
    :
    if (dr.Read())
    {
        costCentreCode = (string) dr["costCentreCode"];
    }
    :
    return costCentreCode
}

No more playing with globals.

So that still brings you back to the original problem of how do you get the data where you need it. You need some way of passing around the information about the user to your various forms. The way to do that change your UserData to not use static field variables, and then instantiate one of these objects and pass that object to everybody who needs it to accomplish their work. Let me reiterate: pass around that object to everybody who needs it to accomplish their work. It will be very tempting to create a singleton and just everybody access that singleton when they need it. This will take you back to previous case when you had global variables -- now you'll just have a single global variable pointing to the instance and then it'll be hard to reason about who changed what when. Take time to read about dependency injection. Avoid the service locator pattern.

So another question is where should GetCostCentreForUser() live? Take some time to read about 3-tier architectures, as well as the repository pattern. In general, you'll want that method to live in your data access layer or your data repository.

 

[andrewmanuja]

Hi,

Thank you very much for your valuable thoughts.

It's bit tricky at this stage for me to understand it entirely.

Now I feel the depth you are talking about and not so sure how I can overcome the said issues.

However, I'll read the areas as you suggested and will get back to you accordingly.

Thank you.

Kind regards,

Andrew

 

[jmcilhinney]

Please don't post the same question multiple times. I just responded to this question as a continuation of another thread and now I find it being discussed here too.

 

[andrewmanuja]

HI,

Sorry about that.

I wanted to post the question as a continuation of the previous post and thought since it was cleared, community will not see it unless specifically go inside the thread.

Also would like to know how to comment a post as answered?

Thank you.

Kind regards,

Andrew