efficient way to apply operator on specific record in MsAccess Database

Question

    public void moveBooks(int quantityOfMovedBooks, int booksID)
    {
        int finalQuantityOfBooks = totalBooksInDB(booksID) - quantityOfMovedBooks;
        queryString = "update Books set bQuantity='" + finalQuantityOfBooks + "'where bID=" + booksID;
        myComm = new OleDbCommand(queryString, myConn);
        myConn.Open();
        myComm.ExecuteNonQuery();
        myConn.Close();
    }
    public int totalBooksInDB(int bID)
    {
        int booksQuantity;
        queryString = "select bQuantity from Books where bID=" + bID;
        myComm = new OleDbCommand(queryString, myConn);
        myConn.Open();
        booksQuantity = (int)myComm.ExecuteScalar();
        myConn.Close();
        return booksQuantity;
    }

im beginner in MSAccess Database and C#, im maintaing a Table in which there are 3 fields one is BookID, second is BookName, third is BookQuantity.. scope is when books are moved to aisle books should be subtracted from main inventory.. im using this approach.. but i wonder is there any better or efficient way of doing this.. thanx in advance

when you run this are you first of all, getting / yielding the expected results..? if not please state the problem. `Also to insure that the newly created objects are being properly Disposed of` `Wrap the code in the following `1st a try{}catch{} and within the try{} implement the `using(){}` construct also I know that it's `Microsoft Crap-Cess` but I would use `Parameters` it would make reading your query a lot easier — MethodMan, Apr 04 '13 at 19:09
ok i got you.. thanx for the advice.. but my Question is still there.. — rummykhan, Apr 04 '13 at 19:13
you can make it work better by updating to a real database.. but other than that I can't see anything that you are doing inefficiently so your code looks fine except for some error handling and assumptions that you are making in regards to assuming that the code will never generate or throw an error.. make sense..? — MethodMan, Apr 04 '13 at 19:14
@DJKRAZE i didnot got the second part of your answer Microsoft Crap-Cess — rummykhan, Apr 04 '13 at 19:15
@DJKRAZE yes i got you... mean my codes are set except i should care about exceptions..?? — rummykhan, Apr 04 '13 at 19:16
`That's what we here in the USA call Microsoft Access` get it `Crap-Cess` never mind if you don't get the joke..LOL — MethodMan, Apr 04 '13 at 19:16
`you should always assume your code will fail `meaning never assume that everything that you write as a developer will not have errors` especially if there are `end users` involved they always seem to find a way to make or `development life cycle` never ending..I call it the `Common Sense Approach` — MethodMan, Apr 04 '13 at 19:17
hahaha.. really i didnot get u earlier.. actually im in learning process in my uni first semester.. so they started from Ms Access.. but i believe that Sql Queries are no more diffirent.. — rummykhan, Apr 04 '13 at 19:17
so you mean i should use try{} Catch{} in the methods too..?? — rummykhan, Apr 04 '13 at 19:19
SQL is SQL is SQL it's only different in my opinion in regards to which database engine / assembly, dll's etc you are using and how they interpret the `ANSCII SQL` but at least you are learning `2 thumbs up` — MethodMan, Apr 04 '13 at 19:19
`Rummy Khan` do you use `Microsoft Outlook` do you know what I call that...??? I call it `Microsoft LookOUT`...lol and that's usually what you have to do when they release new products `LOOKOUT` for all the `added non working / buggy functionality...` LOL — MethodMan, Apr 04 '13 at 19:22
not a problem I am quite sure you will turn out to be one heck of an awesome coder\ — MethodMan, Apr 04 '13 at 19:26

score 3 · Accepted Answer · edited May 23 '17 at 12:13

3

A couple of changes.
First, never use string concatenation to build sql command text. This leads to sql injection attacks. A very serious security problem

Second, your code for getting the number of books could result in a null value returned by ExecuteScalar and thus you will get an error

Third. The connection should be opened when needed, used, and then closed and disposed. Your code will fail to close and dispose the connection if, for whatever reason, you get an exception.
The using statement prevent this issue taking care to close and dispose of the connection also in case of exceptions.

Fourth well this is more a logical problem. I think that you can't move more books than those stored in the inventory, so add a check just to be safe-

public void moveBooks(int quantityOfMovedBooks, int booksID)
{
    int quantity = totalBooksInDB(booksID);
    if(quantity > quantityOfMovedBooks)
    {
        int finalQuantityOfBooks = quantity - quantityOfMovedBooks;
        queryString = "update Books set bQuantity=? where bID=?";
        using ( OleDbConnection myConn = new OleDbConnection(GetConnectionString() )
        using ( OleDbCommand myComm = new OleDbCommand(queryString, myConn))
        {
            myComm.Parameters.AddWithValue("@p1", finalQuantityOfBooks);
            myComm.Parameters.AddWithValue("@p2", booksID);
            myConn.Open();
            myComm.ExecuteNonQuery();
        }
     }
     else
        MessageBox.Show("Invalid quantity to move");
}

public int totalBooksInDB(int bID)
{
    int booksQuantity = 0;
    queryString = "select bQuantity from Books where bID=?";
    using ( OleDbConnection myConn = new OleDbConnection(GetConnectionString() )
    using ( OleDbCommand myComm = new OleDbCommand(queryString, myConn))
    {
        myComm = new OleDbCommand(queryString, myConn);
        myComm.Parameters.AddWithValue("@p1", bID);
        myConn.Open();
        object result = myComm.ExecuteScalar();
        if(result != null)
            booksQuantity = Convert.ToInt32(result);
    }
    return booksQuantity;
}

edited May 23 '17 at 12:13

Community

1
1

answered Apr 04 '13 at 19:19

Steve

213,761
22
232
286

Steve thanks for taking the time to refactor his example he can learn quickly from your clean code +1 from me – MethodMan Apr 04 '13 at 19:20
i cannot understan myComm.Parameters.AddWithValue("@p1", finalQuantityofBooks); why Parameters.AddwithValue is used and what does this "@p1" mean – rummykhan Apr 04 '13 at 19:24
and got your Fist Advice that should not use concatination..!! – rummykhan Apr 04 '13 at 19:25
If you look at the query text, I have removed the string and inserted placeholders `(?)`. The placeholders will be, well, _replaced_ by the parameter value added in the Parameters collection of the command. Each parameter has a name, this is the @p1 etc... When using `OleDb` is important to add the parameters in the exact order in which they replace the `?` because the name used to define the parameter is not used to find the placeholder. – Steve Apr 04 '13 at 19:27
using(OleDbConnection myComm = new OleDbCommand(queryString, myConn)) can you explain this part please..?? is it ok or i should replace first OleDBConnection with OleDBCommand..?? – rummykhan Apr 04 '13 at 19:31
its ok..!! if you wouldnot correct this i would have taken this as correct.. thanx for helping man – rummykhan Apr 04 '13 at 19:35
Fixed. Added a dummy function GetConnectionString() that is supposed to return the connection string from elsewhere. Notice that if you follow this pattern (open connection, use,close/dispose) you don't need a global variable to hold the connection. – Steve Apr 04 '13 at 19:36
one thing if you can explain.. for doing myComm.Parameters.AddWithValue("@p1", bID); i should write like using ( OleDbCommand myComm = new OleDbCommand(queryString, myConn)) or this is pro Coding style..?? – rummykhan Apr 04 '13 at 19:38
No, using statement is useful when you have an object that implements IDisposable interface and thus should be disposed (release the system resources) when you have finished with it. – Steve Apr 04 '13 at 19:39
you mean than i will not be in need to use myConn.Close(); it would be automatically disposed when this part has finished working..?? – rummykhan Apr 04 '13 at 19:42

efficient way to apply operator on specific record in MsAccess Database

1 Answers1