3

I was getting this error: "Input string was not in a correct format."

Here is my Code:

    private void UpdatePOdetailBalance(int Qty)
    {
        int newbal;

        SqlCommand com = new SqlCommand();

        com.Connection = cn;

        newbal = Convert.ToInt16(txtQtyOrdered.Text) - Qty;
        com.CommandText =
            "UPDATE PODetail SET BalanceQty="+ newbal +" WHERE OrderID=" +
             Convert.ToInt16(txtPONumber.Text) + "AND ItemID=" +
             Convert.ToInt16(txtItemNo.Text);


        com.ExecuteNonQuery();

    }

    private void btnOK_Click(object sender, EventArgs e)
    {

            UpdatePOdetailBalance(Convert.ToInt16(txtQuantity.Text));

    }

I want to compute the newbal which is equal to txtQtyOrdered minus Qty but i'm getting this error please help me with this. Thanks.

Improve this question
1
  • 1
    Please use using-blocks around your commands and learn to use bind parameters. For the short term, print your sql string, you are missing a blank in the right place. Commented Oct 12, 2013 at 15:03

7 Answers 7

Reset to default
6

The problem stated by your error message is probably on one of the lines that try to convert the value in the textboxes to a short integer. Without any check, the value typed by your user could be anything but a number and you get this error message (for example, if you user leaves the textboxes empty).

You should try to check if the textboxes content could be converted to a valid short integer using TryParse before attempting to execute the query

int ordered;
if(!int16.TryParse(txtQtyOrdered.Text, out ordered))
{
    MessageBox.Show("Invalid number for Ordered quantity");
    return;
}
int orderID;
if(!int16.TryParse(txtPONumber.Text, out orderID))
{
    MessageBox.Show("Invalid number for OrderId");
    return;
}
int itemID;
if(!int16.TryParse(txtItemNo.Text, out itemID))
{
    MessageBox.Show("Invalid number for ItemID");
    return;
}

At this point you could execute your calculation using the converted short integers and then write your query in this way (adding a space before the AND)

  com.CommandText =
        "UPDATE PODetail SET BalanceQty="+ newbal.ToString() +
        " WHERE OrderID=" + orderID.ToString() + 
        " AND ItemID=" + itemID.ToString();

But the string concatenation of query text and user input is never advised as a good practice (in your case is harmless because if the conversion is successful you don't have to worry about Sql Injection, but don't take the habit to do it).
So the perfect way to write this query is through the use of a parametrized query

  com.CommandText =
        "UPDATE PODetail SET BalanceQty=@newbal " +
        " WHERE OrderID=@orderID " + 
        " AND ItemID= @itemID"

  com.Parameters.AddWithValue("@newbal", newBal);
  com.Parameters.AddWithValue("@orderID", orderID);
  com.Parameters.AddWithValue("@itemID", itemID);
  com.ExecuteNonQuery();

As a good article on Parameterized query and why to use them, I suggest to read these old words from Jeff Atwood

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

2

You need to put a space before your "AND" and that you are trying to convert a string to an integer that isn't an integer.

Improve this answer

5 Comments

And that's it? What about saying something about the bad practices been used here?
Yes there's bad practices, but he is apparently a young programmer learning, so instead of breaking someone down by pointing out bad practices, why not help him get through his immediate needs and then he can learn about good/bad practices.
No. One line isn't a proper answer in this case. You can point out bad practices without breaking someone down!!
@Yosi Agreed, you can help people learn with out breaking them down. My answer was a quick one while cooking breakfast for the family. There are other well thought out in depth answers on this thread. Thank you for your comments. I will in the future be more thoughtful on my answers.
Sorry if I was to harsh. This is a question asked in meta yesterday and relates to this discussion- meta.stackexchange.com/questions/200420/…
2

I'd recommend making changes according to the following code review suggestions based on the code (listed in order of value (cost/benefit of "fixing")):

  1. This method, which is accessing a database should not be reading controls to get its values. Instead there should be an event handler, such as a button click, that parses the values of other controls, using TryParse, as gregjer answered. By segregating the UI and Data code, the data access layer is easier to test and by parsing at the surface (the UI layer) exceptions dealing with bad user input will be caught as soon as possible.
  2. Dynamic SQL via strings in the database or in the data access layer w/i .NET is open to SQL injection. You are resolving that issue by parsing the text, so awesome job by you. BUT, this was already handled by the .NET team by providing parameterized commands. Refer to the MSDN SqlCommand.Parameters or see here for a brief, including how a consuming developer groks this topic: When should "SqlDbType" and "size" be used when adding SqlCommand Parameters?
  3. Variable naming. Instead of Qty, standard .NET naming conventions would call for quantity, camelCased since it is a parameter and the full human language name, not a shorthand or abbreviation, especially for publicly visible bits. IntelliSense makes long variable names not a problem. Since .NET is unwieldy using just Notepad, it should be assumed that other developers are using an IDE such as VisualStudio or SharpDevelop, so use meaningful names.
  4. Stored procedures should be used. Every time this SQL is executed, SQL Server needs to check its command cache minimally, but if the command has been flushed from cache, the SQL command needs to be interpreted and encached (put into cache). This as well as the fact that using a stored procedure requires "shipping" less bytes on every call to the database.
Improve this answer

1 Comment

His code has nothing to do Dynamic SQL (it's open to SQL injection from another reason)
0

That error means that the string you're trying to convert is not an integer. Try to use int.TryParse

int newbal;

if(int.TryParse(txtQtyOrdered.Text, out newbal))
    newbal = newbal - Qty;

the same with other texts you are trying to convert

... and add space before " AND which will generate next error

Improve this answer

Comments

0

I think you need to debug your code. During debugging copy your query from "com.CommandText" and paste in SQL Server you find the error

There is only a query error nothing else... May be txtQtyOrdered value is not integer, there is also need blank space "AND ItemID=" to " AND ItemID="

Thanks,

Taha

Improve this answer

Comments

0

First - You are missing a space before "AND"

  1. You should try to parse the values before the update statement.
  2. You should decide what you want to do in case the input from the textbox wasn't in the correct format rather then just get an exception when you try to update.
  3. This isn't the right way to format strings, You should use string.Format
Improve this answer

1 Comment

You are correct in that string.format is better than string concatenation, however when dealing with query statements and using the command object, it is always better to use parameters.
0

you can sometimes run into this problem when you have multiple parameters and are using Oracle or DB2 databases. They dont's support named parameters or it's not turned on.

Oracle:

    Dim cmd As OracleCommand = DirectCast(connection.CreateCommand, OracleCommand)
    cmd.BindByName = True

Make sure you parameters are added to the command object in the same order as the sql statement

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.