Random Name Generator

Question

I've had ideas for something like this in the past but I've finally implemented one and I could definitely use some code review for it. Obviously the quality of the names generated will depend on what the whole thing is seeded with, but that's a separate question. My concerns are the quality and readability of the code, following Objective-C best practices, and possibly how to better structure it to add functionality to it.

I'm creating an NSString, so I did briefly try to subclass NSString before I realized that it was a lot more complicated than I thought, so I'm just using a string property on the object.

Here is the header:

#import <Foundation/Foundation.h>

@interface DTNameCreator : NSObject

//eventually this would take an enum as a type
-(id) initWithType:(int)characterType;

@property NSString *createdName;

@end

And the implementation:

#import "DTNameCreator.h"

@implementation DTNameCreator {
    NSMutableArray *_prefixArray;
    NSMutableArray *_middleString1Array;
    NSMutableArray *_middleString2Array;
    NSMutableArray *_middleString3Array;
    NSMutableArray *_suffixStringArray;

    NSString *_prefixString;
    NSString *_middleString1;
    NSString *_middleString2;
    NSString *_middleString3;
    NSString *_suffixString;

    NSMutableArray *_generatorArray;
}

#pragma mark - Initialization
-(id) initWithType:(int)characterType {
    self = [super init];
    if (self) {
        [self seedArraysForType:characterType];
        [self createNameGeneratorStringsAndArray];
        [self createName];
    }
    return self;
}
-(void) seedArraysForType:(int)characterType {
    switch (characterType) {
        case 0:
            [self seedArraysForType0];
            break;
        case 1:
            [self seedArraysForType1];
            break;
        default:
            break;
    }
}
-(void) seedArraysForType0 {
    _prefixArray = [[NSMutableArray alloc]initWithObjects:@"Abu", @"Bra", @"Der", @"Eve", @"Far", @"Gar", @"Her", nil];
    _middleString1Array = [[NSMutableArray alloc]initWithObjects:@"ne", @"we", @"re", @"ne", @"be", @"ge", @"te", nil];
    _middleString2Array = _middleString1Array;
    _middleString3Array = _middleString1Array;
    _suffixStringArray = [[NSMutableArray alloc]initWithObjects:@"we", @"re",@"mu", @"ne", @"ge", @"or", @"ar", nil];
}
-(void) seedArraysForType1 {
    _prefixArray = [[NSMutableArray alloc]initWithObjects:@"Wen", @"Pre", @"Mar", @"Nem", @"Car", @"Cat", @"Hel", nil];
    _middleString1Array = [[NSMutableArray alloc]initWithObjects:@"ne", @"we", @"re", @"ne", @"be", @"ge", @"te", nil];
    _middleString2Array = _middleString1Array;
    _middleString3Array = _middleString1Array;
    _suffixStringArray = [[NSMutableArray alloc]initWithObjects:@"pe", @"ra",@"mul", @"nem", @"ge", @"ara", @"era", nil];
}
-(void) createNameGeneratorStringsAndArray {
    int randomNumber1 = arc4random() % _prefixArray.count;
    int randomNumber2 = arc4random() % _middleString1Array.count;
    int randomNumber3 = arc4random() % _middleString2Array.count;
    int randomNumber4 = arc4random() % _middleString3Array.count;
    int randomNumber5 = arc4random() % _suffixStringArray.count;

    _prefixString = [[NSString alloc]initWithString:[_prefixArray objectAtIndex:randomNumber1]];
    _middleString1 = [[NSString alloc]initWithString:[_middleString1Array objectAtIndex:randomNumber2]];
    _middleString2 = [[NSString alloc]initWithString:[_middleString2Array objectAtIndex:randomNumber3]];
    _middleString3 = [[NSString alloc]initWithString:[_middleString3Array objectAtIndex:randomNumber4]];
    _suffixString = [[NSString alloc]initWithString:[_suffixStringArray objectAtIndex:randomNumber5]];

    //only add the middle ones because we add the prefix and suffix manually
    _generatorArray = [[NSMutableArray alloc]initWithObjects:_middleString1, _middleString2, _middleString3, nil];
}

#pragma mark - Name geneneration
-(void) createName {
    NSMutableArray *localGeneratorArray = [[NSMutableArray alloc]init];
    NSString *generatorString = [[NSString alloc]init];

    //have to add the prefix manually first
    [localGeneratorArray addObject:_prefixString];

    //check if any of the middle pieces are already present in the name and add if not
    BOOL isMiddlePresent = NO;
    for (id obj in _generatorArray) {
        for (int i = 0; i < localGeneratorArray.count; i++) {
            if ([obj isEqualToString:[localGeneratorArray objectAtIndex:i]]) {
                isMiddlePresent = YES;
            }
        }
        if (!isMiddlePresent) {
            [localGeneratorArray addObject:obj];
        }
    }

    //check if the suffix is present and add it if not
    BOOL isSuffixPresent = NO;
    for (int i = 0; i < localGeneratorArray.count; i++) {
        if ([_suffixString isEqualToString:[localGeneratorArray objectAtIndex:i]]) {
            isSuffixPresent = YES;
        }
    }
    if (!isSuffixPresent) {
        [localGeneratorArray addObject:_suffixString];
    }

    //create the name string out of the contents of the array
    for (int i = 0; i < localGeneratorArray.count; i++) {
        generatorString = [generatorString stringByAppendingString:[localGeneratorArray objectAtIndex:i]];
    }

    self.createdName = generatorString;
}

