The-CodingSloth / haha-funny-leetcode-extension

MIT License
392 stars 75 forks source link

Refactored storage #85

Closed Mgel closed 9 months ago

Mgel commented 9 months ago

Refactored storage into its own file and added some helper methods Refactored the updateStorage method to make use of promises Also did some minor cleanup

This is a mostly a suggestion for readability improvements. But it will also assist in making the addition of new storage features easier

troygidney commented 9 months ago

This is awesome, I am a huge fan of code spliting, easier to work on as a team and easier for debugging.

Mgel commented 9 months ago

This is awesome, I am a huge fan of code spliting, easier to work on as a team and easier for debugging.

Appreciate it! However my js/ts knowledge is very limited, especially when it comes to the proper usage of async/await and promises.

It did however seem like the implementation in updateStorage with Promise.Then.Catch was bad, so I changed it to use try catch block instead.

In addition it seems like using Promise.all can take advantage of the fact that updating the storage of different keys are independent and provide minor perfomance boost (as well as better errors compared to multiple awaits)

The updateStreak function is however a bit weird now, using a combination. The last part with setting the streak could be written as:

return Promise.all([
    Number(storage.get("bestStreak")) ?? 0,
    (Number(storage.get("currentStreak")) ?? 0) + 1,
    storage.set("lastCompleted", now.toDateString())
]).then(async ([bestStreak, newStreak, _]) => {
    await storage.set("currentStreak", newStreak)
    if (newStreak > bestStreak)
        await storage.set("bestStreak", newStreak)
})

But I really do not know if this is a correct/good approach

The-CodingSloth commented 9 months ago

Sorry for such a delayed response haha, but this looks good. I think this is some good abstraction to make it easier for people if they want to contribute. In my experience when it comes to promise.all (not much experience) is it's a good consideration if the promises/async operations you're doing are dependent of each other so situations where you need to merge async operations. Not too sure though since promises are weird, but I think when it comes to the streak it would make sense.