vuejs / vuex

🗃️ Centralized State Management for Vue.js.
https://vuex.vuejs.org
MIT License
28.42k stars 9.57k forks source link

Some code examples in documentation are mentally exhausting #1122

Closed TheJaredWilcurt closed 6 years ago

TheJaredWilcurt commented 6 years ago

What problem does this feature solve?

There are many code examples in the documentation that are mentally exhausting. The first chunk of the documentation is okay. Once you start into the "Core Concepts" section though, the code examples begin to wreak of "I-am-so-clever" syndrome. The goal of these sections (State, Getters, Mutations, etc) should be to help the reader understand the concepts. When doing so, there should be simple, easy to parse code examples to act as more concrete forms of the concepts being taught.

But the examples are optimized for fewest lines of code when they should be optimized for ease of readability. I'm having to mentally break down what each one is doing because they don't follow the "one idea per line" rule. And at some point I just get so tired and exhausted that I can't understand it anymore.

So the examples being shown end up draining me of mental energy so I can't proceed any further into the concepts I actually came for.

What does the proposed API look like?

Here is an existing example I'd like to dissect:

getters: {
    // ...
    getTodoById: (state) => (id) => {
        return state.todos.find(todo => todo.id === id)
    }
}

First off, why is there a // ...???? What is that there for? I assume it's meant to say "In the real world, there would be other getters too". Not needed, get rid of it.

getters: {
    getTodoById: (state) => (id) => {
        return state.todos.find(todo => todo.id === id)
    }
}

Next, little rocket ships going into other little rocket ships is hard to parse both visually and mentally. The problem here lies in over abuse of arrow functions. They are designed with phantom words and exformation. Let's put those phantom words (function, return) back in so we are being clear and verbose about what is trying to be conveyed.

getters: {
    getTodoById: function (state) {
        return function (id) {
            return state.todos.find(function (todo) {
                return todo.id === id
            })
        }
    }
}

The above is no different than before, it's just functions with all the words put back in, but suddenly it starts to look off. This is a sign of arrow function abuse.

Let's try an experiment where we make the code as verbose as possible, naming every variable and having no shortcuts.

getters: {
    getTodoById: function (state) {
        function findTodoWithMatchingId (id) {
            const todos = state.todos
            const matchingTodo = todos.find(function (todo) {
                if (todo.id === id) {
                    return true
                }
                return false
            })
            return matchingTodo
        }
        return findTodoWithMatchingId
    }
}

So, from this experiment we can see that someone thought "I'm so clever, I bet I can squeeze 15 lines into 5!". Now, some squeezing is fine, some usage of arrow functions can make sense. Like with .find, or .filter, or other simple similar prototype methods. So, let's squeeze that verbose version down, but only a little, to get us somewhere between the two extremes.

getters: {
    getTodoById: (state) => {
        function findTodoWithMatchingId (id) {
            const todos = state.todos
            const matchingTodo = todos.find(todo => todo.id === id)
            return matchingTodo
        }
        return findTodoWithMatchingId
    }
}

With this approach, we prioritize naming variables to give meaning, and separating ideas out to individual lines to make skimming/parsing easier. There's definitely wiggle room around what should/shouldn't be compressed when aiming for ease of reading.

Now the downside of this approach is that you lose some hipster cred. But the upside is that it's verbose and clear, and can be skimmed to understand what is happening. You don't need to mentally unpack 15 lines of meaning out of the 5 lines given. There is no guesswork going on while trying to keep track of the obscured meaning.

Anyone can look at the last example shown and see ways to compress it and write it differently. If a reader copies that code as the basis of their own code, they'll likely modify it to match the style of their existing code. Which is fine, but as a source of documentation aimed at teaching concepts, it should be the priority of the code to be easily understood.

This process of having to mentally translate the code wastes mental energy on tasks unrelated to the goal of understanding the concepts.

Note: My point isn't "fix this one example", it's "most of your examples are like this and it becomes exhausting".

I'm sure the code wasn't written by someone literally saying "look at me, I'm clever, I can do this". I'm sure there's some style guide or linter that is enforcing consistency. But my point is that following it religiously leads to hard to read code, or that it's settings are too strict in ES6 enforcement and don't lend themselves to easily parsable example snippets.

yyx990803 commented 6 years ago

You are more than welcome to submit PRs to fix this for others instead of complaining and making assumptions about why people write code this way.

parzh commented 6 years ago

Let's reverse it.

