Answered ExecuteReader not working

[PDS8475]

I have a database table called Sites with to columns SiteName and SiteAddress
The following code is on a button click event.
I have removed the database password from the connection string intentionally before posting.
When I step through the code, I can see the connection state opens.
However the reader remains null and when the ExecuteReader is called it jumps to the catch block.
the catch block gives the exception message "Failed due to No value given for one or more required parameters"

It appears to me that the query is wrong and therefore the reader can't read, But I can not see how it is wrong. I tried copying and pasting the names of the Table and columns into the query to make sure that I have them exactly right, I've also checked with other update queries online and it seems to be right.
Could anybody please tell me what is going wrong as I have spent two days on this, trying different things and I am getting no where.

            string sn = SiteName_textBox.Text;
            string sa = SiteAddress_textBox.Text;
            try
            {
               OleDbDataReader read;
               OleDbConnection connection = new OleDbConnection(@"Provider = Microsoft.Jet.OLEDB.4.0; Data Source = |DataDirectory|\Admins.mdb; Jet OLEDB:Database Password = ");
               
               string my_query = "UPDATE Sites SET SiteAddress = @sa WHERE SiteName = @sn";
               OleDbCommand comm = new OleDbCommand(my_query, connection);
               connection.Open();
               read = comm.ExecuteReader();
               if (read.HasRows)
               {
                   while (read.Read())
                   {
                      comm.Parameters.AddWithValue("@sn", SiteName_textBox.Text);
                      comm.ExecuteNonQuery();
                   }
               }
               read.Close();
               connection.Close();
            }
            catch (Exception ex)
            {
                MessageBox.Show("Failed due to " + ex.Message);
            }

 

[jmcilhinney]

Your code makes no sense. As you'd expect, the ExecuteReader method is used to read data, which requires a SELECT statement. If that's not what you're doing then don't call that method. If you want execute an UPDATE statement then just call ExecuteNonQuery. There's nothing to read so don;t try to read anything.

 

[PDS8475]

Your code makes no sense. As you'd expect, the ExecuteReader method is used to read data, which requires a SELECT statement. If that's not what you're doing then don't call that method. If you want execute an UPDATE statement then just call ExecuteNonQuery. There's nothing to read so don;t try to read anything.

If I have

            string sn = SiteName_textBox.Text;
            string sa = SiteAddress_textBox.Text;
            try
            {
               OleDbConnection connection = new OleDbConnection(@"Provider = Microsoft.Jet.OLEDB.4.0; Data Source = |DataDirectory|\Admins.mdb; Jet OLEDB:Database Password = ");
               string my_query = "UPDATE Sites SET SiteAddress = @sa WHERE SiteName = @sn";
               OleDbCommand comm = new OleDbCommand(my_query, connection);
               connection.Open();
               comm.Parameters.AddWithValue("@sn", SiteName_textBox.Text);
               comm.Parameters.AddWithValue("@sa", SiteAddress_textBox.Text);
               comm.ExecuteNonQuery();
               connection.Close();
            }
            catch (Exception ex)
            {
                MessageBox.Show("Failed due to " + ex.Message);
            }

in other words take everything pertaining to the reader out. I don't get an exception error but the table record still doesn't get updated.

 

[Skydiver]

From the OleDbParameter documentation says:

The OLE DB.NET Framework Data Provider uses positional parameters that are marked with a question mark (?) instead of named parameters.

Ironically, the sample code in the documentation uses @name rather than the positional ?.

 

[Skydiver]

From a 2008 blogpost, apparently the order matters. Try swapping your lines 9 and 10 so that you add the parameters in the same order as found in the query.

 

[PDS8475]

Thank you to both of you. I didn't realise the order of the parameters mattered.

 

[Skydiver]

The order shouldn't matter for most sane databases. Unfortunately, Microsoft has to keep Access alive and they do the best that they can given how old Access is, as well as the vagaries of the OLEDB adapter for Access. OLEDB lost the ODBC vs OLEDB war.

