0

I want to check if a JSON object is an NSString and if it isn't, assign it a default string. My ultimate goal is to prevent crashing and assign the properties a proper value no matter what. This is an example of a data model I am using where dict is the JSON dictionary the API returns.

 Data *data = [[self alloc] init];
 data.name =  [NSString validateString:dict[@"name"] defaultString:@""];
 data.status = [NSString validateString:dict[@"status"] defaultString:@"OPEN"];

Here is the category method validateString I am using.

+ (NSString *)validateString:(NSString *)aString defaultString:(NSString *)defaultString {
    if ([aString isKindOfClass:[NSString class]]) {
        return aString;
    }
    return defaultString;
}
Curt Rand
  • 1,025
  • 1
  • 9
  • 27
  • 2
    If your ultimate goal is to prevent crashing, please do checks for null values for JSON. – Harjot Singh Oct 20 '18 at 14:54
  • Does the API send such inconsistent data? – vadian Oct 20 '18 at 14:59
  • @HarjotSingh `isKindOfClass` is enough for checking null values – trungduc Oct 20 '18 at 14:59
  • @vadian. No, it's been consistent. However, after programming in swift, it seems best practice to never use the bang operator and make sure the object is what you expect. – Curt Rand Oct 20 '18 at 16:02
  • Objective-C is not Swift. And even in Swift you can force unwrap if the data is consistent. *Never use the bang operator* is wrong. There is more than black and white. – vadian Oct 20 '18 at 16:07

2 Answers2

2

It makes no sense, and is very bad practice, to cast (NSString *)aString and then ask if this is in fact an NSString.

Also, what if it is nil?

All you know when you fetch from a dictionary is that you get an id. Do not assume more than that.

I would suggest writing very plainly: say what you mean, and mean what you say. That is the best practice in Objective-C. Otherwise, dynamic typing and "nil trickery" can lead you into subtle errors. You might not have any trouble in this particular case, but bad habits are bad habits, and it is best not to let them form in the first place. I'd rewrite like this:

+ (NSString *) checkType:(nullable id)obj defaultString:(NSString *)def {
    if (obj == nil || ![obj isKindOfClass:[NSString class]]) {
        return def;
    }
    return obj;
}
matt
  • 515,959
  • 87
  • 875
  • 1,141
0

Like mentioned in other comments: if you want to prevent crashes, you also need to check if it's nil, specially if there is a chance to port your code to Swift in the future.

Just to clarify my last sentence, the line below works in Objective-C even if aString is nil:

if ([aString isKindOfClass:[NSString class]]) {

That's because, in the way Objective-C was made, calling a function on a nil object returns nil, so the if will be considered false, and the function will return defaultString. Yeah... that's certainly a bad idea when they created Objetive-C, since this leads to lots of errors. More details about that behaviour below:

https://stackoverflow.com/a/2696909

Anyway, it's also a good practice to only cast an object after checking its type, so I would recommend adapting your function to this:

+ (NSString *)validateString:(id)obj defaultString:(NSString *)defaultString {
    if (obj != nil && [obj isKindOfClass:[NSString class]]) {
        return (NSString*)obj;
    }
    return defaultString;
}

Every object that implements NSObject* has isKindOfClass: (and NSDictionary* only stores objects that implement NSObject*), so we don't need to check if the object responds to it. Also, even if we wanted, respondsToSelector: is also an NSObject* function.

Still, the method that you are using still works. The revised function above is just adapted to better practices and to avoid problems in case you ever need to port this code to Swift (or any other language) in the future.

EDIT: Updated code based in @matt's suggestion.

VitorMM
  • 1,060
  • 8
  • 33
  • The link in your question doesn't say that `this leads to lots of errors`. He said `it's not an error`. I can call it like **an Objective-C feature**. `aString != nil` is useless in this case. – trungduc Oct 20 '18 at 15:42
  • 1
    I still don't like your advising him to say `(NSObject*)aString`. First of all, `id` will do. Second, the name `aString` is assuming exactly what we don't know. I would advise something more like `(id) obj`. – matt Oct 20 '18 at 15:42
  • 1
    @trungduc The OP asked about "best practices". Opinions can differ. In my opinion, assuming the behavior of `nil` is bad practice, just like assuming the type of something without checking it when using dynamic typing. I've certainly run into trouble when behaving that way. So I don't think you should advise the OP to use that sort of trickery, even if it is standard trickery. – matt Oct 20 '18 at 15:44
  • @matt So we should discuss about what is "best practices". With me, "best practices" is which everyone uses widely and effectively. If you said it's trickery, do you think he should have an `else statement` for `return defaultString;`? – trungduc Oct 20 '18 at 15:49
  • @matt Yeah, I guess your suggestion is pretty reasonable. In fact, I just used `aString` because it was already there. I will update my answer. – VitorMM Oct 21 '18 at 16:13
  • @trungduc assuming `nil` behavior is as much a bad practice as taking use of the fact that `BOOL` is an 8 bit variable. It's part of the language, but someone might have to migrate your code in the future, and that someone may not be aware of the language peculiarities. You may not make a mistake, but you need to worry about other people using your code as well. – VitorMM Oct 21 '18 at 16:38