patterncMinor
Obj-C wrapper for OpenLDAP
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.
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
+ (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
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
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:
And one final note about this code, given that it's Objective-C, we should use
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.