Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnamed method parameters should be replaced #1

Open
tonyarnold opened this issue Jun 17, 2012 · 3 comments
Open

Unnamed method parameters should be replaced #1

tonyarnold opened this issue Jun 17, 2012 · 3 comments

Comments

@tonyarnold
Copy link

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
Copy link
Owner

mpw commented Jun 17, 2012

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
Copy link
Author

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants