Javascript javascript1 week4/jalil #110
Conversation
|
Great work overall. Most of the suggestions I have are just that - they're things you could do, rather than things you should. There is one potential bug in there that it would be great for you to look into, though! |
| const toDoList = []; | ||
| function getReply(command) { | ||
| const assitantsave = { | ||
| addName(command) { |
There was a problem hiding this comment.
Great use of additional functions!
As a small optimization to make the code slightly more readable at-a-glance, you could consider moving them out to live alongside the getReply() function. Currently, it's a little hard to tell immediately what that function does vs. what it delegates to other functions, since we have to scroll past these ones first.
One convention you'll often see is to put the main/important/public functions higher up in a file or class. Then any less important helper functions go lower down.
There was a problem hiding this comment.
Thank you for the suggestion! I'll reorganize the functions.
| addName(command) { | ||
| const name = command.split(" ").slice(-1)[0]; | ||
| console.log("Nice to meet you " + name); | ||
| clientName = name; |
There was a problem hiding this comment.
Consider assigning directly to clientName to remove the intermediate name variable.
| clientName = name; | ||
| }, | ||
| viewMyName(command) { | ||
| if (clientName === "" || clientName === " ") { |
There was a problem hiding this comment.
Is it actually possible for clientName === " " to happen? The only time we assign to the clientName variable it's using const name = command.split(" ").slice(-1)[0];, and the .split(" ") part should mean we don't end up with space characters in the assigned value.
That said, it's good practice to code "defensively", which means trying to anticipate errors or other ways in which the code might misbehave and coding in a way to minimize the impact of those.
Ultimately if you're 100% certain that some condition should never happen, then you can choose to omit that code and perhaps end up with something slightly simpler. If you're not certain, you can leave it in - it's a bit of a balancing act and a judgment call though. Sometimes you won't think something can happen, but it does!
There was a problem hiding this comment.
Also, if it's possible to end up with a single space in the name, is it possible to end up with more? If you want to be even more "defensive", then you could consider trimming whitespace from the string and then checking if there's anything left over.
There was a problem hiding this comment.
now that i think about it, it might never happen. but it is better to think about rare cases. so trim was a good idea.
thanks for highlighting it.
| console.log("Your name is " + clientName); | ||
| }, | ||
| addToMyToDo(command) { | ||
| const match = command.match(/add (.*?) to my todo/); |
| removeFromMyToDo(command) { | ||
| const match = command.match(/remove (.*?) from my todo/); | ||
| let index = toDoList.indexOf(match[1]); | ||
| toDoList.splice(index, 1); |
There was a problem hiding this comment.
Uh oh... We might have a problem!
What happens if the item that the user tries to remove wasn't found in the list? E.g. if the user runs this sequence of commands:
getReply("add going out for dinner to my todo");
getReply("add ordering dessert to my todo");
getReply("remove fishing from my todo");
getReply("what is on my todo");
There was a problem hiding this comment.
Thanks for catching that! I'll add a check to handle cases where the item isn’t in the list and provide feedback to the user.
| console.log(`${day}. of ${month} ${year}`); | ||
| }, | ||
| simpleCalculation(command) { | ||
| const match = command.match(/what is (\d+)\s*([\+\-\*\/])\s*(\d+)/); |
There was a problem hiding this comment.
One other possibility is that we can extract the calculation from the string and run it using a function called eval(...).
However, I like your method a lot better!
It's a bit more restrictive than the eval() approach, because it can only run pre-defined operations (i.e. from your switch statement below). eval() can run any arbitrary code that the user enters.
But that's potentially a big security problem. If someone finds a way to trick users into entering something they didn't intend to, then eval() will just run it regardless. Sometimes this might not even need the user to be tricked into entering something - the input to eval() could come from anywhere, depending on how a site is coded.
So great work here, and definitely think twice before resorting to eval()!
There was a problem hiding this comment.
never thought about it this way. 😄 thanks for explaining in details
| result = num1 / num2; | ||
| break; | ||
| default: | ||
| result = "Invalid operator"; |
There was a problem hiding this comment.
Good stuff having a default condition in case something's gone wrong. If you want to enhance it further, you could consider making the message you show back to the user a bit more detailed, e.g. by suggesting how they could fix the problem.
Perhaps something like:
Invalid operator - Try again with a +, -, *, or / equation
took me 7 hours,
lots of google
lots of trial and error
different methods
it is not perfect.
thanks for reviewing my code