0

I have a Objective-C class called FactorHelper whose definition is below. It has a property called factors which is an NSMutableArray of NSNumbers. I have a custom isEqual: method in this class that returns true if the factors property in the two FactorHelper objects have the same numbers (even if the numbers are in a different order).

I tried to test by creating two FactorHelper objects one with 10,5,2 and the other with 10,2,5. Then I created a NSMutableSet, added firstObject and then second object. I was expecting second object to be not added, but I see that it is added. When I step through the code I find that isEqual is being called by addObject and is returning TRUE. What am I doing wrong?

UPDATE

Changing the [NSMutableSet new] to [NSMutableSet alloc] init] makes things work as expected.

Also, changing all the TRUE, FALSE is isEqual to YES, NO makes it behave correctly (even if I keep it as [NSMutableSet new]).

I have no idea what is going on. Can someone please shed some light?!

Class definition

@interface FactorHelper: NSObject
 @property NSMutableArray <NSNumber *> *factors;
 -(BOOL) isEqual:(FactorHelper *)other;
 -(instancetype) initWithFactors:(NSMutableArray *)factors;
 -(NSString *) description;
@end

@implementation FactorHelper

- (instancetype) initWithFactors:(NSMutableArray *)factors
{
    self = [super init];

    if (self) {
        _factors = factors;
    }

    return self;
}

-(BOOL) isEqual:(FactorHelper *)other
{
    if ([self.factors count] != [other.factors count])
    {
        return FALSE;

    }
    else
    {
        NSMutableDictionary <NSNumber *, NSNumber *> *myHashTable = [[NSMutableDictionary alloc] init];
        for (NSNumber *nextNumber in self.factors) {
            if(myHashTable[nextNumber] == nil)
            {
                myHashTable[nextNumber] = @(1);
            }
            else
            {
                myHashTable[nextNumber] = @([myHashTable[nextNumber] integerValue]+1);
            }
        }

        for (NSNumber *nextNumber in other.factors)
        {
            if(myHashTable[nextNumber] == nil)
            {
                return FALSE;
            }
            else
            {
                myHashTable[nextNumber] = @([myHashTable[nextNumber] integerValue] - 1);

                if ([myHashTable[nextNumber] integerValue] == 0) {
                    [myHashTable removeObjectForKey:nextNumber];
                }
            }
        }

        if ([[myHashTable allKeys] count] == 0)
        {
            return TRUE;
        }
        else
        {
            return FALSE;
        }

    }
}
@end

Unit test code

NSMutableSet *testSet = [NSMutableSet new];
FactorHelper *fact1 = [[FactorHelper alloc] initWithFactors:[@[@(10),@(5),@(2)] mutableCopy]];
FactorHelper *fact2 = [[FactorHelper alloc] initWithFactors:[@[@(10),@(2),@(5)] mutableCopy]];
[testSet addObject:fact1];
[testSet addObject:fact2];
NSLog(@"Are factors 1 and 2 the same: %d",[fact1 isEqual:fact2]);
Smart Home
  • 801
  • 7
  • 26
  • Where in your code do you use `NSMutableSet`? – Willeke Jul 30 '16 at 20:53
  • The testSet is a NSMutableSet. Added in the main code above. The code is within the main method of a OS X command line tool. See my answer below. Changing all the TRUE/FALSE to YES/NO fixed the issue. – Smart Home Jul 30 '16 at 20:56

2 Answers2

2

NSMutableSet is a hash value based set. You need to override hash method for its element types consistent with isEqual:.

In your case, something like this:

- (NSUInteger)hash {
    NSCountedSet *factorCounts = [[NSCountedSet alloc] initWithArray:self.factors];
    return [@"FactorHelper" hash] + [factorCounts hash];
}

I'm not sure how you checked if I see that it is added, but this makes your FactorHelper work with NSMutableSet.

By the way, your isEqual: can be implemented a little bit shorter utilizing NSCountedSet.

-(BOOL) isEqual:(FactorHelper *)other {
    NSCountedSet *myFactorCounts = [[NSCountedSet alloc] initWithArray:self.factors];
    NSCountedSet *otherFactorCounts = [[NSCountedSet alloc] initWithArray:other.factors];
    return [myFactorCounts isEqual:otherFactorCounts];
}

This shows clearer consistency with the hash above.

OOPer
  • 47,149
  • 6
  • 107
  • 142
  • Thanks @OOPer. One question: Do all the Apple collection classes have inbuilt "hash". For e.g If I have a custom class, CLassX, that has two properties a `NSArray *array` and a `NSInteger vale`. And I want to say that two objects of CLassX are same as long as they have elements in array and value. Do I just set hash to [self.array hash] + [self.value hash]? – Smart Home Jul 31 '16 at 02:59
  • @SmartHome, as far as I know, all collection types return hash values consistent with their `isEqual:` method. I mean, if `[col1 isEqual:col2]` returns TRUE, `[col1 hash] == [col2 hash]` always get TRUE. (This is the requirement for `hash` being consistent with `isEqual:`.) To implement `hash` with equality defined for (`NSArray *array`, `NSInteger value`), `[self.array hash] + (NSUInteger)self.value` will work. (There may be better implementations, but this is not too bad.) – OOPer Jul 31 '16 at 03:19
  • @SmartHome, maybe I need to note that "consistent does not mean efficient". To get a better performance with hash based collections, such as `NSDictionary` or `NSSet`, minimizing the possibility of "`[col1 hash] == [col2 hash]` but not `[col1 isEqual:col2]`" is needed. Default implementations of `hash` in Apple's frameworks may be far from best optimized for that purpose. – OOPer Jul 31 '16 at 03:45
  • Thanks @OOPer. Looks like I will just go with default Apple implementations until I get to a point where I have my own custom implementations of a complex data structure. – Smart Home Jul 31 '16 at 19:41
  • You could implement `hash` to return `1` and your set would start working. It wouldn't work very efficiently, but it would work! The reason it doesn't work now is that two `equal` objects are winding up in two different hash buckets, so the runtime looks to see if there is anything already in this bucket, finds nothing, and doesn't even bother to call `isEqual` as there is no need. – matt Jul 31 '16 at 19:52
1

Your code was never working, even if it sometimes appeared to.

The problem is that a custom implementation of isEqual is not the only requirement for making a class work in a Set. Think: what is a Set? It is a hash table. So you must also provide a matching custom implementation of hash — and you have not done that.

The requirement for hashability is: the hash value of two objects must be the same if those two objects are equal.

matt
  • 515,959
  • 87
  • 875
  • 1,141