How to get a value to return in a textbox?

Question

The program is to supposed count the refurb in the db and return a value in a textbox. I initialized my values to 0, but my problem is how do I get it to calculate each time a user scans in a unit to be refurbed. Anything I try to do gives me an error in my program. Right now it's returning a value of of 1 because I added RefurbRate++. Can anyone help me?

Here is what I've done with my code:

Collapse

private int GetRefurbRate()
{
     string sql = "";
     int Refurb_Rate = 0;
     int totalRefurb = 0;
     int totalUnits = 0;
     string error_msg = "";

     sql = "SELECT COUNT(*) " +
            "FROM " + schema + ".repair_part rp  " +
            "WHERE rp.repair_ord = '" + txtRO.Text + "' ";
     while (true)
     {
          if (!myDb.RunSql(sql, true))
          {
             error_msg = "DBError for getting Refurb Rate";
             break;
          }
          if (myDb.dbRdr.HasRows)
          {
              if (myDb.dbRdr.Read())
              {
                 try
                 {
                     Refurb_Rate = (totalRefurb / totalUnits * 100);
                 }
                 catch (Exception e)
                 {
                      Console.WriteLine(e);
                 }
               }

              Refurb_Rate++;
          }
          break;
     }
     myDb.dbRdr.Close();
     if (error_msg != String.Empty)
     {
         MessageBox.Show(error_msg, "Get Refurb Rate",
                          MessageBoxButtons.OK, MessageBoxIcon.Asterisk);
     }
     return Refurb_Rate;
}

Answer 1

1. You're using a field (myDb) to hold a custom database interface object. This is bad because it does not compose well. Multiple methods using the same object may interfere with each other. It is difficult to reason about how it is used, because the entire program must be considered. This sort of persistant state should be avoided when possible. A better approach is to pass the stateful object to the methods that need it without referencing it in a field. Even better would be to abandon the interface completely, and just use the database objects provided by the .NET framekwork.

   1. You are dynamically creating a SQL statement as a string. This is bad because it opens you up to SQL injection attacks (especially when the contents of a textbox are pasted into the string! " '; DELETE FROM repair_ord; -- "). A better approach would be to use a stored procedure, or, at the vary least, to use named arguments in your statement. Clearly, this was done to change the schemas (to change environments?). Switching between whole databases would be safer.

   2. You have the database reading code in a while loop with a constant true controlling expression, which serves no purpose since every flow of control inside the loop leads to a break statement. If this is to create an implied goto at the end of the first if statement, then there are better ways to do this (like an else). The while statement and the break statements should be removed.

   3. You do not appear to be using the value returned from the select statement anywhere.

   4. The variables totalRefurb and totalUnits are never set to anything other than zero. Therefore, the division will always throw a DivisionByZeroException. Therefore, the only time Refurb_Rate is modified from zero is when it is incremented at the end. So it will always end with a value of 1.

   5. The try/catch is completely unnecessary. It was obviously placed there to handle the DivisionByZeroException, but it would be better to avoid the exception by checking for totalUnits = 0 before doing the division.

   6. A catch statement should never catch the general Exception object. Instead the specific exception that you are expecting should be caught and handled if it cannot be avoided, and if you know how to handle it.

   7. Stylistically, it is best to limit the amount of program text in which a variable is used to be as small as possible. Therefore, it would be better to declare the sql variable on the same line in which it is initialized. Also there is no reason to initialize it twice, as you are doing when you set it equal to the empty string at the beginning.

Here is another version of your program:

private static int GetRefurbRate(string repairOrd, int totalUnits, string connection, string schema)
{
    if (string.IsNullOrEmpty(repairOrd))
        return 0;
    if (totalUnits == 0)
        return 0;
    if (string.IsNullOrEmpty(connection))
        throw new ArgumentException("Missing database connection for getting the refurb rate.");
    if (string.IsNullOrEmpty(schema))
        throw new ArgumentException("Missing schema for getting the refurb rate.");

    string sql = string.Format(@"
             SELECT COUNT(*) AS TotalRefurb
             FROM {0}.repair_part rp 
             WHERE rp.repair_ord = @repair_ord
         ", schema);

    int totalRefurb;
    using (SqlConnection conn = new SqlConnection(connection))
    {
        conn.Open();
        using (SqlCommand comm = new SqlCommand(sql, conn))
        {
            comm.CommandType = CommandType.Text;
            comm.Parameters.AddWithValue("@repair_ord", repairOrd);
            using (SqlDataReader reader = comm.ExecuteReader())
            {
                reader.Read();
                totalRefurb = (int)reader["TotalRefurb"];
            }
        }
    }
    return totalRefurb / totalUnits * 100;
}

Answer 2

There are several issues you need to address before you can solve this poroblem.

First and most important is that your application is vulnerable to SQL Injection. This will allow bad people to do bad things to your database. Read up on parameterized queries to protect against this.

Secondly, I don't know what myDb.RunSql(sql, true) does, but it seems like you are not using the standard methods for querying data from a database using C#. Typically you want to use the following:

• SQLConnection
• SQLCommand
• SQLDataReader

If you use these it will be easier for us to tell you what you are doing wrong. If you roll your own things will be more difficult.

If you look at the MSDN's code example for SQLDataReader it will show you how to use these three classes to connect to a database, execute a command and process the results. Link here:

http://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqldatareader.aspx

Answer 3

Wild guess, you have result always equal to zero because you're dividing int numbers here:

totalRefurb / totalUnits * 100

so if totalRefurb is less than totalUnits and greater than zero then result will always be zero because they are divided in 'integer' way. You should cast one of them to double:

double refurb_Rate = ((double)totalRefurb / totalUnits * 100);

Of course we should also mention that the code is written in a way which makes many other errors very possible.

Answer 4

The error comes from a division by zero error with totalUnits. It's always zero. When the error is thrown it's eaten by your catch block which does nothing to report the error to callers. This allows the method to finish and will always return 1.