@end

And a sample of the output:

2014-04-07 10:12:33.273 DwarfTowers01[21566:60b] Eveweor
2014-04-07 10:12:33.273 DwarfTowers01[21566:60b] Abuwetegere
2014-04-07 10:12:33.274 DwarfTowers01[21566:60b] Garnear
2014-04-07 10:12:33.274 DwarfTowers01[21566:60b] Fargewene
2014-04-07 10:12:33.275 DwarfTowers01[21566:60b] Derneteor
2014-04-07 10:12:33.275 DwarfTowers01[21566:60b] Evenewe
2014-04-07 10:12:33.276 DwarfTowers01[21566:60b] Farnewegemu
2014-04-07 10:12:33.276 DwarfTowers01[21566:60b] Dernege
2014-04-07 10:12:33.277 DwarfTowers01[21566:60b] Gargemu
2014-04-07 10:12:33.277 DwarfTowers01[21566:60b] Farbewenere

Answer 1

So, here's a quick lesson on Objective-C @properties.

When you declare a @property, you've created 3 things. An instance variable, a setter, and a getter.

For example, @property NSString *foo; is almost the same as this:

@interface SomeClass : NSObject

- (void)setFoo:(NSString*)foo;
- (NSString *)foo;

@end

@implementation SomeClass {
    NSString *_foo;
}

- (void)setFoo:(NSString *)foo {
    _foo = foo;
}

- (NSString *)foo {
    return _foo;
}

@end

Now, that's just a default @property. The thing is, we can do much more with properties--such as giving them attributes.

In this case, we'll definitely want the readonly attribute.

@property (readonly) NSString *createdName;

Now, instead of creating an instance variable and a setter and a getter, we'll only generate an instance variable and a getter.

So, how do we set the instance variable?

Well, we certainly don't want to allow the instance variable to be set externally. That'd be no good and wouldn't make our generated name very random, would it? So, we have two options. We can either simple access the instance variable directly, or we can redeclare the property in the .m as a readwrite property.

@property (readwrite) NSString *createdName;

But... you can also completely eliminate the instance variable.

Currently, your class calls createName in the init method and it creates a name. The only publicly exposed aspects of the class are the init and the @property. Once I've grabbed the first name generated, I can't do anything else with the class. The BEST way to use this class as is would be as such:

NSString *name;
@autoreleasepool {
    name = ([[DTNameCreator alloc] initWithType:0]).createdName;
}

This assures that ARC quickly deallocs the object since it's no good to us once we've extracted the single generated name.

But, there's a better option.

First, let's take the call to createName out of init. We don't need to create a name before someone calls it, and this is especially true if we want generate numerous names from the same instance.

Now, remember what I said? ... you can also completely eliminate the instance variable...

Okay, so let's change our property declaration to readonly:

@property (readonly) createdName; // I think randomName is a little bit better name

Now, it's readonly, so we've got no setter method. But we don't need it. "Setting" this value is merely a matter of random generation... the work you're doing in createName.

So, change the name of your createName method to createdName and change its return type from void to NSString *. And finally, change the last line of the method.

Eliminate this line:

self.createdName = generatorString;

And add this line:

return generatorString;

So, what have we done? Because this method name has the same return type and name as one of our @properties, this is now the getter for that property. Because we declared the property as readonly, there's no setter created. And because you've eliminated both the automatically generated setter and getter and haven't used what would be the autosynthesized instance variable backing this property, the instance variable is never created.

So now,

DTNameCreator *nameCreator = [[DTNameCreator alloc] initWithType:0];

The object is instantiated but no names are generated.

for(int i=0; i<100; ++i) {
    NSLog(@"Random name: %@",nameCreator.createdName);
}

Now you'll generate 100 random names as simple as that without instantiating a new object each time.

---

A couple other notes for completeness.

1. An enum, as you already mentioned, defining the types of names to generate.
2. A factory method: + (instancetype)nameCreatorWithType:(int)type;
3. I can't see why your arrays other than the localGeneratorArray in createName would need to be mutable. You should change these to NSArray.
4. The default init method.

Four is a big one. It's important that your class still work properly when someone tries to initialize it with the default init it inherits from NSObject.

In some cases, such as a delegated object that must have a delegate, you might choose to simply prevent init from being called:

- (id)init __attribute__((unavailable("Cannot instantiate without a delegate")));

But in this case, I don't think that's necessary. We just need to be sure that the initializations take place, so the following should be sufficient:

- (id)init {
    return [self initWithType:0];
}

You don't need to add anything in .h, just add these lines to the .m file.

---

