-
-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor(achievement): Rewritten Achievements helpers with async/await syntax #587
Conversation
Signed-off-by: Akash Gupta <akashgp9@gmail.com>
Signed-off-by: Akash Gupta <akashgp9@gmail.com>
Signed-off-by: Akash Gupta <akashgp9@gmail.com>
Signed-off-by: Akash Gupta <akashgp9@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most fo the comments below are repeated, as the code itself is repetitive.
Overall that's a good conversion !
src/server/helpers/achievement.js
Outdated
const unlock = await new UnlockType(awardAttribs).save(null, {method: 'insert'}); | ||
return unlock.toJSON(); | ||
} | ||
return new Promise((resolve) => resolve('Already unlocked')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntax is redundant, along with the same in the catch block.
Th async
keyword already defines that the function returns a Promise, so instead of return new Promise([…] resolve(myReturnValue)
you can simply return myReturnValue
.
On the other side in the catch block you can simply throw err
.
I'm not exactly sure why this code either returns an unlock object or the string 'Already unlocked', that seems a bit odd to me, but it's clear from the docs above that it's intended, so not an issue for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other side in the catch block you can simply throw err
I have removed the try catch statement as the catch clause was only rethrowing the original error, which is redundant.
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
Co-authored-by: Monkey Do <MonkeyDo@users.noreply.github.com>
@MonkeyDo thank you so much for the detailed review 💯 . I have refactored the code as suggested, do let me know if changes are needed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a bunch, the code is a lot more readable now ! :)
Problem
This PR Fixes: BB-598
Solution
A lot of the code uses promises in the
promise.then(…)
format, which makes the code hard to read and maintain. Rewritten it using the async/await syntaxAreas of Impact
file src/server/helpers/achievement.js