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

Server review #7

Open
wants to merge 21 commits into
base: feedback
Choose a base branch
from
Open

Server review #7

wants to merge 21 commits into from

Conversation

gretaivan
Copy link
Owner

No description provided.

Copy link
Collaborator

@yourlocaldeveloper yourlocaldeveloper left a comment

Choose a reason for hiding this comment

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

Great work again guys, me and Ravil were impressed that you used a public API. Super refreshing as we created one so was nice to see someone take a different approach. Works well, we ran the server side with no issues. Although, probably not your fault, but results took a few seconds to return and show on the page. Other than that great job and was a pleasure too look through and try out 👍

@@ -1 +1,33 @@
# Gclone Server
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great use of README, as mentioned in other repo

@@ -0,0 +1,15 @@
function formatData(data, length){
data.length = length
return data.filter(item => item !== 'undefined')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good use of validation!


function randomData(data){
length = data.length
randomIndex = Math.floor(Math.random() * length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good use of Math functions

@@ -0,0 +1,6 @@
const server = require("./server");
const port = process.env.PORT || 3000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good use of OR rule

axios.get(url)
.then(function (response) {
// formatData(response);
res.status(200).send({body: helpers.formatData(response.data.organic_results, 10)})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line could be placed into two lines to make it more readable:
const formated = helpers.formatData(response.data.organic_results, 10) res.status(200).send({body: formated)

Comment on lines +38 to +43
axios.get(url)
.then(function (response) {
// formatData(response);
res.status(200).send({body: helpers.randomData(helpers.formatData(response.data.organic_results, 10))})
})
.catch(console.warn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could create a separate function for fetching response passing through as arguments, the url and the function you want to use to format the data. This would reduce duplicate code!

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.

3 participants