neetcode-gh / leetcode

Leetcode solutions
MIT License
5.63k stars 2.32k forks source link

Revert JavaScript Solutions #2295

Open telunc opened 1 year ago

telunc commented 1 year ago

I was looking at the JavaScript solution for Leetcode 124 (https://github.com/neetcode-gh/leetcode/blob/main/javascript/0124-binary-tree-maximum-path-sum.js). I find it much harder to read and understand compared to Python solutions.

After further investigation, it seems like the refactor PR by @aakhtar3 made the solutions worse. Please revert https://github.com/neetcode-gh/leetcode/pull/1150. And all those JavaScript refactor PRs https://github.com/neetcode-gh/leetcode/pulls?q=aakhtar3.

charlie-yao commented 1 year ago

Agreed. I know code style is subjective, but there were some things that didn't really make sense to me. Maybe I'd agree with those style decisions if I knew the intention behind them, and maybe it speaks about my lack of ability more than anything else, but I find it hard to decipher the intention through the code alone.

For example, there are many times where code is abstracted into separate functions, and I generally agree with that technique. But the function is written in a way that's confusing to me, and it forces me to spend time trying to parse the function instead of the actual solution to the LeetCode problem.

For example, see the second JS solution for longest increasing subsequence.

There's a function getMax() that provides a default value for the parameter index. But, it seems like index shouldn't even be a parameter because having it as a parameter implies that there are situations where outside code can/should supply values for index. But, it's apparent that index is a variable that should fully controlled by getMax(). It seems like the intention was to use JS' ability to provide default parameters to initialize a variable, but semantically speaking, I think it's the wrong application of that facility.

Rather than think about the LeetCode problem, I have to spend time thinking about tangential details like, "are there situations where it makes sense to call the function recursively, thus justifying having it as a parameter with a default value? Well, there's only ever one non-recursive call to the function, but maybe..."

Or, there are times where logic for a conditional is moved to a separate variable. I think that technique is often a good one, but there are cases where I think it just makes the code harder to understand. Oftentimes, I find that the variable names aren't actually helpful or descriptive (IMO). For example, if(canX). What is it about the underlying logic that allows X to execute? The variable name doesn't actually explain or provide the context for me to understand why something is happening.

To me, these refactors are often just extra layers of unnecessary abstraction that my brain has to keep track of, and it's at a point where I think I'd rather just directly read the logic because I think that would actually require less effort. Maybe these abstractions would make more sense to me if I already fully understood the solution to the LeetCode problem, but I'm trying to read the code so I can understand the solution.

Again, I know code style is subjective, but it's been a repeated source of frustration for nearly all of the JS NeetCode solutions that I've been lookng up.

aakhtar3 commented 1 year ago

The repo has changed quite a bit and might cause other issues by doing a revert.

I am happy to revert the solutions if you submit pull requests that address any conflicts.