TechEcho

Let me ask you: What do you think this code does?<pre><code> resizeImage(imagePath, 300, 200, true, true, 1) </code></pre> It resizes image... but what exactly does it do? For the most part, we cannot tell without looking up the function definition.Let's say you are reviewing a PR and it includes this change:<pre><code> -resizeImage(imagePath, 300, 200, true, true, 1) +resizeImage(imagePath, 300, 200, false, true, 1) </code></pre> Can you confidently say what is the impact of this change? For the most part... no. You need to know what each positional argument does.Let's say you know that the interface is:<pre><code> function resizeImage( imagePath: string, width: number, height: number, upscale: boolean, crop: boolean, quality: number, ): Promise<Buffer> </code></pre> But now a PR introduces a change to the parameter order (e.g. to make it consistent with other functions):<pre><code> function resizeImage( imagePath: string, width: number, height: number, + crop: boolean, upscale: boolean, - crop: boolean, quality: number, ): Promise<Buffer> </code></pre> How do you review this change? Sure, reviewing the interface diff is easy, but what about the dozens or hundreds of diffs that update function invocation?<pre><code> -resizeImage(imagePath, 300, 200, true, false, 1) +resizeImage(imagePath, 300, 200, false, true, 1) resizeImage(imagePath, 300, 200, false, false, 1) -resizeImage(imagePath, 300, 200, false, true, 1) +resizeImage(imagePath, 300, 200, false, false, 1) -resizeImage(imagePath, 300, 200, true, false, 1) +resizeImage(imagePath, 300, 200, false, true, 1) </code></pre> Hopefully the problem is self-explanatory: Positional arguments create a breading ground for hard and even impossible bugs to catch/debug/fix, esp. when code needs to be refactored. Fear not though as there is a better way.Let's start from the start, but this time use a single-object parameter:<pre><code> resizeImage({ imagePath, width: 300, height: 200, upscale: true, crop: false, quality: 1, }) </code></pre> Can you tell what is the intention behind this code? Yes, you can get a good sense, even if you are not familiar with the implementation.Can you easily refactor the interface? Yes, linter will warn you if contract is not satisfied.We end up with positional arguments because they feel the most natural to start with. However, as functions grow in scope, what started as a simple function with 1-2 arguments becomes an unreadable mess.This is where [`max-params`](https://eslint.org/docs/latest/rules/max-params) comes to the rescue. Simply adding an ESLint rule that restricts having functions with more than X parameters ensures that your code remains legible and easy to refactor as your codebase scales.

You are going from one extreme to other and in my experience this will end up messy, unless interface in question really allows for all possible parameter combos. I'd rather ask which params are required and give a serious thought to setting up sensible defaults for the rest, so that you can end with something like this:<pre><code> resizeImage(imagePath, 300, 200, {crop: true}) </code></pre> Python use this pattern everywhere, with keyword params it just feels natural.

The ultimate perpetrator in this example is to have more than one positional (i.e. unnamed) boolean parameter. Even having a single positional boolean is a lot of the time bad design. I'm aware of one piece of (JS) software (<a href="https://chevrotain.io/docs/" rel="nofollow">https://chevrotain.io/docs/</a>) whose entire API is (apparently) built on the principle that each method accepts a single argument (an object with key/value pairs) and returns a single value (of the same shape). Personally, I try to somewhat keep to the first part of that pattern, but don't quite manage to—quite a few methods have very obvious mandatory, first parameters that I then often accept as positionals as in `f = ( image, cfg ) => ...`.In this particular example, though, one might argue that picture data may be communicated in many different ways: as an URL, an FS path, a buffer, a stream...; question is: do you really want your `resizeImage()` method to read from the network or from the file system? I'd say that this is probably in violation of the principle of separation of concerns. The best choice for the first, mandatory argument is probably a stream of octets in this case, and when you can't be 100% sure, then indeed the way to design such an interface is to first constrain the method to accept a single object (dictionary) of properly named values, and at first only implement `{ stream: ..., width: ..., height: ..., ..., }`. Later on, when the software has shown it's up to the task, it will almost always be possible to introduce new parameters (say, `path` and `url`, all exclusive with `stream`), and, if need be, allow `resizeImage stream, { width: ..., ..., }` as a variant.

The contract here can become somewhat casual. If "crop" is optional and I include "crup: true" would that be considered an error? Also, outside of say JavaScript, what happens to parameter types?

The most important code linting rule: max-params

3 comments

The most important code linting rule: max-params

3 comments