Cappuccino Cappuccino → 0.6.5

Comments and changes to this ticket

• 

  Ross Boucher October 19th, 2008 @ 05:30 PM

  Hey Bailey,

  Apologies for taking so long to review this. Here are my comments so far. There are a lot, but don't let that discourage you, the balance of this was really good.

  The ivar declarations shouldn't have type "var". Looking at them, they should probably be CPArray and unsigned.

  There's a lot of duplication in the initializers. Typically, in Objective-J, you should do all the initialization in one "designated initializer", and then call that initializer from all the others, taking only whatever extra steps are necessary. This isn't always possible, but it usually is.

  Similarly, initWithSet should call [set copy], so that at the very least its a different object. We haven't built in deep copies yet, so don't worry about that.

  This check isn't necessary:

  
  		if (_contents.hasOwnProperty(property))
  			array.push(_contents[property]);
  

  In fact, if anything, it should be the reverse check -- you don't want to include properties that are on the Object prototype. hasOwnProperty means that the property belongs to the type, not the individual object.

  In intersectsSet, you loop like this:

  for (var i = 0; i<items.length; i++)
  
  

  But this incurs a property access for the length lookup on every iteration. This value should be cached, something like thi:

      for (var i=0, count=items.length; i<count; i++)
  
  

  Actually, its usually better to just use reverse iteration and a while loop so you only need one loop variable, not two.

  In - (BOOL)isSubsetOfSet:(CPSet)set, you should just return NO immediately, and then return YES at the end, rather than introduce an additional variable.

  With - (void)makeObjectsPerformSelector:(SEL)aSelector, we have the code duplication problem again. This should just call the withObject:version with a nil object.

  In general, if you can find places where you can reuse common methods, its best.

  One style note, I can't tell for sure, but it looks like these are all tabs -- we only want to use spaces (4), to keep compatibility between editors.

  Overall this is a great start, and if we can get these issues fixed, I think we'll be able to move it into the project.

  Oh, and we'll be switching comments to use /!, rather than just /, so you may want to change those. The more documentation we have the better.

• 

  Ross Boucher November 9th, 2008 @ 10:56 AM

  Any update on this?

• You flagged this item as spam.
  

  Boog November 23rd, 2008 @ 12:48 PM

  • Assigned user set to “Ross Boucher”

  Hey Ross, sorry it took me so long to get back on this. I guess I'm a bit confused about some of your remarks.

  First, I changed my count instance variable to unsigned but I designed contents to be a JS Object, not a CPArray, is there a type I can use instead of var for that?

  Second, I'm not sure I'm seeing the duplication in my initializers. All my inits call [self init] then any init specific code to that initializer. Is there something I'm missing? Can you give me an example?

  Third, according to my reference book, hasOwnProperty "returns true if the object has a particular property. The hasOwnProperty method does not look at the prototype chain". So from what I've seen it should not look at the Object prototype, only the object itself which is exactly what I wanted. I'm storing the contents of the Set in a JS Object with the hashcode as the property name. I thought that would be more efficient than an Array for indexing purposes.

  I don't understand what you mean by, "With - (void)makeObjectsPerformSelector:(SEL)aSelector, we have the code duplication problem again. This should just call the withObject:version with a nil object" What is the withObject:version selector you're referring to?

  I made the other changes you suggested. Thank you, and sorry I was so slow on this.

• You flagged this item as spam.
  

  Boog November 23rd, 2008 @ 12:53 PM

  Woops, I understand your problem with makeObjectsPerformSelector:. I overlooked the method. I've fixed that.

  • CPSet.j 10.9 KB
• 

  Francisco Tolmasky November 24th, 2008 @ 04:00 AM

  in setWithObjects and initWithObjects: you loop through the entire arguments array. This is incorrect because item 0 in arguments is self, and argument 1 is _cmd. u want args 2 through length. Also, you should check for nil termination since people coming from cocoa are used to this (and since nil can't be placed into the set anyways). CPArray has this behavior implemented already so it should be easy to copy:

  • (id)initWithObjects:(Array)anArray, ... { // The arguments array contains self and _cmd, so the first object is at position 2. var i = 2,

      argument;
    
    

    for(; i < arguments.length && (argument = arguments[i]) != nil; ++i)

    push(argument);
    
    

    return self; }
• 

  Francisco Tolmasky November 24th, 2008 @ 04:00 AM

  Which reminds me, it would be nice if you included test cases with this too.... :)

• You flagged this item as spam.
  

  Boog December 1st, 2008 @ 02:39 PM

  I'm not sure how much time I'll find for modifying this code. If either of you want to take it and apply the changes you see fit, feel free. I hope I gave you a good start.

• You flagged this item as spam.
  

  Nabil Elisa December 1st, 2008 @ 03:57 PM

  Hi all, I'm new to Objective-C/J and Cappuccino but I thought I'd have a go at this. I've tried to complete those changes that I understood from the above comments.

  I did not made any test cases because : 1) I don't know how (got an example handy I can study?) 2) I'm not familiar enough with this class to know what to test

  Not sure if I'm skilled enough to make any further changes but if you guide me through it I'd be glad to put the time in. Anyway, all feedback is welcome.

  • CPSet2.j 10.9 KB
• 

  Ross Boucher December 5th, 2008 @ 06:26 PM

  This is looking better.

  A few more comments:

  
      if (![self containsObject:anObject])
      {
          _contents[[anObject hash]] = anObject;
          _count++;
      }
  

  There's no need to do this check, because in the worst case you overwrite an object with itself. The check makes the code slower in every other case. I think there may be a few other similar cases like this.

  Also, the implementation of containsObject should just return the expression in the if block, rather than if (expression) return YES; return NO;

• You flagged this item as spam.
  

  Nabil Elisa December 7th, 2008 @ 02:07 PM

  Ok, I had another go and changed what you mentioned.

  I also changed the imports to the new syntax and applied the for (var i = 0; i<items.length; i++) to for (var i=0, count=items.length; i<count; i++) trick to a couple more places. I believe it's more efficient (for speed) that way, even if only by a tiny amount.

  • CPSet2.j 10.7 KB
• 

  Ross Boucher December 17th, 2008 @ 04:03 PM

  • Milestone cleared.
  • State changed from “new” to “resolved”

  Alright, CPSet is now part of Foundation! Thanks for the help guys. It still needs to become CPCoding compliant, and could probably benefit from some OJUnit tests. So, there's room for further work if you're still interested.

  There's plenty of other stuff to work on too :)

• 

  Francisco Tolmasky December 22nd, 2008 @ 11:01 AM

  • Milestone set to “0.6.5”