0
\$\begingroup\$

I am no expert in C#, the opposite really, I just wanted an overall review on my UserInfo class that gets user information from a database using MySQL. I was also wondering if it is a good idea to really process the data in the way I did, getting everything from the database, even the things that aren't needed. But I think this way is a lot more tidy and neat. I was just wondering what you would say about it.

using MySql.Data.MySqlClient;
using System;
using System.Collections.Generic;
using System.Data;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Scratch.Application.Habbo.Users
{
    class UserInfo
    {
        private Dictionary<string, string> _userInfo;

        public UserInfo(int Id)
        {
            this._userInfo = new Dictionary<string, string>();

            using (var DbCon = Scratch.GetEnvironment().GetDatabase().GenerateNewConnection())
            {
                DbCon.AssignQuery("SELECT * FROM `users` WHERE `id` = @id");
                DbCon.RegisterParameter("id", Id);
                DbCon.OpenConnection();

                using (MySqlDataReader Reader = DbCon.ExecuteReader())
                {
                    if (Reader.HasRows)
                    {
                        while (Reader.Read())
                        {
                            for (int index = 0; index < Reader.FieldCount; index++)
                            {
                                //bypass duplicate error, just replaces if duplicate
                                this._userInfo[Reader.GetName(index)] = Reader.GetString(index);
                            }
                        }
                    }
                }
            }
        }

        private void UpdateData(string Key, string NewValue)
        {
            if (this._userInfo.ContainsKey(Key))
                this._userInfo.Add(Key, "");

            this._userInfo[Key] = NewValue;
        }

        public string GetString(string Key)
        {
            return this._userInfo[Key];
        }

        public int GetInt(string Key)
        {
            return int.Parse(this._userInfo[Key]);
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

Why do you do this:

if (this._userInfo.ContainsKey(Key))
    this._userInfo.Add(Key, "");

this._userInfo[Key] = NewValue;

Those first two lines are completely superfluous; moreover if you need to get a value from a Dictionary<T, T> and aren't sure the key is present, you need to use Dictionary<TKey, TValue>.TryGetValue.


Local variables like Reader need to be camelCase. Ditto parameters like Key and NewValue.


Why aren't you using an ORM like Entity Framework instead of this ugly, needlessly generic code? I mean: public string GetString(string Key), public int GetInt(string Key) -- really?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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