Ex4 #39
Ex4 #39
Conversation
There was a problem hiding this comment.
Great work. the server performs well and even though I tried to find bugs, didn't find any.
Please mind my comments, and lets have a short GIT session.
Try to reduce the functions' length, shouldn't cross 20 lines, otherwise, you need to break it into small functions with dedicated functionality.
| "license": "ISC", | ||
| "dependencies": { | ||
| "cors": "^2.8.5", | ||
| "express": "^4.18.1" |
| @@ -0,0 +1,16 @@ | |||
| import express from "express"; | |||
There was a problem hiding this comment.
If you rename the file api.js I would assume that the route will be localhost/api/items, please rename the file or the routing
| @@ -0,0 +1,7 @@ | |||
| export function logger(req, res, next) { | |||
There was a problem hiding this comment.
cool. worth changing the name to requestLogger as logger is more generic name and the functionality is coupled to incoming requests
| @@ -0,0 +1,11 @@ | |||
| export default function errorHandler(err, req, res, next) { | |||
| @@ -0,0 +1,5 @@ | |||
| export function validation(item) { | |||
| const elementsArr = item.split(/\s*,\s*/); | |||
There was a problem hiding this comment.
elementsArr is not a meaning full name, would rename it
| if (!isPokemon) { | ||
| //check pokemon by name | ||
| const isPokemon = await pokemonClinet.checkByPokemonName(inputArr[0]); | ||
| if (isPokemon) return this.checkByPokemonName(isPokemon); |
There was a problem hiding this comment.
checkByPokemonName is an async function, use await
| const task = this.pokemonInit(false, inputArr[0]); | ||
| this.itemsArray.push(task); | ||
| this.newItems.push(task); | ||
| await fs.writeFile(this.jsonFile, JSON.stringify(this.itemsArray)); | ||
| return this.newItems; |
There was a problem hiding this comment.
this else scope will never run as you already checked !isPokemon in line 92
| } | ||
| } | ||
|
|
||
| pokemonInit(isPokemon, item, imageUrl = "", pokemonId = "") { |
| try { | ||
| this.readFile(); | ||
| const idx = this.itemsArray.findIndex((item) => item.itemId === itemId); | ||
| if (idx === -1) throw "err"; |
There was a problem hiding this comment.
throw meaningful error, and use throw new Error('my error')
| throw `There is no task with id: ${itemId} `; | ||
| } | ||
| } | ||
| itemToAdd(arr) { |
There was a problem hiding this comment.
rename the method's name to its purpose filterExistingPokemons
There was a problem hiding this comment.
you can use this.exsitsPokemon that already checking if the pokemon exists, you may need to pass an array you would like to perform the checking on
@liorshamian
@monadav