views:

2498

answers:

4

When retrieving objects from an NSMutableArray in cocoa-touch is the below code ok? Should I be allocating([alloc]) new Page objects each time or is just pointing to it alright? Do I need to do anything to the Page *pageObj after, such as set it to nil?

const char *sql = "insert into Page(Book_ID, Page_Num, Page_Text) Values(?, ?, ?)";
for (i = 0; i < ([[self pagesArray] count] - 1); i++) {
    if(addStmt == nil) {
        if(sqlite3_prepare_v2(database, sql, -1, &addStmt, NULL) != SQLITE_OK) {
            NSAssert1(0, @"Error while creating add statement. '%s'", sqlite3_errmsg(database));
        }
    }
    Page *pageObj = [[self pagesArray] objectAtIndex:i];
    if(pageObj.isNew) {
        sqlite3_bind_int(addStmt, 1, self.book_ID); 
        sqlite3_bind_int(addStmt, 2, pageObj.page_Number);  
        sqlite3_bind_text(addStmt, 3, [[pageObj page_Text] UTF8String], -1, SQLITE_TRANSIENT);
        if(SQLITE_DONE != sqlite3_step(addStmt)) {
            NSAssert1(0, @"Error while inserting data. '%s'", sqlite3_errmsg(database));
        }
        NSLog(@"Inserted Page: %i into DB. Page text: %@", pageObj.page_Number, pageObj.page_Text);
    }
    //Reset the add statement.
    sqlite3_reset(addStmt);                     
}

Thanks. I also understand this should probably be in a transaction but I didn't quite get that working just yet.

+1  A: 

Of course not. You have allocated it before and are just referencing the same object. No need to reallocate. Also you don't need to set it to nil.

Mehrdad Afshari
+4  A: 

The way you're declaring a pointer is correct. You don't need alloc, since that creates a new object, when you want to refer to an existing object in the array. You would want to retain it if you were going to keep the reference outside of that method, but since you're only using it temporarily it's fine not to.

The actual pointer variable will be destroyed and recreated every trip to the loop, so there's no need to set it to nil. Even if you declared the variable outside the loop, simply assigning it to a new object is fine. The only time you'd set it to nil is when you're releasing the object stored in the pointer (or the object may be released elsewhere). If you didn't set it to nil in that case, the pointer would refer to an invalid memory location after the object is dealloced, usually causing a crash.

One bug I can see though, you're skipping the last element in your array in your for loop by subtracting 1 from the count.

Marc Charbonneau
Thanks Marc, that off-by-one error sure creeps up on you!
Sean
+2  A: 

Aside from the previously mentioned count error, it looks good.

As far as transactions go, I highly recommend wrapping this write loop in one. It will greatly increase your write performance and I found that it helps with memory usage as well. I use the following class method to begin a transaction:

+ (BOOL)beginTransactionWithDatabase:(sqlite3 *)database;
{
    const char *sql1 = "BEGIN EXCLUSIVE TRANSACTION";
    sqlite3_stmt *begin_statement;
    if (sqlite3_prepare_v2(database, sql1, -1, &begin_statement, NULL) != SQLITE_OK)
    {
     return NO;
    }
    if (sqlite3_step(begin_statement) != SQLITE_DONE) 
    {
     return NO;
    }
    sqlite3_finalize(begin_statement);
    return YES;
}

and this one to end a transaction:

+ (BOOL)endTransactionWithDatabase:(sqlite3 *)database;
{
    const char *sql2 = "COMMIT TRANSACTION";
    sqlite3_stmt *commit_statement;
    if (sqlite3_prepare_v2(database, sql2, -1, &commit_statement, NULL) != SQLITE_OK)
    {
     return NO;
    }
    if (sqlite3_step(commit_statement) != SQLITE_DONE) 
    {
     return NO;
    }
    sqlite3_finalize(commit_statement);
    return YES;
}

I should probably store the SQL statements for later reuse, but these transaction statements are called much less frequently than my other queries.

Brad Larson
Thanks Brad, do I just keep my above query the same as is and wrap it in a beginTransaction and endTransaction as you posted?
Sean
Yes put a [YourClass beginTransactionWithDatabase:database] before the above code and a [YourClass endTransactionWithDatabase:database] after it, and that should be all that's required to perform your loop of queries within a single transaction.
Brad Larson
A: 

how to drop a column in sqlite....?

vijay
Vijay, you should ask this as a standalone question to get responses.
Sean