fix: moved lastCategory assignment inside the if condition for efficiency and logic

[matys1]

In thinking-in-react moved lastCategory = product.category inside the if (product.category !== lastCategory) condition which is more efficient and logical. This ensures that lastCategory is only updated when a new category is encountered, which is precisely the condition under which you want to update it.

[facebook-github-bot]

Hi @matys1!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[github-actions]

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[matys1]

@sophiebits @lunaleaps @smikitky @rickhanlonii @mattcarrollcode @A7med3bdulBaset

could someone please give a quick glance to this PR? I think this is a good and valid change.

[smikitky]

I'm not an author of the original docs (I'm just a translator), but since you mentioned me...

• The original code is not logically incorrect. lastCategory must hold the category of the last item, as the name suggests, and the current code is clearer in its intention.
• This code is not the kind that runs millions of times per second, so this type of performance optimization is unnecessary. Compared to the amount of code that runs around this (JS engine, React, browser...), I believe the difference it makes is not measurable.

[sophiebits]

Thanks @smikitky. I tend to agree: both versions of the code will run with similar performance so it is a question of which one is clearer. I can see arguments either way but I don’t think it is decisive so I will leave it as is.

[matys1]

@smikitky @sophiebits

The original code is not logically incorrect. lastCategory must hold the category of the last item, as the name suggests, and the current code is clearer in its intention.

I strongly disagree and you didn't provide a reason as to why you think the current version is clearer. The readability of code is the main reason why I submitted this PR.

The current version is objectively less efficient than my proposal because you update lastCategory unconditionally and unnecessarily on each iteration. But let's put performance aside because you're right for this trivial example performance doesn't matter. What matters is readability, especially because the "Thinking in React" paper is read by beginners.

I provided GPT-4 with both versions of the ProductTable component with a simple prompt: "Describe to me which version of the component is more readable and provide your reasoning". Version 1 is the original and Version 2 is my proposal. You can confirm this yourself but here's what it said:

"Version 2 is more logical for a reader because it groups the action of appending a category row and updating the lastCategory state together. This proximity makes it clearer that these two actions are related, which is beneficial for beginners trying to understand the flow of the program. For beginners, the logical grouping and proximity of related code make a significant difference in understanding the code's intent. It teaches them good practices about updating related state as close to each other as possible."

A the end its up to you guys but I just don't understand why would we reject a PR that makes code both more efficient and easier to read and understand, especially for beginners.

[sophiebits]

I don't agree that it is definitely clearer.

You could think of it as either
a. add a category header when the next row has a different category than the last row, or
b. add a category header when the next row has a different category than the last category header

In a., lastCategory represents the category value for the last row we saw (essentially previousItem.category).
In b., lastCategory represents the category value for the last category header we added (essentially previousHeader.category).

These are of course equivalent in this case but I think you could have either mental model. The current code does a., while yours does b.

[smikitky]

Sophiebits explained this better than I, but this is a matter of whether the intended meaning of lastCategory is lastProductCategory or lastHeaderCategory. The original code obviously assumes it's lastProductCategory. At the end of the loop, the original code makes its intent crystal clear by saying lastCategory = product.category. While your version is equally correct, there is no strong reason to "redefine" the meaning of this variable.

[matys1]

@sophiebits @smikitky

My thought process was that lastCategory keeps track of the last category added to the UI. If the current products category doesn't match the lastCategory then push to rows (add to UI) AND immediately update the lastCategory such that it reflects the latest added category. To me it didn't make sense to update lastCategory in a different scope after a bunch of unrelated code even when the last category stayed the same. The change makes the code's logic and flow easier to understand as the related actions are grouped together reducing the cognitive load of having to jump between two different scopes trying to figure out if something else relies on lastCategory being out of sync with the UI. This way lastCategory reflects the actual state of the UI (the last category rendered) rather than just the state of the last iterated product. This is important because the rendering logic (the <ProductCategoryRow> component being pushed to rows) is fundamentally about representing the UI state, not just iterating over data.

So while I think I understand the point about the alternative semantic interpretation of the lastCategory variable, for the reasons listed above I think the change was a genuine improvement.

Apologies for tagging a bunch of potentially unrelated folks. I not sure who wrote the original example and how to officially request reviewers.

[smikitky]

My thought process was that lastCategory keeps track of the last category added to the UI.

I know, and I'm saying that's just one of the two equally valid thought processes. After all, as sophiebits said, "I can see arguments either way but I don’t think it is decisive".

If you cannot imagine what the other argument would look like, here it is:

• Unconditionally remembering the previous item, like in the original code, is a common iteration pattern. Some programming language offer a method for this (e.g., Python's pairwise). While it is generally good to put related code together, simply keeping track of the previous item is not part of the condition; it's part of the loop logic itself.
• When you conditionally assign a value in an if-clause, you are introducing a piece of mutable "state" that changes independently of the loop logic itself, increasing the cognitive load. Since you can achieve the same goal just by checking the previous value's category, this extra state is simply unnecessary. On the other hand, the original code is essentially the same as writing const lastCategory = products[i-1]?.category at the beginning of the loop, so it's not a stateful value. This means you can understand what the code does without running the loop several times in your brain.
• Additionally, it is simply considered bad to unnecessarily increase the amount of indented code in a conditional branch, which will increase the cognitive load.

I don't want you to misunderstand this; I have no intention of deciding which thought process is superior. It is sufficient to show that this is a debatable issue with no definitive answer.

[smikitky]

An experienced developer might write this without any "imperative" stuff like if, push or let:

const rows = products.flatMap((p, i) => [
  p.category !== products[i - 1]?.category && (
    <ProductCategoryRow category={p.category} key={p.category} />
  ),
  <ProductRow product={p} key={p.name} />,
]);

As you can see, there is no need to introduce a variable to remember the category of the last heading in the first place. The original code doesn't use flatMap because not everyone knows it, but conceptually it's based on the same idea, and that's why it's preferred by some.

[matys1]

Thanks for your explanation. I think I was reading the paper with the mindset or expectation that things will be closely coupled and if things were not closely coupled there would be a very good reason for it, like some other piece of code relies on the current state of lastCategory. But since this was not the case it raised all sorts of questions like why is it temporarily out of sync with the UI, looks harder to debug, maintain, scale etc. So if I were to move the lastCategory assignment within the already existing if condition then it would make it very explicit: this variable is just there to track the last category, it's entirely in sync with the UI, there's no other piece of code that relies on the state of lastCategory, the logic ends with the if block and is entirely self-encapsulated.

However, with saying all that I understand your argument. Indeed unconditionally tracking the last category of each item in a loop is a more data-centric approach (as opposed to the UI-centric approach I suppose) and in some ways could also be seen as a "simple" solution to this problem. And indeed you can abstract the logic even more concisely (and perhaps more elegantly) using the logical && short-circuiting but since it would also be a lot less explicit and the paper is aimed towards beginners this would definitely be a tradeoff in terms of readability and understandability from a readers perspective.

While I still prefer my proposal over the original I've come to appreciate that there are devs out there that have very different perspectives and approaches to the same problem. I've spent way more time on this PR than I anticipated (with a lot stronger pushback that I anticipated :)) so I rest my case.