6
\$\begingroup\$

I feel like I can make my switch statements more elegant with a function pointer, i.e. I'd like to set the digestLength and a pointer to a function in the same switch statement instead of setting the length, declaring the result array, then calling the function.

- (NSString *)hashWithDigestType:(NSStringDigestType)type {
    const char *s = [self UTF8String];
    int digestLength;

    switch (type) {
        case NSStringDigestTypeMD5:
            digestLength = CC_MD5_DIGEST_LENGTH;
            break;
        case NSStringDigestTypeSHA1:
            digestLength = CC_SHA1_DIGEST_LENGTH;
            break;
        case NSStringDigestTypeSHA512:
            digestLength = CC_SHA512_DIGEST_LENGTH;
            break;
    }

    unsigned char result[digestLength]; 

    switch (type) {
        case NSStringDigestTypeMD5:
            CC_MD5(s, strlen(s), result);
            break;
        case NSStringDigestTypeSHA1:
            CC_SHA1(s, strlen(s), result);
            break;
        case NSStringDigestTypeSHA512:
            CC_SHA512(s, strlen(s), result);
            break;
    }

    NSMutableString *digest = [NSMutableString stringWithCapacity:(digestLength * 2)];
    for (NSUInteger i = 0; i < digestLength; i++)
        [digest appendFormat:@"%02x",result[i]];

    return [NSString stringWithString:digest];
}
\$\endgroup\$
4
\$\begingroup\$

I can't write the code in Objective-C, but in C, you could create yourself a structure type to get the information in one move. I don't know whether the NSStringDigestTypeXXX values are conveniently numbered compactly from 0 or 1, so I'm taking the pessimistic view that they are not. You can simplify the code below if they are compact and small.

struct Digestifier  // Declaration in a header (probably)
{
    int      hashtype;
    void   (*hash)(const char *source, size_t length, char *result);
    size_t   hashlen;
};
static const struct Digestifier digests[] =
{
    { NSStringDigestTypeSHA1,   CC_SHA1,   CC_SHA1_DIGEST_LENGTH   },
    { NSStringDigestTypeMD5,    CC_MD5,    CC_MD5_DIGEST_LENGTH    },
    { NSStringDigestTypeSHA512, CC_SHA512, CC_SHA512_DIGEST_LENGTH },
};
{ enum NUM_DIGESTS = sizeof(digests) / sizeof(digests[0]) };

You can then write a lookup function for this:

const struct Digestifier *digest_lookup(int hashtype)
{
    for (i = 0; i < NUM_DIGESTS; i++)
    {
        if (digests[i].hashtype == hashtype)
            return &digests[i];
    }
    assert(i != NUM_DIGESTS);  // Or other error handling!
    return 0;
}

And in your function:

- (NSString *)hashWithDigestType:(NSStringDigestType)type {
    const char *s = [self UTF8String];
    const struct Digestifier *digest = digest_lookup(type);

    // Error check digest if digest_lookup() does not do it for you!
    unsigned char result[digest->hashlen];
    digest->hash(s, strlen(s), result);

    NSMutableString *digest = [NSMutableString stringWithCapacity:(digest->hashlen * 2)];
    for (NSUInteger i = 0; i < digestLength; i++)
        [digest appendFormat:@"%02x",result[i]];

    return [NSString stringWithString:digest];
}

Note that you could also write the hash function invocation as:

    (*digest->hash)(s, strlen(s), result);

To some of us old-school (pre-standard) C programmers, that might perhaps be clearer.

Also, if Objective-C supports the C99 designated initializer notation, you could make the initializer for the digests[] array more robust (and render the hashtype member superfluous except for a cross-check):