getters: {
    getTodoById: (state) => {
        function findTodoWithMatchingId (id) {
            const todos = state.todos
            const matchingTodo = todos.find(todo => todo.id === id)
            return matchingTodo
        }
        return findTodoWithMatchingId
    }
}

I find this a bit verbose, because every value has its own identifier. First of all, not all values are important enough, to have a distinct identifier. There might and will be transitory values too. Second, every variable is used exactly once, so why bother? Just return the value right away:

getters: {
    getTodoById: (state) => (id) => state.todos.find(todo => todo.id === id),
}

Well, there is still a couple of problems with that snippet. First, that's a bit tight, it is hard to distinguish at a glance, where does the body of the function start. Second, there are too much chars at a single line, we need to split it. Finally, we do not need the whole state object, but rather state.todos. We'll use destructuring assignment for the last one:

getters: {
    getTodoById: ({ todos }) => id => {
        return todos.find(todo => todo.id === id);
    },
}

We've came to what we started with (without unnecessary parentheses though). Further reducing is possible (e.g. we don't need the whole todo object, but rather todo.id), but will probably lead to ambiguity and misunderstanding.

If you're concerned about double anonymous function ({ todos }) => id =>, then fear not. This is called currying, it is unambiguous and is used a lot.

emileber commented 6 years ago

Not using an arrow function for the getter would make it clearer that it's a function returning a function.

getters: {
    getTodoById({ todos }) {
        return id => todos.find(todo => todo.id === id);
    },
}
TheJaredWilcurt commented 6 years ago

@parzh

I'm familiar with currying. Your user's aren't trying to learn about currying, they are trying to learn about Vuex.

not all values are important enough to have a distinct identifier.

You make Uncle Bob sad. Those variable names clue the reader in to what the code is doing (getTodoById, findTodoWithMatchingId, todos, matchingTodo). It makes it easy to read and that should be the goal of all of the code in a documentation website. The number 1 goal should be making the code as easy to read and understand as possible. And it is very obvious that is not the case on the current Vuex website.

The documentation is so painful.

"Sometimes we may need to compute derived state based on store state."

Someone not familiar with the topic would have difficulty understanding that, and it's literally the introduction to the concept.

If this were any other project I would be a lot more forgiving, but the Vue.js documentation is literally the best documentation on the internet. So it's really frustrating that Vuex's docs are so unpolished with hard to parse examples that people are defending.


@yyx990803

I point out a problem and bring up a general approach for solving it, saying specifically that this approach leaves a lot up for interpretation. Instead of being given any guidance or instruction around how a solution should be implemented I'm just told "stop complaining and make a PR", which to me sounds like "I don't care, go make a PR so I can reject it".

I'm looking for a discussion around how to improve this, and instead I can't even get people to acknowledge there is a problem.

yyx990803 commented 6 years ago

You are making assumptions again. If you made a quality PR there's no way we'd reject it. But as I pointed out, your idea that we wrote it that way just to look smart is completely off-point and unnecessary. Complaining maintainers being defensive when you were the one being offensive is even worse. It seems to me you care more about your opinions than actually helping.

parzh commented 6 years ago

@TheJaredWilcurt: I don't know who's Uncle Bob unfortunately.

Your user's [...] trying to learn about Vuex.

... assuming that they know JavaScript at the first place. And if they do, it should be obvious enough that they can read the JavaScript code. Of course, it doesn't exclude the need to write readable code, but the code (first) => (second) => third is considered pretty much readable by majority of modern JavaScript developers, - by far more readable then this for example:

function takeFirst(first) {
    return function takeSecond(second) {
        return third;
    };
}

To be honest, the original snippet, provided in the very first comment, is even more readable than my own derivation of it (see my previous comment).


@TheJaredWilcurt @emileber let's pause for a moment and think about the original problem: finding to-do item by its identifier. Before you write any code, you first plan the logic, regardless of the language and framework. Here is what comes into my mind at first:

This would be the best, by current implementation suggests a list of all items instead of a map. So we have to tweak the logic a bit:

Given the syntax of Vuex getters, the original snippet comes as close to my logic as possible. In contrary, the last snippet in the first comment suggests the following logic:

... which is verbose (see my previous comment) and IMO just not how people's mind works, - therefore, not (much) readable.


I would agree with @yyx990803 with the addition that you are probably a bit prone to overreacting over the little things. If you see that something is wrong, try to evaluate your need of this thing. If you really need this to be fixed, try to fix it by yourself (if it is possible). Finally, if you cannot do it alone, ask for help. That flow makes a lot more sense to me than saying out loud that everybody around is wrong.