If you don't really need an Access database, consider SQLite or SQL Express.

 

[jmcilhinney]

By the way, you should be closing your connection in a finally block rather than in the try block. As it is, your connection will remain open if an exception is thrown. Better still, create it with a using statement and then it is guaranteed to be closed at the end of the block, even if an exception is thrown and not caught.

 

[NoUserHere]

Just for fun, I wanted to contribute a point of interest and have a healthy discussion on : try/catch + using, right syntax and hopefully the comments will also help our op determine best practice. I know we all have our own preferences, and so I wonder what your thoughts on what the syntax usage should be. I'll bring the question into the topic for convenience, and add finally statement for completeness since it wasn't used in the original question. :
Which one

            try
            {
                using (var myObject = new MyClass())
                {
                    // something here...
                }
            }
            catch (Exception ex)
            {
                // Handle exception

            }
            finally
            {
                // Finish up
            }

OR

            using (var myObject = new MyClass())
            {
                try
                {
                    // something here...
                }
                catch (Exception ex)
                {
                    // Handle exception
                }
                finally
                {
                    //Finish up
                }
            }

Take note there is no mention of finally in the original topic, so I've added them here. Should using come before try, or after it? I mostly find myself following the first principle block as it has served me well, but I can see the reasoning behind both the arguments made on that topic. What are your thoughts? Chime in don't be shy

 

[Skydiver]

I tend to write code using the former mostly as a carry over from my C++ days.

Before C# 7.0, I would write things like this:

public IEnumerable<Record> DoSomething(int someParam, string otherParam)
{
    try
    {
        return DoSomethingReally(someParam, otherParam);
    }
    catch (SomeException ex)
    {
        Log.Error(ex);
        // throw or return empty depending on if error is recoverable.
    }
}

IEnumerable<Record> DoSomethingReally(int someParam, string otherParam)
{
    using (var someObj = new Some(someParam))
    using (var otherObj = new Other(otherParam))
    {
        // do things here
    }
}

With C# 7.0, I've been using this style more and more:

public IEnumerable<Record> DoSomething(int someParam, string otherParam)
{
    try
    {
        return DoSomethingReally();
    }
    catch (SomeException ex)
    {
        Log.Error(ex);
        // throw or return empty depending on if error is recoverable.
    }

    IEnumerable<Record> DoSomethingReally()
    {
        using (var someObj = new Some(someParam))
        using (var otherObj = new Other(otherParam))
        {
            // do things here
        }
    }
}

It's very likely with C# 8.0, I'll even transition to this style:

public IEnumerable<Record> DoSomething(int someParam, string otherParam)
{
    try
    {
        return DoSomethingReally();
    }
    catch (SomeException ex)
    {
        Log.Error(ex);
        // throw or return empty depending on if error is recoverable.
    }

    IEnumerable<Record> DoSomethingReally()
    {
        using var someObj = new Some(someParam);
        using var otherObj = new Other(otherParam);
        // do things here
    }
}

OR if the DoSomethingReally() is really short, then:

public IEnumerable<Record> DoSomething(int someParam, string otherParam)
{
    try
    {
        using var someObj = new Some(someParam);
        using var otherObj = new Other(otherParam);
        // do things here
    }
    catch (SomeException ex)
    {
        Log.Error(ex);
        // throw or return empty depending on if error is recoverable.
    }
}

Anyway, the point is that I try to keep all the "do something" code focused on doing the operation along the happy path. Any errors or exception handling is outside of that happy path and is handled elsewhere. I broke out the DoSomethingReally() to try to reduce the indentation depth, although admittedly the local methods of C# 7.0 doesn't really help much in that regard.

 

[jmcilhinney]

I would tend to keep the code inside a try block to only that that can throw an exception. Unless a constructor can reasonably throw an exception, I would have the using outside the try.