Bandwidth Reporter

My company is cheap but wanted a way to pinpoint bandwidth on certain computers. Me being a programming guy came up with the following small program. I just wanted some expert opinion on my code. I am especially interested in you opinion on how I usually pull permanent variables. I usually put these variables in a class and then call them from that class. That will be the class called ProgReferences. Be as brutal as you want. I want to improve.

Main Service

```
namespace YWIPSnitch
{
public partial class Service1 : ServiceBase
{
ProgReferences r = new ProgReferences();
MySqlConnection conn = new MySqlConnection();

public Service1()
{
InitializeComponent();
}

public void OnDebug()
{
OnStart(null);
}

protected override void OnStart(string[] args)
{
MonitorBandwidth();
}

protected override void OnStop()
{

}

private void SendRegister(string computerName, string username, char inOrOut)
{
conn.ConnectionString = r.getSQLConnString();

try
{
conn.Open();
using (MySqlCommand cmd = conn.CreateCommand())
{
cmd.CommandText = r.getRegisterInsertSQL();
cmd.Parameters.AddWithValue("@computer_name", computerName);
cmd.Parameters.AddWithValue("@username", username);
cmd.Parameters.AddWithValue("@register_type", inOrOut);

cmd.ExecuteNonQuery();
}
}
catch (MySqlException)
{
ReestablishDatabaseConnection();
}

conn.Close();
}

private void MonitorBandwidth()
{
NetworkInterface[] interfaces = NetworkInterface.GetAllNetworkInterfaces();

while (true)
{
forea

Your connection should also be inside of a using statement.

The way that I normally do it, I create the connection inside the using statement and not at a higher scope like you have done. I understand the reason you did what you did here, but not sure that it is necessary because you are only reusing the connection inside of 2 methods. I think it makes more work, but this is my personal opinion.

Your first method that uses the connection looks like this:

private void SendRegister(string computerName, string username, char inOrOut)
{
    conn.ConnectionString = r.getSQLConnString();

    try
    {
        conn.Open();
        using (MySqlCommand cmd = conn.CreateCommand())
        {
            cmd.CommandText = r.getRegisterInsertSQL();
            cmd.Parameters.AddWithValue("@computer_name", computerName);
            cmd.Parameters.AddWithValue("@username", username);
            cmd.Parameters.AddWithValue("@register_type", inOrOut);

            cmd.ExecuteNonQuery();
        }
    }
    catch (MySqlException)
    {
        ReestablishDatabaseConnection();
    }

    conn.Close();
}

I would do it like this:

private void SendRegister(string computerName, string username, char inOrOut)
{

    using (MySqlConnection conn = new MySqlConnection())
    {
        conn.ConnectionString = r.getSQLConnString();
        try
        {
            conn.Open();
            using (MySqlCommand cmd = conn.CreateCommand())
            {
                cmd.CommandText = r.getRegisterInsertSQL();
                cmd.Parameters.AddWithValue("@computer_name", computerName);
                cmd.Parameters.AddWithValue("@username", username);
                cmd.Parameters.AddWithValue("@register_type", inOrOut);

                cmd.ExecuteNonQuery();
            }
        }
        catch (MySqlException)
        {
            ReestablishDatabaseConnection();
        }
    }         
}

I was taking a look at your ReestablishDatabaseConnection method and was a bit confused.

The method name doesn't really reflect what you are doing here:

private void ReestablishDatabaseConnection()
{
    bool broken = true;
    conn.ConnectionString = r.getSQLConnString();

    while (broken)
    {
        System.Threading.Thread.Sleep(30000);
        try
        {
            conn.Open();
            conn.Close();
            broken = false;
        }
        catch (MySqlException)
        {

        }
    }
}

In the first bit of code that I reviewed, your catch statement sends your code here and then the exception never bubbles up so that you can see what is happening; if you hit the wrong kind of exception, you will be sitting here for a very long time waiting for the code to "fix itself" and it never will because it will be stuck in the while (broken) loop

Get rid of this and let the exceptions bubble up, then fix them. Or even better, you should log these exceptions so that you can find the exceptions somewhere and allow the service to continue running. It looks like you already have a database location; I would just set up a table that you could insert these exceptions into, and then dump the table after a set period of time or have old records "fall off the table".

StackExchange Code Review Q#149197, answer score: 5