Random Name Generator

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 

@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];
_middleString1Arra

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.

• An enum, as you already mentioned, defining the types of names to generate.

• A factory method: + (instancetype)nameCreatorWithType:(int)type;

• I can't see why your arrays other than the localGeneratorArray in createName would need to be mutable. You should change these to NSArray.

• 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.

Xco

StackExchange Code Review Q#46566, answer score: 4