static const struct Digestifier digests[] =
{
    [NSStringDigestTypeSHA1]   =
           { NSStringDigestTypeSHA1,   CC_SHA1,   CC_SHA1_DIGEST_LENGTH   },
    [NSStringDigestTypeMD5]    =
           { NSStringDigestTypeMD5,    CC_MD5,    CC_MD5_DIGEST_LENGTH    },
    [NSStringDigestTypeSHA512] =
           { NSStringDigestTypeSHA512, CC_SHA512, CC_SHA512_DIGEST_LENGTH },
};

This initializer correctly places the three rows in the array regardless of which member of the enumeration is mapped to 0, 1, 2.


With the additional information that the NSStringDigestTypeXXX values are 0, 1, 2, you can simplify the digest_lookup() function by:

  1. Ensuring that the rows in the digests array are in the correct (0, 1, 2) sequence.
  2. Changing from a search loop to a direct array lookup.
  3. Probably asserting that the value is under control.

For the purposes of the code below, I'm assuming that NSStringDigestTypeMD5 is 0 and NSStringDigestTypeSHA512 is 2, but the ordering of the names is arbitrary; just choose the equivalent of 0 for the first name in the assert and 2 for the second.

const struct Digestifier *digest_lookup(NSStringDigestType hashtype)
{
    assert(hashtype >= NSStringDigestTypeMD5 &&
           hashtype <= NSStringDigestTypeSHA512);
    assert(digest[hashtype].hashtype == hashtype);
    return &digests[hashtype];
}

The first assertion ensures that the value is in range. The second assertion ensures that the table is properly sequenced and you are getting back the entry your expect.

\$\endgroup\$
2
  • \$\begingroup\$ Objective-C is just C with a bunch of additional syntax, so anything that works in C will work for me. NSStringDigestType is an enum I declared in the header, so the values are just 0,1,2. Give me a bit to parse your answer, I'm not as comfortable with C as I should be. Thanks! \$\endgroup\$ – kubi Feb 20 '11 at 0:38
  • \$\begingroup\$ @kubi: I am aware that C is a subset of Objective-C, which is one reason I proffered an answer with some confidence that it would be useful to you. However, it was also only fair to point out that I cannot reliably use the distinctive Objective-C notations, so there may be a better, more idiomatic construction for the ideas. Nevertheless, I think the code is generally better as shown. \$\endgroup\$ – Jonathan Leffler Feb 20 '11 at 1:15
4
\$\begingroup\$

Here's some 2 cents.

  1. You should never use identifiers beginning with NS, as this prefix is reserved for Apple only. In fact, recently Apple has begun discouraging the use of two- or three-letter prefixes entirely.
  2. [I'm not sure about this approach, but] consider implementing each hash calculation as separate methods; and simply using selectors instead of function pointers:

    - (NSString *) hashWithDigestType:(StringDigestType) type
    {
        SEL digestMethods[] = {
            @selector(hashWithMD5),
            @selector(hashWithSHA1),
            @selector(hashWithSHA256)
        };
    
        return [self performSelector:digestMethods[type]];
    }
    

    Consider also validating the type variable and the possibility of using designated initialisers, so that you can directly map the enum values to the appropriate selectors.

\$\endgroup\$
4
  • \$\begingroup\$ Very good point on the prefix, I'll make that change. As far as your selector solution, it's not quite what I'm looking for. I'll end up having to write three separate methods that perform nearly identical tasks. \$\endgroup\$ – kubi Feb 20 '11 at 13:55
  • \$\begingroup\$ @kubi, it was discussed at WWDC 2010. \$\endgroup\$ – dreamlax Feb 20 '11 at 19:56
  • \$\begingroup\$ Are you sure? The Cocoa coding guidelines only discourage prefixes for method names and structure fields. Beyond that, they stress the importance of prefixes: developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/… \$\endgroup\$ – Philip Regan Apr 14 '11 at 16:45
  • \$\begingroup\$ @Philip: The documentation always lags behind the discussions at WWDC. The documentation you linked to was last updated before WWDC in 2010. \$\endgroup\$ – dreamlax Apr 14 '11 at 22:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.