Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As a Java native, I am always a little worried when doing Objective-C because of memory leaks and pointers flying at my head...

All comments are welcome on the Category below. The Category functions are correct.

GitHub

#import "NSString+r47.h"

@implementation NSString (r47)
-(NSString *) r47String
{
    const char *_string = [self cStringUsingEncoding:NSASCIIStringEncoding];
    int stringLength = [self length];
    char newString[stringLength+1];
    int x;
    for( x=0; x<stringLength; x++ )
    {
        unsigned int aCharacter = _string[x];
        if( 0x20 < aCharacter && aCharacter < 0x7F ) // from ! to ~
            newString[x] = (((aCharacter - 0x21) + 0x2F) % 0x5E) + 0x21;
        else  // Not an r47 character
            newString[x] = aCharacter;
    }
    newString[x] = '\0';
    NSString *rotString = [NSString stringWithCString:newString encoding:NSASCIIStringEncoding];
    DLog(@"%@ = %@",self,rotString);
    return rotString ;
}

@end
share|improve this question
1  
I would say it looks ok — as long as you dont use it to gain safety in real world projects. ROT is hardly obfuscating and not secure at all. –  vikingosegundo Jan 2 '13 at 22:51
    
@Frank, I'd personally avoid using the leading underscore naming on _string here. I think that's distracting, and potentially confusing. –  Nate May 24 '13 at 21:32
add comment

2 Answers 2

up vote 4 down vote accepted

This bit looks just a bit suspect:

int stringLength = [self length];
char newString[stringLength+1];

You're using the NSString "length" property to size the C char array allocation. This assumes that the string's "length" value (which reflects number of composed unicode characters) will always be equal to the ASCII encoded length, but that's not necessarily true. This is a case of using a value from one domain in another. NSString provides a method that you should use instead:

int stringLength = [self lengthOfBytesUsingEncoding:NSASCIIStringEncoding];

An equivalent, and probably even clearer way to handle this is to get your length from the C-native function, which makes sense since that's the string you're iterating over in the first place:

size_t stringLength = strlen(_string);
share|improve this answer
    
+1, thats good info. –  Frank Jan 9 '13 at 5:58
add comment

The question title suggested that this would be a Rot47 category on NSString, but it's not. It's an r47 category (which tells me nothing... what's "r47"?).

This is Objective-C, not C or C++ or any other language where cryptic terseness is favored. We prefer verboseness. The category should be called Rot47. This should be @implementation NSString (Rot47). The method should be called - (NSString *)rot47String;


In Objective-C, we also prefer our opening braces go on the same line, not on their own line. For example:

for (;;) {

Braces are technically optional, but you really shouldn't think of them as such. Apple even agrees and made them non-optional in . You've left them out of your if/else blocks, which is a great way to leave your code open for future bugs.


When we implement categories on Foundation classes, we should take care to ensure their portability. The method should be designed to work no matter what project we import this file into and no matter what the contents of the string.

The problem here is that your method doesn't do that. Your method works fine presuming a particular encoding, but for anything else it will produce incorrect results.

Rather than a char array, we need a unichar array.

NSUInteger length = [self length];
unichar letters[length + 1];

[self getCharacters:letters range:NSMakeRange(0,length)];

Now letters is an array of unichars for us to iterate over and transform correctly.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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