-
Notifications
You must be signed in to change notification settings - Fork 64
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
Basic auction optimization of code and null checks #226
base: develop
Are you sure you want to change the base?
Basic auction optimization of code and null checks #226
Conversation
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.
Great changes done 👍🏾 🥳
Recommended some minor tweaks which will improve code readability!
setAuctionsData(json.auctions); | ||
if (!response) return null; | ||
setAuctionsData( | ||
JSON.parse(response && response.auctions ? response.auctions : 0) || 0 |
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.
You could use the new syntax here!
JSON.parse(response.auctions ?? 0)
It mostly won't need the fallback || 0
as well 😄
} | ||
}; | ||
|
||
useEffect(() => { | ||
(async () => { |
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.
Please create named functions!
Reading and understanding stack traces would be far easier and better 😄
async function funcname = () => {}
funcname()
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.
It's an IIFE ,please recheck the implementation,thanks
await fetchAndSetAuctions(); | ||
await getUserWallet(); | ||
await fetchSelfDetails() | ||
.then((res) => { |
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.
Please don't mix the .then().catch()
syntax with async-await if not needed.
You can wrap the block in a try-catch as any of the above requests could also fail 😓
title={bidder} | ||
> | ||
const auctionHandler = | ||
auctionsData && auctionsData.length |
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.
Use the auctionsData?.length
syntax, it's simpler 😄
> | ||
const auctionHandler = | ||
auctionsData && auctionsData.length | ||
? auctionsData.map(({ id, seller, quantity, highest_bid, bidders }) => { |
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.
Please create a sub-component which you can then use in the ternary operator...
Easier to read and understand the fallback, which in this case we have to scroll a lot to find 😅
</div> | ||
); | ||
}) | ||
: null; |
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.
Please use the inline && operator to avoid the null condition 😄
Sure will do thanks. |
@vishwasrvalke Would you like to fix this PR so that we can merge it? |
ADDED NULL CHECKS TO PREVENT PAGE FROM CRASHING ON API FAIL ,
Optimisation of code and leakages with async calls and hooks