The-CodingSloth / haha-funny-leetcode-extension

MIT License
392 stars 75 forks source link

Made it possible to toggle whether to include premium problems #77

Closed Mgel closed 9 months ago

Mgel commented 9 months ago

Premium problems can now be toggled on/off Introduced checkbox component Refactored logic a bit to only do a single filterpass and removed now unnecessary while-loop

Mgel commented 9 months ago

Fun project! This is actually my first time ever contributing to any github project as well as my first time working with typescript. I thought it would be interesting to try and do something small - the TODO related to this was removed after I first saw it.

I actually ended going a bit overboard in my first attempt, refactoring a lot of the generate problem code. But since it probably doesn't align with the contribution guidelines, I decided to keep it to a minimum. I would however like to share it with you either way, just to get your thoughts (if you're interested in having a look). https://github.com/Mgel/haha-funny-leetcode-extension/tree/feature/refactor-generate-problem

There are problems with getting data from some of the sources since I made this before they were added, otherwise it should work like before

The-CodingSloth commented 9 months ago

sweeeet this is awesome, glad you came to help haha. I looked at your first attempt and I'm liking the way you separated all logic into their own functions and its own file, but I'm curious on why you're returning promise.resolve on all of them? From my understanding it looks like by the time you finish everything in those functions, they shouldn't be promises anymore since they've been resolved. oh and I'll merge this once I make sure this bug is fixed.

Mgel commented 9 months ago

I can't with confidence explain why, since I didn't dig deep down into async function and promises. I'm using Visual Code to edit, and it recommended/suggested that async function always should return promises, so thats what I did 😆

I just used Promise.resolve to explicitly return a promise. I don't know if you could just do return { url: "url", name: "name" }, but i would assume that if you could it would implicitly be a resolve either ways, since the return type of the function is set as such.

The-CodingSloth commented 9 months ago

lmao fair enough, I think you could just do a normal since when you use await you're telling the code to stop until that line gets resolved, but I also don't fully understand promises.

Mgel commented 9 months ago

Alright, I had a look at promises and async/await again trying to understand it a bit better. I agree with you that using Promise.resolve does not make sense in the current implementation - it is a messed up mix of both with no real value.

I tried a couple of things and you can go kinda crazy with this, and since I'm not familiar with best practices when it comes to typescript (I'm a C# developer) I can only say what I prefer, but not whether it is a good approach 😆

I see two possibilities:

  1. using async/await in most cases, and having outer function/caller use the promise then/catch design
    
    async function getProblem() {
    const problems = await getProblemsFromApi();
    return { problems[0].url, problems[0].title };
    }

async function writeProblemToConsole() { getProblem().then((problem) => { console.log(problem.name + ": " + problem.url) }).catch((error) => { console.error("Something went wrong ", error) }); }

Which in the case where no problems were found could give you an error message along theses lines:
**Something went wrong TypeError: Cannot read properties of undefined**

2. Chaining the promise then/catch design
```typescript
async function getProblem() {
  return getProblemsFromApi().then((problems) => {
    return { problems[0].url, problems[0].title }; // is wrapped in Promise behind the scenes - would equal return Promise.resolve(obj)
  }).catch((error) => {
    return Promise.reject("Failed to get problem: " + error)
  });

}

async function writeProblemToConsole() {
  getProblem().then((problem) => {
    console.log(problem.name + ": " + problem.url)
  }).catch((error) => {
    console.error("Something went wrong ", error)
  });
}

Which instead would give you an error message along theses lines: Something went wrong Failed to get problem: TypeError: Cannot read properties of undefined

I myself prefer case 1 because of the code readability. Also sorry for the wall of text, a PR is probably not the best place to write this kind of stuff. But it is at least as much for my own purpose as I understand it a bit better if i write it out and I guess you're just the (un)lucky receiver. I'll focus on some other more pressing issues now.

The-CodingSloth commented 9 months ago

haha no worries love to see the thought process and learn some interesting approaches. I appreciate you even came to contribute when this isn't your area.