menu

React

A community of developers, designers and others who love React.js. ⚛️

Channels
Team

[Code Review] I just completed my minimalist music app

January 18, 2020 at 5:07pm

[Code Review] I just completed my minimalist music app

January 18, 2020 at 5:07pm
Hi guys! I just completed my minimalist music app
If somebody could have a quick look at the repo and tell me how bad/good is my code it would be greatly appreciated. Thank you :) !

January 18, 2020 at 6:32pm
Hi I saw your code, more on the hooks folder. Of course, there always space to improve, but in general the code seems fairly good. I am going to list two things that in my opinion, you can improve.
e.g
if (refreshToken === true) { // false mean firebase haven't release token, so throw unauthorized
should be
if (refreshToken)
Why? because if (refreshToken) expects a boolean expression and in this case b because refreshToken can only be true or false if(refreshToken === true) it's redundant.
In the following code block, you have a lot of ident levels. Personally, I prefer to use inverted logic: Also, use always { after an if, you avoid a lot of errors maintaining the application.
const loadMore = async (params) => {
if (hasMore) {
try {
const res = await myApiAxiosInstance.get(endpoint, {
params: {
limit,
lastVisible: offset,
},
});
if (res.data.lastVisible == 'end') setHasMore(false);
addTrendingTracks(res.data.collection);
setOffset(res.data.lastVisible);
} catch (error) {
message.error(errorSelector(error));
}
}
};
would be
const loadMore = async (params) => {
if (!hasMore) {
return;
}
try {
const res = await myApiAxiosInstance.get(endpoint, {
params: {
limit,
lastVisible: offset,
},
});
if (res.data.lastVisible == 'end') {
setHasMore(false);
}
addTrendingTracks(res.data.collection);
setOffset(res.data.lastVisible);
} catch (error) {
message.error(errorSelector(error));
}
};
Edited
like-fill
1
  • reply
  • like

January 19, 2020 at 3:36am
Hi I saw your code, more on the hooks folder. Of course, there always space to improve, but in general the code seems fairly good. I am going to list two things that in my opinion, you can improve.
e.g
if (refreshToken === true) { // false mean firebase haven't release token, so throw unauthorized
should be
if (refreshToken)
Why? because if (refreshToken) expects a boolean expression and in this case b because refreshToken can only be true or false if(refreshToken === true) it's redundant.
In the following code block, you have a lot of ident levels. Personally, I prefer to use inverted logic: Also, use always { after an if, you avoid a lot of errors maintaining the application.
const loadMore = async (params) => {
if (hasMore) {
try {
const res = await myApiAxiosInstance.get(endpoint, {
params: {
limit,
lastVisible: offset,
},
});
if (res.data.lastVisible == 'end') setHasMore(false);
addTrendingTracks(res.data.collection);
setOffset(res.data.lastVisible);
} catch (error) {
message.error(errorSelector(error));
}
}
};
would be
const loadMore = async (params) => {
if (!hasMore) {
return;
}
try {
const res = await myApiAxiosInstance.get(endpoint, {
params: {
limit,
lastVisible: offset,
},
});
if (res.data.lastVisible == 'end') {
setHasMore(false);
}
addTrendingTracks(res.data.collection);
setOffset(res.data.lastVisible);
} catch (error) {
message.error(errorSelector(error));
}
};
Thank you for your detail advice. I just refactored my code base on your recommendation. I believe there are a lot of room for improvement in my code. How about the way I name files and organize the code ?
  • reply
  • like

January 19, 2020 at 12:16pm
Thank you for your detail advice. I just refactored my code base on your recommendation. I believe there are a lot of room for improvement in my code. How about the way I name files and organize the code ?
It seems good. The only think I would change it's the hook naming. You should omit the use work from the filenames. It's implicit. Happy coding.
like-fill
1
  • reply
  • like
It seems good. The only think I would change it's the hook naming. You should omit the use work from the filenames. It's implicit. Happy coding.
Hello, For example: useFetchFavoriteTrackIds.js would be fetchFavoriteTrackIds.js ? How about hook function name ? If I change to fetchFavoriteTrackIds, it will violate Hook rule
  • reply
  • like
The hook function should mantain use of course :). There is a reason not to include use in filename. e.g geometry.js then inside this file:
export const useSquare ...
export const useCircle ...
like-fill
1
  • reply
  • like