SQLConnect - A library for connecting Objective-C/Swift applications to Microsoft SQL Server

The complete SQLConnect library can be found here.

The file from which the following method belongs can be found here.

At some point I may or may not open questions for reviewing other aspects of this library, but I intend to keep the questions relatively small. This is the largest method in the entire library and is responsible for all the hard work.

Once the user has successfully connected to the database, they call this method in order to execute a SQL command on the database. This happens in a background thread. The thread waits for response from the server, then parses the results into Objective-C objects.

The structure of the results look like this:

• Results is an array of tables.

• Each table is an array of rows.

• Each row is a dictionary, with the key being the column name and the value being the value in that column.

I'm curious to any ways the code can be more efficient from a speed perspective. In practice, I've found that any query that selects less than about 1000 rows can produce the results without the user really noticing. Once we venture into queries over 1000 rows, the time to finish all this works starts to be noticeable by the user.

Now obviously, a slow query will run slow. But I can run a query that will select 5000 rows in SQL Management Studio that returns relatively quickly, while the same query here takes noticeably longer. I don't have any specific examples (nor do I know of a publicly available SQL Server for you to test against), but my testing has shown that there is definitely more time spent turning the results into usable Objective-C objects than there is time spent executing the query and getting the results.

Here's the code:

```

• (void)execute:(NSString *)statement {

void(^cleanUp)(SQLColumn , SQLColumn , int numCols) = ^(SQLColumn pcol, SQLColumn columns, int numCols) {
for (pcol = columns; pcol - columns dataBuffer);
}
free(columns);
};

// work on background queue

Style and Correctness

Before I get into your desired efficiency points, I'd like to first take a stab at style and correctness (both of which are minor, and my apologies if you weren't particularly looking for this type of feedback).

This is a really, really big function. I would seriously consider compartmentalizing it into a few private methods or static functions (the C kind). At the very least, pull some of your loop body or switch cases into functions.

You're not checking the return of dbcmd, and it can fail (not very likely though, of course -- just a good idea to do especially thorough error handling in any kind of library).

Declare variables in the tightest scope possible. It's confusing looking through this and wondering where in the world certain variables came from only to realize that they were set halfway up the function even though they're reset each loop iteration. (columns andpcol are examples of this.)

Unless you're doing pointer trickery with void, It's typically a good idea with sizeof to use the deferenced pointer instead of the type. This leaves you less likely to make typos, and if the type changes, it only has to be updated in one place. (For example, columns = calloc(numCols, sizeof(SQLColumn)) becomes columns = calloc(numCols, sizeof(columns)).)

Why are all local variables camelCase except for row_code?

dbcollen can theoretically fail, and that's not checked (how likely this is... I would imagine not very -- just being pedantic).

// clean up immediately followed by a function named cleanUp is redundant. Considering a lot of ObjC people have no manaul resource management background, a comment might be nice, but if there's to be one, a bit more detail about motivation might be nice (and only once).

I would consider either storing an end pointer or basing off of index for your loops:

SQLColumn* columnsEnd = columns + numCols;
for (SQLColumn* pcol = columns; pcol != columnsEnd; ++pcol) { }

Or:

for (int i = 0; i < numColumns; ++i) {
    SQLColumn* pcol = columns + i;
}

// parsing results complete, return immutable copy of results
[self executionSuccess:result];

This comment seems outdated. I'm sure executionSuccess takes a NSArray rather than NSMutableArray, but there's no copying going on.

// allocate C-style array of COL structs
columns = calloc(numCols, sizeof(SQLColumn));

Same situation -- the comment doesn't seem quite right.

The FreeTDS docs are vague on this, but I believe that bailing out of reading rows in the middle of a row-fetch-loop will leave the results cursor open. This means that the server is going to have to hold on to all of its active state about the query until the next query lets it know the connection has moved on, or it times out and the sever kills it. If this is actually what happens, this could be a rather annoying little behavior for someone down the road (though it should be incredibly rare...).

For the future when you do logic more complex than converting everything to a string, you might want to store the byte for the null character directly in pcol->dataSize` rather than adding it on in allocation. Types other than strings won't need a null byte. (Then again, I suppose there's merits for dataSize representing the size of the data, not the size of the buffer to store the data...)

// get current column number <-- Not really needed with a decent variable name
int columnIdx = (int)(pcol - columns + 1);

StackExchange Code Review Q#59308, answer score: 6