Tyler Hevia Code Review#6
Open
Tyler Hevia (tylerjhevia) wants to merge 5 commits intowillowtreeapps:masterfrom
Open
Tyler Hevia Code Review#6Tyler Hevia (tylerjhevia) wants to merge 5 commits intowillowtreeapps:masterfrom
Tyler Hevia (tylerjhevia) wants to merge 5 commits intowillowtreeapps:masterfrom
Conversation
- Refactor API call function -- instead of instantiating a new Promise, just return fetch call, which will return a promise by default - Change error handling from if statement to catch method
-Change getFirstName from arrow function to traditional function for the sake of consistency. There is no significant performance benefit to be gained from using arrow functions (to my knowledge), and since getFirstname and getLastName are performing similar tasks, I want to structure them consistently to avoid confusion
- Replace slice with Array.from - List.slice(1) wasn't making a copy of the whole list, because it was starting at the second value in the array. Array.from makes a complete copy
- Use ES6 syntax to shuffle values
- sortByLast name was being called with firstName as the argument of sortObjListByProp. Change firstName to lastName and remove the reverse() method
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here is an overview of my proposed changes:
- Removed the Promise instantiation in getPersonList function, instead returned fetch directly,
which already returns a promise
- Used a catch instead of an if statement to handle unsuccessful fetch
functionality, but I wanted to write getFirstName and getLastName in the same style. They do
serve similar purposes and I think it makes sense to structure them the same way. Is it acceptable
to use arrow functions and normal functions alongside one another, or should I try pick one syntax
for a project and stick to it?
resultvariable is a complete copy of the original list.The original copy method with slice(1) excluded the first element in the list array, so I used
Array.from() instead.