Addendum: I spent a few minutes looking at createName in more detail and noticed a few issues.

Your forin loops.

for (id obj in _generatorArray)
//...
if ([obj isEqualToString:[localGeneratorArray objectAtIndex:i]])

You're declaring obj as type id but then treating it as a NSString object in the body of the loop.

Xcode may not complain, but I will. You need to make one of the following changes.

You can change the declaration to NSString *:

for (NSString *obj in _generatorArray)

Or, you can change the comparison method:

if ([obj isEqual:blahblahblah])

The former is probably better because we all know there's nothing but strings in that array. But the latter is fine too. isEqual: is an NSObject method, so all Objective-C object respond to it. In the case of NSString, it does almost the exact same thing as isEqualToString:. I think internally it first does a type check (after the pointer comparison) to see if the compared objects are of the same type (because if they're not, they're definitely not equal).

The main point here is, if we know everything in the array is a string, we should declare the iterator object as that type of object. If we don't know this, however, then we can't be calling isEqualToString on every object in the array, because only NSString and its subclasses respond to that selector.

Finally, you can make your loops a little more efficient by adding a break; statement after the spots where you set the BOOL variables to YES. There's nothing in the loop that sets it back to NO, so why continue iterating through the loop? Once it's YES, we know the final outcome and can stop checking.

Answer 2

I'm no Obj-C expert, but it looks like you're working much, much too hard here.

For one, this could easily be a straight-up function; no class needed. Or, you make it a static method. Or - likely overkill - create a singleton DTNameGenerator instance. But certainly less overkill than the current solution.

My point is that there's no need to instantiate an object here. There's no internal state, no real dependencies; only fixed (hard coded) input (the name fragments), and an output.

And I'm not sure what's going on with the underscored variable names everywhere; no need for that here either.

Moreover, things can be improved by simple looping. Say you build your array of name fragments like this (using the @[ ... ] shorthand for creating arrays):

NSArray *prefix    = @[@"Abu", @"Bra", @"Der", @"Eve", @"Far", @"Gar", @"Her"];
NSArray *repeated  = @[@"ne", @"we", @"re", @"ne", @"be", @"ge", @"te"];
NSArray *suffix    = @[@"we", @"re",@"mu", @"ne", @"ge", @"or", @"ar"];
NSArray *fragments = @[ prefix, repeated, repeated, repeated, suffix ];

Then you just need to loop through the fragments and pick a fragment from each nested array. To figure out if a particular fragment is already in the name, you can just check whether [nameArray indexOfObject:pick] == NSNotFound

Finally, those "types" are completely opaque and meaningless - what's "type 0" vs. "type 1"? I'm guessing it's male/female since we're talking about names. In that case call a spade a spade, and typedef the values you expect, e.g.

typedef enum DTGender {
  kDTGenderFemale,
  kDTGenderMale
} DTGender;

Then you can pass kDTGenderFemale or kDTGenderMale to your function or method, and it's obvious what's going on. Even if it's not specifically genders, you should still make it explicit what those "types" are.

In all, I get something like this (a more experienced Obj-C coder will probably scream, but it's still an improvement over the original code):

DTNameGenerator.h

#import <Foundation/Foundation.h>

typedef enum DTGender {
  kDTGenderFemale,
  kDTGenderMale
} DTGender;

@interface CRNameGenerator : NSObject
+ (NSString*)randomNameForGender:(DTGender)gender;
@end

DTNameGenerator.m

#import "CRNameGenerator.h"

@implementation CRNameGenerator

+ (NSString*)randomNameForGender:(DTGender)gender {
  NSArray *prefix;
  NSArray *suffix;
  NSArray *middle = @[@"ne", @"we", @"re", @"ne", @"be", @"ge", @"te"];

  if (gender == kDTGenderMale) {
    prefix = @[@"Abu", @"Bra", @"Der", @"Eve", @"Far", @"Gar", @"Her"];
    suffix = @[@"ne", @"re", @"mu", @"we", @"ge", @"or", @"ar"];
  } else {
    prefix = @[@"Wen", @"Pre", @"Mar", @"Nem", @"Car", @"Cat", @"Hel"];
    suffix = @[@"pe", @"ra", @"mul", @"nem", @"ge", @"ara", @"era"];
  }

  NSArray *fragments = @[ prefix, middle, middle, middle, suffix ];

  NSMutableArray* name = [NSMutableArray array];

  for(NSArray *parts in fragments) {
    // get a random index. The docs recommend using
    // arc4random_uniform() rather than the modulo trick
    NSUInteger i = arc4random_uniform((u_int32_t) parts.count);
    NSString* part = (NSString*)[parts objectAtIndex:i];
    if( [name indexOfObject:part] == NSNotFound ) {
      [name addObject:part];
    }
  }

  return [name componentsJoinedByString:@""];
}

@end

And you just have to call [DTNameGenerator randomNameForGender:...] to get a random name. Of course, the hard coded name fragments are still pretty iffy; would probably be better to load those from somewhere else or otherwise have a way to define them at runtime.