Skip to content
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

weather-api #35

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

weather-api #35

wants to merge 6 commits into from

Conversation

eoneoff
Copy link
Contributor

@eoneoff eoneoff commented Dec 17, 2019

Name your PR following the template: Add <taskname> by <your github username> and remove this paragraph.

I've read ./README.md and ./CODE_QUALITY.md carefully.

The code is checked by yarn run lint:js and linter reported no errors.

The code is submitted in its own sub-directory and in a dedicated feature branch.

Please, review.

Copy link
Contributor

@Siusarna Siusarna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job. Pls, see comments.

if (mode === 'weather') {
const dData = makeWeatherData(data);
dData.city = data.name;
wData.push(dData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to create a "processingData" function and just call it in both situations


function saveRecent (city) {
const recent = readRecent();
if (recent.indexOf(city) === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use recent.includes(city) for determines whether an array includes a certain value


const fs = require('fs');

function saveRecent (city) {
Copy link
Contributor

@Siusarna Siusarna Dec 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove nested in this function, if by using negative check and then return

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I can't see how. I need to remove the last record from recent if there are more then 9 records and save the changes anyway. If I return the saving will be skipped.
I have removed the nested if another way.

Copy link
Contributor

@Siusarna Siusarna Dec 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be writing not-working code, but if it works, then its solution would be better

Suggested change
function saveRecent (city) {
function saveRecent (city) {
const recent = readRecent();
if(!recent.includes(city)) return;
recent.unshift(city);
if (recent.lenght > 9) recent.pop();
writeRecent(recent);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I have done. One thing: not
if(!recent.includes(city)) return
but
if(recent.includes(city)) return
because we add a new city to recent if it is not in the list

Actually I'm not sure, that it is correct to do so, because thus we make code somewhat shorter but, in my opinion less readable. This can make some sense in browser js where shorter code means less traffic, but in server node.js code it makes no sense. IMHO.

But there is another problem. Semistandard finds an error in my code, but the error message encoding is a mess and I can not get where the error is. The semistandard extension for my VS Code shows no linter errors.

}

function readFavorite () {
if (fs.existsSync('favorite.csv')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove nested if by using negative check and then return


function writeFavorite (favorite) {
fs.writeFile('favorite.csv', favorite.map(c => {
return `${c.name};${c.count}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return `${c.name};${c.count}`;
favorite.map(c => `${c.name};${c.count}`)

data.name);
saveCities(location);
}).catch(error => {
if ((error.response.data || {}).message === 'city not found' || (error.response.data || {}).message === 'Nothing to geocode') {
Copy link
Contributor

@Siusarna Siusarna Dec 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if you write const message = (error.response.data || { }).message; and then use message variable in if-else block

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants