[Discussion] class-methods-use-this

[davidcornu]

https://github.com/Shopify/javascript#12.13

• 12.13 Instance methods that don’t refer to this don’t need to be instance methods. If they relate to the class, make them static methods; otherwise, make them functions in scope.

  Why? This pattern is generally a sign that you are providing a bad public API for the class, and should either hide this method (if it’s an implementation detail) or export it as a utility method.

  ESLint rule: class-methods-use-this

  // bad
  class BadClass {
  badMethod(string) {
    console.log(string.toUpperCase());
  }
  
  anotherMethod() {
    return this.badMethod('oops!');
  }
  }
  
  // good
  function goodFunction(string) {
  console.log(string.toUpperCase());
  }
  
  class GoodClass {
  anotherMethod() {
    return goodFunction('oops!');
  }
  }

---

This rule bothers me a little. While I understand the intent, moving instance methods to named functions outside of the class

1. Doesn't automatically make them reusable (their functionality could be specific to what the instance does
2. Makes code harder to read as you can no longer group related functions together
3. Makes classes harder to subclass (as you have no way of reusing parts of the internal implementation) - this is probably a good thing though
4. Forces you to export those methods if you want to test them directly (which makes the module surface area larger than it needs to be)

5. Why? This pattern is generally a sign that you are providing a bad public API for the class, and should either hide this method (if it’s an implementation detail) or export it as a utility method.

   While I agree this would be useful in the case of libraries/SDKs, we already signal internal vs external methods using the leading underscore convention

   • 2.3 Use a leading underscore when naming "private" properties. Functions and variables in scope should be named normally.

     Why? The leading underscore sends a signal to other developers that these methods should not be called or relied upon. Some tools can also obfuscate methods with leading underscores to ensure that they are not called by outside objects. Items in scope but not exported are completely private, so no signaling is required for these.

     Note: Use these underscore-prefixed members as a last resort. Prefer moving them to be functions/ variables in scope or making them part of the public API over using this naming convention.

   Also, TypeScript should help with this in the long run.

---

For instance, in the following class

class ListComponent {
  constructor(items) {
    this.items = items;
  }

  render() {
    const list = document.createElement('ul');
    this.items.forEach((item) => list.appendChild(this._renderItem(item)));
    return list;
  }

  _renderItem({body, id}) {
    const item = document.createElement('li');
    item.innerText = body;
    item.dataset.id = id;
    return item;
  }
}

_renderItem() would caught by the class-methods-use-this linting rule, forcing the author to refactor to something like

function renderItem({body, id}) {
  const item = document.createElement('li');
  item.innerText = body;
  item.dataset.id = id;
  return item;
}

class ListComponent {
  constructor(items) {
    this.items = items;
  }

  render() {
    const list = document.createElement('ul');
    this.items.forEach((item) => list.appendChild(renderItem(item)));
    return list;
  }
}

While we now have the advantage of having renderItem being completely private, it's also completely outside of it's usage context and we leave it up to the reader to jump between both and figure out how it's being called.

[perrupa]

Great idea creating an issue for this topic, this has come up once or twice before and would be nice to have a canonical issue to reference when this decision becomes questioned.

My take on this:

1.

Doesn't automatically make them reusable (their functionality could be specific to what the instance does

Increased reusability isn't the intent of this rule. I believe it is more to make you question whether your method is actually dependant on the instance (using this) or not. If not, the behaviour of the function likely shouldn't be available as a method on the instance.

2.

Makes code harder to read as you can no longer group related functions together

I think this the core of the reason for this lint rule and would like to challenge your assumption here. It was an improper grouping to begin with, the fact that the function does not contain any reference to this is the smell that the function isn't related to the instance and should be pulled off the class.

There is however nothing to stop you from grouping related helper functions outside the class

const AwesomeHelpers = {
  doThing: options => { /* ... */ },
  doOtherThing: (arg1, arg2) => { /* ... */ },
}

export default class AwesomeClass {
  // ...
}

3.

Makes classes harder to subclass (as you have no way of reusing parts of the internal implementation) - this is probably a good thing though

I agree, this is a good thing. If the subclass needs that helper function you can always export it from the original file as well as the class, or include it in a separate file containing related helpers.

This is JS though, so we should always consider other reuse patterns like mixins or composition. I've always preferred the philosophy that inheritance is more of a pattern for deviation/special cases, vs reusability.

4.

Forces you to export those methods if you want to test them directly (which makes the module surface area larger than it needs to be)

I would disagree here. The private, not exported function does not need to be tested. We should only be testing the public interface against our use cases. As @m-ux has said before: "test the interface, not the implementation"

5.

Why? This pattern is generally a sign that you are providing a bad public API for the class, and should either hide this method (if it’s an implementation detail) or export it as a utility method.

While I agree this would be useful in the case of libraries/SDKs, we already signal internal vs external methods using the leading underscore convention

The leading underscore for private methods is really more of a shitty workaround for not actually having private functions available in ES5. Given that we can now hide them inside modules using closure, it's less of a point and we should probably stop using this leading underscore pattern.

If a function is hidden/not visible there is no longer a need to differentiate it to the consumer of your API.

---

My main takeaway from this rule is that it challenges your assumptions about a specific instance method and tells you that you're wrong without emotion or tact. You should embrace this robotic calousness as a chance to rethink how and why you wrote that function in the way that you did.

Overall I think we can and should be shipping the minimal functional interface rather than adding confusing public methods that are privately named and hoping that no one implements them.

A designer knows he has achieved perfection not when there is nothing left to add, but when there is nothing left to take away. -- Antoine de Saint-Exupery

edit: I didn't mean to imply @davidcornu is wrong, just whoever was encountering the rule at a given time. David is an amazing person and we all love him

[lemonmade]

@perrupa did a good job of voicing my usual points in favour of keeping this rule. In general, I have seen this rule be a problem in the following cases:

1. Someone trying to write a JS class as they would a Ruby class, where you are expected to attach all of your functions to some sort of class. This is not necessary in JS because we have a different method of organization (modules), and because "floating" functions are a more expected part of JS.
2. Someone using a base class as an interface, where they expect subclasses to override empty/ error-throwing versions of the methods in the superclass. This is definitely a code smell in JS, I would question someone doing even one additional level of class hierarchy in almost every case
3. Needing a method to stub out interaction with some global in test (for example, a method that sets window.location, which is the most common one I have seen). This is not strictly necessary in most case (can stub out global functions, though not always consistently in every environment), but in some cases it is unavoidable

Of these three, I think that 3 is acceptable, but the other two are legitimately bad patterns that this rule actively discourages. I like rules that generally encourage good code patterns, so my preference would be to keep this rule.

(as an aside, TypeScript does have private members, but we have still been moving functions that don't reference this off the class. It cleans up the class, and methods that don't need this are often either incomplete, rely on a global that will make the component hard to test, and/ or are a piece of functionality that many components could use, so it makes sense as a utility)

[davidcornu]

There's a solution to this 😛 https://github.com/Shopify/shopify/search?utf8=%E2%9C%93&q=class-methods-use-this

[arapehl]

Consistency and readability are paramount and this rule makes code far less consistent and far less readable by requiring some private methods (but not all) to be broken out into floating functions.

Having floating functions as generic (globally or locally scoped) helpers makes sense. Having floating functions performing work that’s specific to a class does not. It’s the quintessential example of blindly adhering to a rule for the sake of that rule.

Some real effects of this rule on my code, which has been evolving as requirements change, have been:

1. Having to convert and move private methods because they no longer refer to this, affecting the coherence and readability of the module.
2. Having to remove unit tests that were testing the now converted method, reducing the coverage of the module and making it more brittle.
3. Having to convert functions back into private methods because they’re now referring to this again.
4. Having to add back tests that were previously removed.

And as far as unit testing private methods goes, when 75% of my module’s functionality are private methods, I’m going to unit test them to make sure things don’t break as my module evolves. I need to protect myself against myself over time.

[lemonmade]

I feel all of your points to not go to addressing my central concern about removing this rule. I would ask you the following for the effects this has on your code:

1. Why do your methods no longer refer to this? Are you sure you actually need a class if a significant number of methods do not use this?
2. Why do you want to test private members? This is considered a smell in almost every language, it just happens to be easy to do in JS. Why aren't you just testing your public API? Are the methods you are breaking out really just utilities that should be separately tested as pure functions?
3. When does this happen? Are you sure your class is splitting up code in an appropriate way (it's usually better to isolate stateful code to a pretty small number of places, not have it in every method).
4. Not sure I totally understand this one.

As I noted in my original response, I think there are some cases where this can make sense, but I feel they are generally exceptions, and that the common case here is helping to identify smelly code.

[lemonmade]

Honestly, this rule causes enough headache that I'd be fine taking it off. The thing I don't want to lose is that people continue to ask the questions I asked above when they see methods that don't use this — it's not always a code smell, but often is. If the team has strong agreement on this being the right way forward, please make the PR and add some content to the style guide that makes it clear what situations a method without this makes sense in, to generally limit how many methods mutating this, and what situations you should a) not use a class or b) use utility functions instead.

[davidcornu]

Did some digging to figure out how to update the rule.

• We have it configured as a warn which shouldn't affect the exit code,
• But then we run ESLint with --max-warnings=0

I would have expected warn rules to be used to suggest that the offending code might be a code smell rather than enforcing the rule all the time (which is exactly what we want, based on the above discussion). They should show up in your editor but not prevent you from merging.

@lemonmade should I remove the rule altogether or should we update the rules to correctly reflect what should be a warn and what should be an error?

[GoodForOneFare]

For me, the rule flags meaningful information. As @lemonmade detailed, there are valid alternative approaches to consider.

If you've validated your approach, or you're just quickly iterating in a branch, the rule can be disabled, no?

[lemonmade]

@davidcornu we have a weird split between error/ warning that comes out of Atom originally doing horrible things if you just have everything as errors. The basic rule of thumb is that everything we turn on should fail CI, warnings should be for things that might logically come up during the development flow that are not likely to be logical errors (so, most stylistic rules). If we are turning it off, we should turn it all the way off.

[davidcornu]

Was resolved in https://github.com/Shopify/eslint-plugin-shopify/pull/45 https://github.com/Shopify/javascript/pull/266

Closing