search

mpw / MPWDrawingContext

An Objective-C wrapper for CoreGraphics CGContext
101 stars 9 forks source link

Unnamed method parameters should be replaced #1

Open tonyarnold opened 12 years ago

tonyarnold commented 12 years ago

This looks like a great project, Marcel — I noticed in some of the samples you've posted to the mailing list and elsewhere that some of the methods use unnamed parameters. ie.

-(id <MPWDrawingContext>)translate:(float)x :(float)y;

This is not great practice, and I feel you'd be better off using something simpler like:

-(id <MPWDrawingContext>)translate:(CGSize)size;

I don't think a change like this violates what you're trying to achieve here, but it also means that you're not off the beaten path of unreadable method names.

mpw commented 12 years ago

Hi Tony,

thanks for the feedback! The issue you raise is one I've been struggling for some time and going back and forth as well.

There were two issues I had with just plain -translate:

  1. You always have to create those structures, so the triangle example becomes:

    [context moveto:NSMakePoint(0,0)]; [context lineto:NSMakePoint(100,0)]; [context lineto:NSMakePoint(50,50)]; [context closepath];

The NSMakePoint()s really start to drown out what you're trying to do, similar to the CGContext... prefix when using CG functions.

  1. I'd like to save the save the plain versions ( moveto:, lineto:, etc.) for an OO variant of the API that can take various different forms, for example an array of coordinates to create a polyline. (I actually renamed -rect:, which does take an NSRect, to -nsrect: in the last minute due to this). We could conceivably add such 'ns' variants, so nstranslate: , nsscale:, but I am not sure that's a good idea.

I also considered other alternatives, such as naming the parameters, but that also got awkward:

-translateX:10 y:20.

Just looks ugly, if you ask me. Coming from a Postscript background, I never found 0 0 moveto 100 0 lineto 50 50 lineto closepath confusing in terms of what is x and y, so I finally decided to just leave the parameters unnamed, especially since that is used consistently throughout the API. If you see something:100 : 100, you can be sure that this will be a coordinate pair.

So yes, the point you raise is a cognizant one, and I am aware that the solution I arrived at is debatable. I just haven't found a better one, yet.

tonyarnold commented 12 years ago

All fair points, @mpw. Given that most of this really is stylistic, I guess it doesn't matter too much so thankyou for taking the time to explain your thinking.

Motti-Shneor commented 5 years ago

actually -translateByX:10 andY:10 is not ugly. It is self explaining, and closer to human language. Even if a little longer to write, (that's ObjC...) I would always prefer reading such code. -translate:10:20 forces me to either jump to the headers time and again to see what are the first and second params, or to memorize the signatures of too many API calls.