Implement calculator#1
Conversation
NedReidSoftwire
left a comment
NedReidSoftwire
left a comment
There was a problem hiding this comment.
Absolutely amazing work! Just have some comments but it should be ready to merge after! 🤩
| console.log("Invalid operator " + operator); | ||
| } | ||
| // console.log('index: ' + index); | ||
| // console.log('current answer: ' + answer); |
There was a problem hiding this comment.
Could you remove these two comments please! 😊
| @@ -0,0 +1,47 @@ | |||
| const operators = ['+', '-', '*', '/']; | |||
|
|
|||
| exports.create_calculator = function (operator) { | |||
There was a problem hiding this comment.
Currently create_calculator is a function which generates an object with functions. Would it be better as a class instead? For example:
export class Calculator {
constructor(operator) {
this.operator = operator;
}
add_number() {...}
calculate() {...}
}
This would make it slightly less messy! 😃
| answer = answer / number; | ||
| break; | ||
| default: | ||
| console.log("Invalid operator " + operator); |
There was a problem hiding this comment.
Should we throw this error when creating the calculator object, rather than during execution of the calculation. This way any errors regarding the operator will be thrown when the calculator is created, so it should be easier to spot when an invalid operator is used!
One way to do this would be to add in the constructor:
if (!operators.includes(operator)) {
// throw an error here
}
There was a problem hiding this comment.
On a related note, should this be throwing rather than logging? We should be consistent with the above error for when there are no numbers to calculate 🙂
| console.log('Enter the operator:'); | ||
| const operator = readline.prompt(); | ||
|
|
||
| console.log(`How many numbers do you want to ${operator} ?`); |
There was a problem hiding this comment.
This will read as "How many numbers do we want to - "? Is this how we want to format the question?
| calculator.add_number(n); | ||
| } | ||
|
|
||
| console.log('The answer is: ' + calculator.calculate()); |
There was a problem hiding this comment.
I think changing this to a template string would be really cool and fun and consistent with the log on line 9! 😎
| const calc = require('../calculator') | ||
|
|
||
| describe('calculator', function () { | ||
| describe('#calculate()', function () { |
There was a problem hiding this comment.
Is there a reason why there's a hash here? Just wanted to double check that this was the standard formatting for this repo! 😃
| const chai = require('chai'); | ||
| const calc = require('../calculator') | ||
|
|
||
| describe('calculator', function () { |
There was a problem hiding this comment.
These tests are an amazing start! 😍 Would it be worth having some tests for the other operators? 😀
| numbers.push(number); | ||
| }, | ||
| calculate: function () { | ||
| // Return zero if there are no numbers to calculate |
There was a problem hiding this comment.
Is this comment necessary? 😀 It contradicts what's actually going on below!
| const operator = readline.prompt(); | ||
|
|
||
| console.log(`How many numbers do you want to ${operator} ?`); | ||
| const c = parseInt(readline.prompt()); |
There was a problem hiding this comment.
Could we have a slightly more clear variable name here? Possibly something like "numberOfNumbersToOperateOn" or something slightly more concise and clear than that if you can think of one! 😅
|
|
||
| for (var i = 0; i < c; i++) { | ||
| console.log('Enter a number:'); | ||
| const n = parseInt(readline.prompt()); |
There was a problem hiding this comment.
Same here in regards to the variable names! Possibly inputtedNumber or something like that!
2 participants
No description provided.