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

"Function arguments (2 or less ideally)" example seems dubious #70

Closed
adamshone opened this issue Jan 7, 2017 · 7 comments
Closed

"Function arguments (2 or less ideally)" example seems dubious #70

adamshone opened this issue Jan 7, 2017 · 7 comments

Comments

@adamshone
Copy link

Hey, thanks for the work on this - very nice!

The suggestion of using a config object to reduce the number of function arguments does not solve the underlying problem and is really just cheating - the function still takes four inputs and you still have to test every permutation, it just pretends to only take one input :)

In my opinion it's actually worse because rather than declaring the parameters explicitly it presents the consumer with an opaque method signature:

function createMenu(menuConfig)

If I'm writing code that calls this function I now have to read the full implementation of the function to find out which magic property names are expected within the config object, rather than just read the signature. Or you could add jsdoc for the function, but then you've got a maintenance problem.

There must be a better way to reduce the number of parameters, maybe currying?

@jljorgenson18
Copy link

This suggestion has less to do with the total number of data points and more with how the method is called. The issue you run into with a bunch of arguments is that when calling the function, it is difficult to keep track of what argument corresponds to the right parameter. For the most part, it doesn't make a huge difference in the function declaration.

I wrote about this a little bit here to show how many parameters can cause problems.

@ghengeveld
Copy link

As usual, this is mostly personal preference. I'd argue that passing an object is preferable most of the time due to the fact that it avoids the need to deal with argument ordering. It also makes the function signature easier to change because arguments can be added and reordered without having to update callers.

What tripped me up with this one is the phase "Zero arguments is the ideal case". A function with zero arguments is a code smell in my opinion, because it must mean that this is not a pure function. It either accesses variables declared outside of its own scope, or mutates outer scope somehow (side effects). Both are awful, the latter in particular.

@adamshone
Copy link
Author

Both fair points, and yes I have also felt the pain of having to call functions like:

foo('bar', null, null, null, 'value I care about');

The ideal solution to that would be Python style keyword arguments :)

I agree that it's mostly personal preference. I would argue that functions taking a config object are less correct because they sometimes require the developer to find a usage of the function somewhere in the codebase to get an idea of what values are expected, whereas that information really belongs with the function itself (i.e in the signature). Obviously if the function is fairly simple this won't be necessary, but in some cases the function is complex or lives in a third party library.

If nothing else I would suggest rewording or removing the justification for this rule:

Limiting the amount of function parameters is incredibly important because it makes testing your function easier. Having more than three leads to a combinatorial explosion where you have to test tons of different cases with each separate argument.

Having stated that limiting the number of parameters is essential to avoid a test case explosion, it then offers a solution that does nothing to mitigate the problem.

@jljorgenson18
Copy link

@ghengeveld While it is often true that zero argument functions are written in a way such that they mutate or access variables in a higher scope, that is not always the case. A creator function that is used to return new instances of an object is often used in a way to make the app more immutable. You see this pattern in redux a ton

export const getDocuments = () => {
  return {
    type: 'getDocuments',
    fetch: true
  };
};

Purity is also ideal but not overly realistic. Mutating variables in upper scopes is one thing but accessing them is a different situation all together. There is a distinction between "state variables" in upper scope and everything else. Accessing state variables should be frowned upon and that state should be passed in as an argument. However, accessing additional functions, global constants, private variables inside of a closure, or any global functions are all referencing items in upper scopes. If you are accessing non state variables, there is a ton you can do with no arguments in creator functions to build out complex objects. Purity should be something that if given the opportunity, you should implement but non-pure functions should not be considered a code smell.

@ghengeveld
Copy link

Those are valid points. The sentence tripped me up because it suggests that zero arguments is something to strive for, which I don't think is a good idea. That doesn't mean there's no valid use cases for zero argument functions, as you mentioned.

@M1ke
Copy link

M1ke commented Jan 8, 2017

I agree with this; having an object/array replace multiple parameters just confuses things more, unless the object can have a known signature itself (i.e. is an instance of a class, such as in ES6, TypeScript or an alternative dynamic language). A good IDE can help when calling func('abc', 1, 2, null, false, 10) and tell you the parameter names which lets you develop almost as fast as having fewer params you can remember.

@ryanmcdermott
Copy link
Owner

See the following issue for a final thought on this subject: #85 (comment)

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

5 participants