Method to Retrieve Data from a MySQL Database

Question

I have a method that retrieves information relating to a Contact that was made between two Companies. I am looking to improve my code in any way and am intrigued in hearing any recommendations you may have on how this method can be improved. I use it throughout my program but modify it according to the DataModel I am attempting to load.

string constr = ConfigurationManager.ConnectionStrings["sqlString"].ConnectionString;
using (var sqlCon = new MySqlConnection(constr))
{
    sqlCon.Open();
    using (var myQuery = sqlCon.CreateCommand())
    {
        myQuery.CommandText = @"SELECT DISTINCT contacts.contactID, contacts.companyID, 
        companies.name, contacts.donestatus, contacts.telephone, employees.ID, employees.name,
        people.ID, people.firstname, people.lastname, 
        contacts.contractID, contacts.date, contacts.time, 
        presets.presettext, contacts.enteredby, contacts.description
        FROM contacts
        LEFT OUTER JOIN companies ON contacts.companyID = companies.ID
        LEFT OUTER JOIN employees ON contacts.employeeID = employees.ID
        LEFT OUTER JOIN people ON contacts.personID = people.ID
        JOIN presets ON contacts.type = presets.presetIDFoxPro
        WHERE presets.presetreferencefoxpro = 8
        AND contacts.date > @date
        ORDER BY contacts.date DESC";
        myQuery.Parameters.AddWithValue("@date", date);
        using (var myReader = myQuery.ExecuteReader())
        {
            var listOfContacts = new ObservableCollection<ContactModel>();
            while (myReader.Read())
            {
                var details = new ContactModel
                {
                    ID = Convert.ToInt32(myReader[0]),
                    CompanyID = DBNullCheck(myReader[1]),
                    CompanyName = myReader[2].ToString().Trim(),
                    DoneStatus = Convert.ToInt32(myReader[3]),
                    Telephone = myReader[4].ToString().Trim(),
                    EmployeeID = DBNullCheck(myReader[5]),
                    EmployeeName = myReader[6].ToString().Trim(),
                    PersonID = DBNullCheck(myReader[7]),
                    PersonName = myReader[8].ToString().Trim() + " " + myReader[9].ToString().Trim(),
                    ContractID = DBNullCheck(myReader[10]),
                    Date = Convert.ToDateTime(myReader[11]),
                    Time = Convert.ToDateTime(myReader[12]),
                    TypeOfContact = myReader[13].ToString().Trim(),
                    EnteredBy = myReader[14].ToString().Trim(),
                    Notes = myReader[15].ToString(),
                };
                listOfContacts.Add(details);
            }
            return listOfContacts;
        }
    }
}

I would be happy if you could recommend improvements to my C# code or my MySQL statement alike.

woa that's a huge query with a lot of joins. Personally i don't like writing queries like that, i generally try to keep it shorter and simpler. but have you thought of using something like Entity Frameworks to minimise this sort of querying? — BenKoshy
– BenKoshy, Commented Oct 19, 2016 at 13:48

t3chb0t · Accepted Answer · 2016-10-26 07:30:51Z

2

In my review I assume you cannot use EF or alike...

I don't see any particular really bad practices here but there are a few minor things that make me uncomfortable when I look at your code. As the important things are correct (everything is disposed and you use parameters to prevent sql injection) there is not much left to review. But hey! this a good news ;-)

The my-prefix ;-) It doesn't hurt but it doesn't look professional either. You know everything there is yours so there's no need to emphasize it with my.

Try to name things by what they really are instead of giving them misleading names. If you do sqlCon.CreateCommand() then name the variable command or cmd not a query. It isn't one. Your sql is a query and it would be fine to name it like so.

Returning an ObservableCollection is a questionable practice. I suggest to use a List instead and update the real observable collection with the new data.

The listOfContacts. It's a collection and for collections we usually just use a plural noun. In this case just contants would be enough.

The reader[0] & magic numbers. You should use constants here like reader[Column.ID]. It looks much better now, doesn't it?

class Column
{
    public const int ID = 0;
    ...
}

PersonName = myReader[8].ToString().Trim() + " " + myReader[9].ToString().Trim(),

PersonName or FullName should be a readonly property of the ContactModel and it should have two normal properties for the first and last names. Your repository shouldn't care and know how to build a person name. It should just pass the data to the ContactModel.

answered Oct 26, 2016 at 7:30

t3chb0t

44.7k9 gold badges84 silver badges191 bronze badges

\$\begingroup\$ Thanks for your recommendations. The only place I am slightly confused is the use of constants that you suggest. Do I need to define a column class for every model of data I have? I.E, ContactCoulmn, PeopleColumn, CompanyColumn etc etc.. \$\endgroup\$

CBR
– CBR

2016-10-27 07:26:14 +00:00
Commented Oct 27, 2016 at 7:26
\$\begingroup\$ @CBR yes and no, I would add a nested Column class to each model and put the constants there. Then you could use it more fluently like Contact.Column.ID. \$\endgroup\$

t3chb0t
– t3chb0t

2016-10-27 07:30:18 +00:00
Commented Oct 27, 2016 at 7:30
\$\begingroup\$ Ok great. Also, whats the disadvantage of returning an ObservableCollection? Isn't it more time consuming to return a List, loop through the List and all of the Data to an ObservableCollection? \$\endgroup\$

CBR
– CBR

2016-10-27 08:02:19 +00:00
Commented Oct 27, 2016 at 8:02
\$\begingroup\$ @CBR actually you're right however it all depends on the design again. If you had an onbservable collection already bound to a view in WPF for example, then returning a new one could break this binding. If this is however the initialization and you don't recreate it later then it might work just fine. \$\endgroup\$

t3chb0t
– t3chb0t

2016-10-27 08:15:40 +00:00
Commented Oct 27, 2016 at 8:15

Add a comment |

Geek · Accepted Answer · 2016-10-26 13:05:15Z

I would look to improve this in several ways.

You're compiling the SQL into your code, so if you want to change the query, say for performance improvements, yet yield the same results you have to re-release your code. From a maintenance perspective and downtime it's not good. Look at Stored Procedures or some type of ORM's like EF, NHibernate, etc. With ORM's you have to take your education to the next level and learn about design patterns. Common one for EF is The Repository Pattern and Unit of Work. To keep things simple, look at Stored Procedures first.
On reading the data you are using numerical index references. Again looking at stored procedures you can refer to the column names and not worry about the order changing. Its a lot more safer than assuming that 0 or 1 would always be the same thing. If you are using a ORM then you dont have to mess about with a data reader.

Stack Exchange Network

Method to Retrieve Data from a MySQL Database

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Method to Retrieve Data from a MySQL Database

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions