Obj-C wrapper for OpenLDAP

Answerer's note: I'm moving commentary about NSString versus C-style strings out of this original answer and into their own answer.

The strings we use in creating the error objects, as well as the integers we use for the code in these objects should be defined, named constants (and probably an enum for the integers) all put together in one common place. This way, if we ever want to reuse any of these, that's simple. If we ever want to rename, reword, renumber, etc., they're all located in one easy to find location no matter how messy the code in this particular section gets.

The word "and" should be used to indicate that a method performs two distinct actions (and typically should just be two separate methods anyway). We shouldn't use the word "and" to chain our variables.

Why not make an enumeration for our operation constants? This will make our Objective-C code slightly more clear, and if someone uses this in a Swift project, they'll really like that they get errors if trying to send an invalid argument for this.

Our enum might look like this:

typedef NS_ENUM(NSInteger, LDAPOperation) {
    LDAPOperationAddUser = LDAP_MOD_ADD,
    LDAPOperationDeleteUser = LDAP_MOD_DELETE
};

And one final note about this code, given that it's Objective-C, we should use YES and NO rather than true or false. There's no technical difference, but the Objective-C style is to use the YES/NO defines.

The "context" code also has some issues.

For a start, I know that the Group and Authenticator classes are not included, and I'm not reviewing those explicitly so to speak, but they need some better class names.

We don't have namespaces in Objective-C. Namespaces are used in other programming languages to prevent name collisions. But since we don't have them in Objective-C, we use this convention of prefixing all of our class names with some common prefix based on the framework they are in. This is why everything from Foundation starts with NS, everything from UIKit starts with UI, everything from CoreGraphics starts with CG, etc.

We need to do the same with our own classes. And we should expect that any 3rd party library/framework we import to our projects should be doing the same thing as well.

+ (void)subscribeToGroup:(Group *)group 
              completion:(void (^)(BOOL success, NSError *error))completion;

This method signature is kind of ugly. But any method signature that accepts a block as an argument tends to be ugly... unless we typedef our block.

It's pretty simple to typedef our blocks:

typedef void (^MyBlockName)(BOOL success, NSError *error));

And now we can rewrite our method signature simply as:

+ (void)subscribeToGroup:(Group *)group completion:(MyBlockName)completion;

Even if we only have one method that uses this block, the advantage here (besides cleanliness), is that if we need to change the block, we need to only change it once, at its typedef, and not where we expose the method in .h and implement the method in .m.

But before you typedef that block as is, let's consider whether or not this is how we actually want our block to look. Per my estimation, we're getting redundant information while also not getting enough information.

We don't need a BOOL to indicate success AND an NSError object to tell us what the error was when there was one. We can simply pass the error object. The error object will be nil when there wasn't an error, right? So instead of:

if (success) {
    // do stuff
} else {
    // do something with the error variable
}

We can eliminate the success variable and our block can use this logic:

if (!error) {
    // do stuff
} else {
    // do something with the error variable
}

And that fixes the redundancy. But we're still left with the problem of not having enough information in our completion block.

The code in our subscribeToGroup:completion: method executes asynchronously. There's nothing to prevent me from calling it 100 times in quick succession if I want to add the current user to 100 different groups.

When my callback events start firing, how am I able to discern which groups I was successfully subscribed to and which groups I had what sort of errors for?

We should change our completion block:

typedef void (^GroupSubscriptionCompletion)(Group *group, NSError *error);

And change the code within the method to add the group object that was passed originally to the callback group so that the completion block knows which group it just completed.

We have a really, really big problem right here:

dispatch_async(queue, ^{
    // stuff
    if (![Authenticator currentUser]) {

And then later, still within the asynchronous block:

const char *userDN = [[Authenticator currentUser].distinguishedName UTF8String];

The problem is, as I can gather from method naming, Authenticator seems to be a singleton-esque class. And either way, based on the name, currentUser, the value that [Authenticator currentUser] returns will likely be something that can change, based on which user is logged in or something, right?

Neverminding my opinions on that design, the problem is that if the value that [Authenticator currentUser] can (and will) change, then we can make zero guarantees that the value it holds when our asynchronous block asks for it is the same value it held when whatever dispatched this method called it. In fact, we can't even guarantee that in the time between the nil check and asking for the distinguishedName property it won't have been made nil by some other thread.

Also, I don't particularly care for relying on the other class to determine the user. We should probably instead just take the user as an argument. Let's change our method signature:

+ (void)subscribeUser:(Authenticator *)user 
              toGroup:(Group *)group
           completion:(GroupSubscriptionCompletion)completion;

Now the caller can be explicit about who should be subscribed to what, and what should happen when that's done happening.

And with that said, it should be pretty obvious that we need to go ahead and add the user to the completion block so that the completion block knows who was subscribed to what:

typedef void (^GroupSubscriptionCompletion)(Authenticator *user, Group *group, NSError *error);

And of course, that means passing the user to the completion block as well.

NSString versus C-String

First and foremost, we can't call anything an Objective-C wrapper of a C library if we're expecting the end user to provide us with C-strings. If C-strings are necessary, we should allow our Objective-C end-users to provide us when an NSString object which we will then convert to the appropriate C-style string at the appropriate time.

Presuming that these methods:

• + (BOOL)addUser:toGroup:
• + (BOOL)removeUser:fromGroup:
• + (BOOL)performOperation:withUser:andGroup:

are not in anyway exposed to the user, then your code successfully accomplishes the most important thing.

But that again presumes that these methods aren't in a .h file anywhere or that you are compiling this library to a .a file and not distributing the .h file that contains these method declarations.

So, with that said, once we've received the NSString object from the user, when should we make the conversion from NSString to C-String? The answer to that question depends on who is most likely going to be maintaining your code.

Will your code be maintained by Objective-C programmers who know just enough C? Or will your code be maintained by C programmers who know just enough about Objective-C?

Given that you seem uncertain about your C memory management, and you only actually use much C code in a single method (other than passing around C-style strings), I'm going to assume that you're primarily an Objective-C programmer who doesn't know a whole lot of C.

And in that scenario, we generally should be making the conversion at the last possible moment from Objective-C to C.

At any point in the future, some bug or some feature you want to implement might require you to do something with those strings in a particular part of the code. Aren't you going to be far more effective implementing and bug-fixing in Objective-C than you would be in C?

But the language of the maintainers isn't the only consideration. There's one final thing that also must be considered, and this final point is actually what completely decides whether we should use C-strings or NSStrings for me. What are the performance implications?

Since we're not doing anything to the string besides passing it around until finally passing it into the C function, the only possible performance difference would be any sort of memory consideration.

All we're passing around in either case is a pointer, and that's going to be the same size in either case. Either only occupies the size of a pointer on the stack.

How about the heap?

If we create a C-String and an NSString individually, assuming the same string, the NSString will take up slightly more space on the heap--it has a little more overhead.

But in this scenario, that's not the only consideration. Creating the C-string doesn't make the NSString go away. The NSString already existed on the heap and continues to exist on the heap. So creating the C-string basically nearly doubles our heap memory usage for the time it exists.

So with that said, in order to improve our memory performance, we can simply, again, all together now, wait until the last possible moment before creating a C-style representation of our string.