javascript-javascript1-week4/jianxin #116
Conversation
|
|
||
|
|
||
| function userNameLogin(command) { | ||
| let loginName = command.split(' ')[4].toLowerCase().trim(); |
There was a problem hiding this comment.
It is working good solution,
here you could just use "const" instead of "let" if you are not intended to reassign this variable - it's like common convention "to use as little let as possible - so variable won't get reassign accidently later, which improves code safety and clarity"
Also "toLowerCase()" is not neccessary here as you use this method in "getReply" function for "command" which passed to this function
There was a problem hiding this comment.
Same with "let" and "const" throughout the whole code
| }else{ | ||
| userName = loginName; | ||
| return `Nice to meet you ${userName}` | ||
| } |
There was a problem hiding this comment.
Here function do it's job well - you could just improve code readability avoiding
use of "else if" "else" - conditionals, because you have "return" statements inside and it's will finish function execution and won't let code go into next if statement
Similar as you performed in "getReply" function - but there also "else" isn't necessary but still ok to use
There was a problem hiding this comment.
The same with "else if", and "else" troughout the whole code
|
|
||
|
|
||
| function replyMyName() { | ||
| if(userName != ''){ |
There was a problem hiding this comment.
I would recommend always to use strict equality and inequality operators, unless you have a specific reason for not doing that
'!==' insted of '!='
Try 0 != '' or false!='', what are you going to get here?
| return `Nice to meet you again, ${userName}` | ||
| }else{ | ||
| userName = loginName; | ||
| return `Nice to meet you ${userName}` |
There was a problem hiding this comment.
Also when you get a new user would be nice to reset todo list to empty array, in order not to have a saved todos for another user.
We don't want to go deep - to save users and appropriate todos for each user and when familiar user "returns" have it's todos ready, but just to reset all todos would be "ok" logic
| function numberCalculator(command) { | ||
| let calculatedString = command.slice(8).trim(); | ||
| let result = eval(calculatedString); | ||
| return result; |
There was a problem hiding this comment.
"eval" is easy and working approach but it's generally not recommended to use due to potential code injection attacks as eval() executes any code passed to it
| let userName = ''; | ||
| const toDoList = []; | ||
|
|
||
|
|
There was a problem hiding this comment.
Really good decision to extract logic of each conditional into different smaller functions - so code stayed easy to read and furthermore to maintain and debug
Good job!
| let todayDateYear = todayDate.getFullYear(); | ||
| const allMonths = ['January', 'Feburary', 'March', 'April', 'May', 'June', 'July', 'August', 'September', 'October', 'November', 'December'] | ||
| let todayDateMonth = allMonths[todayDate.getMonth()] | ||
| let todayDateDay= todayDate.getDate(); |
There was a problem hiding this comment.
Good solution here and if you don't want to create an array with all months
you could use inbuilt method - todayDate.toLocaleString('default', { month: 'long' });
in order to get a full name of the month
Hand in js1 week4 homework