HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Obj-C wrapper for OpenLDAP

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
wrapperobjforopenldap

Problem

This section of code reside in a utility class (LDAPUtility). It's purpose is to subscribe or unsubscribe a user from an LDAP group. My main questions are if I am using the LDAP library correctly and if I am handling the memory management correctly.

+ (BOOL)addUser:(const char *)userDN toGroup:(const char *)groupDN {
    return [LDAPUtility performOperation:LDAP_MOD_ADD withUser:userDN andGroup:groupDN];
}

+ (BOOL)removeUser:(const char *)userDN fromGroup:(const char *)groupDN {
    return [LDAPUtility performOperation:LDAP_MOD_DELETE withUser:userDN andGroup:groupDN];
}

+ (BOOL)performOperation:(int)op withUser:(const char *)userDN andGroup:(const char *)groupDN {
    LDAPMod **mods;
    LDAPMod mod;

    int err;

    mod.mod_op = op;
    mod.mod_type = "member";
    mod.mod_vals.modv_strvals = malloc(sizeof(char *));
    mod.mod_vals.modv_strvals[0] = (char *)userDN;
    mod.mod_vals.modv_strvals[1] = NULL;

    mods[0] = &mod;
    mods[1] = NULL;

    if ((err = ldap_modify_ext_s(ld, groupDN, mods, NULL, NULL)))
    {
        free(mod.mod_vals.modv_strvals);
        DDLogError(@"   ldap_modify_ext_s(): %s", ldap_err2string(err));
        return false;
    }

    free(mod.mod_vals.modv_strvals);
    return true;
}


And the layer of code above this (which is not what I am submitting for a review, but may help contextualize the code above):

```
+ (void)subscribeToGroup:(Group )group completion:(void (^)(BOOL success, NSError error))completion {
dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0ul);
dispatch_async(queue, ^{
NSError *error = nil;
BOOL success = false;

if (![Authenticator currentUser]) {
// Go away, you're drunk.
error = [NSError errorWithDomain:@"Not Authenticated" code:401 userInfo:@{NSLocalizedDescriptionKey:@"User has not authenticated."}];
} else if (!group) {
error = [NSError errorWithDomain:@"Missing Parameter" code:400

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#71680, answer score: 6

Revisions (0)

No revisions yet.