Some of the code seems to be missing - I can't see where you assign @numbers. I'm going to assume that it is set to "//+\n1+2" or the like.
DRY - twice in your code you calculate the delimiter (@numbers[2,1]), either pass it as an argument, or make it a method.
Btw, you seem to have a syntax error there, you might have meant to say supports? ? @numbers... instead.
Be clear, not clever you show good control in ranges, array, chars, etc., but still the following will be much clearer to future readers of your code (you included!)
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
(if you don't like to see escaped sequences, you can use %{!"\#$%&'()*+,-.:;<=>?@} instead)
Be Ruby - when you need to return a boolean, which is returned on a condition, there is no need to explicitly say .. ? true : false, simply return the condition result
Harness the power of regular expressions - when matching strings, captures are set to $n $n variables - you can use this to check your delimiter more elegantly:
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
def supports?(delim)
ValidDelimiters.include? delim
end
def delimiter
return ',' unless @numbers.match(%r{^//(.)})
delim = 1ドル
supports?(delim) ? delim : fail('Unsupported delimiter')
end
Edit - with the new code
Naming - choose names that convey the meaning of the member - @numbers implicitly say it is a group of numbers, although it is actually a string... better name might be input_expression or something like that.
Redundant code with side-effects - your constructor accepts a string as input, which you assign to @numbers. This is not part of the requirements, and you probably don't have tests for that, but it may have serious impact on your result!
Also, for each time you call add, the input is concatenated to the end of the previous result (also not part of the requirements), which will break your code.
Assume nothing - Ruby's to_i method is very liberal - anything that is not a number will be parsed to - 0, so an input like "1,2,goat,3" will give a valid result of 6...
When state is redundant - when assuming each add is supposed to be a separate calculation - no state is needed. You can simply pass the expression around to your helper methods, and return the result. You can even turn your class into a module, with all methods being class methods, and the usage will be `StringCalculator.add('//+\n1+2')
Some of the code seems to be missing - I can't see where you assign @numbers. I'm going to assume that it is set to "//+\n1+2" or the like.
DRY - twice in your code you calculate the delimiter (@numbers[2,1]), either pass it as an argument, or make it a method.
Btw, you seem to have a syntax error there, you might have meant to say supports? ? @numbers... instead.
Be clear, not clever you show good control in ranges, array, chars, etc., but still the following will be much clearer to future readers of your code (you included!)
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
(if you don't like to see escaped sequences, you can use %{!"\#$%&'()*+,-.:;<=>?@} instead)
Be Ruby - when you need to return a boolean, which is returned on a condition, there is no need to explicitly say .. ? true : false, simply return the condition result
Harness the power of regular expressions - when matching strings, captures are set to $n variables - you can use this to check your delimiter more elegantly:
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
def supports?(delim)
ValidDelimiters.include? delim
end
def delimiter
return ',' unless @numbers.match(%r{^//(.)})
delim = 1ドル
supports?(delim) ? delim : fail('Unsupported delimiter')
end
Edit - with the new code
Naming - choose names that convey the meaning of the member - @numbers implicitly say it is a group of numbers, although it is actually a string... better name might be input_expression or something like that.
Redundant code with side-effects - your constructor accepts a string as input, which you assign to @numbers. This is not part of the requirements, and you probably don't have tests for that, but it may have serious impact on your result!
Also, for each time you call add, the input is concatenated to the end of the previous result (also not part of the requirements), which will break your code.
Assume nothing - Ruby's to_i method is very liberal - anything that is not a number will be parsed to - 0, so an input like "1,2,goat,3" will give a valid result of 6...
When state is redundant - when assuming each add is supposed to be a separate calculation - no state is needed. You can simply pass the expression around to your helper methods, and return the result. You can even turn your class into a module, with all methods being class methods, and the usage will be `StringCalculator.add('//+\n1+2')
Some of the code seems to be missing - I can't see where you assign @numbers. I'm going to assume that it is set to "//+\n1+2" or the like.
DRY - twice in your code you calculate the delimiter (@numbers[2,1]), either pass it as an argument, or make it a method.
Btw, you seem to have a syntax error there, you might have meant to say supports? ? @numbers... instead.
Be clear, not clever you show good control in ranges, array, chars, etc., but still the following will be much clearer to future readers of your code (you included!)
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
(if you don't like to see escaped sequences, you can use %{!"\#$%&'()*+,-.:;<=>?@} instead)
Be Ruby - when you need to return a boolean, which is returned on a condition, there is no need to explicitly say .. ? true : false, simply return the condition result
Harness the power of regular expressions - when matching strings, captures are set to $n variables - you can use this to check your delimiter more elegantly:
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
def supports?(delim)
ValidDelimiters.include? delim
end
def delimiter
return ',' unless @numbers.match(%r{^//(.)})
delim = 1ドル
supports?(delim) ? delim : fail('Unsupported delimiter')
end
Edit - with the new code
Naming - choose names that convey the meaning of the member - @numbers implicitly say it is a group of numbers, although it is actually a string... better name might be input_expression or something like that.
Redundant code with side-effects - your constructor accepts a string as input, which you assign to @numbers. This is not part of the requirements, and you probably don't have tests for that, but it may have serious impact on your result!
Also, for each time you call add, the input is concatenated to the end of the previous result (also not part of the requirements), which will break your code.
Assume nothing - Ruby's to_i method is very liberal - anything that is not a number will be parsed to - 0, so an input like "1,2,goat,3" will give a valid result of 6...
When state is redundant - when assuming each add is supposed to be a separate calculation - no state is needed. You can simply pass the expression around to your helper methods, and return the result. You can even turn your class into a module, with all methods being class methods, and the usage will be `StringCalculator.add('//+\n1+2')
Some of the code seems to be missing - I can't see where you assign @numbers. I'm going to assume that it is set to "//+\n1+2" or the like.
DRY - twice in your code you calculate the delimiter (@numbers[2,1]), either pass it as an argument, or make it a method.
Btw, you seem to have a syntax error there, you might have meant to say supports? ? @numbers... instead.
Be clear, not clever you show good control in ranges, array, chars, etc., but still the following will be much clearer to future readers of your code (you included!)
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
(if you don't like to see escaped sequences, you can use %{!"\#$%&'()*+,-.:;<=>?@} instead)
Be Ruby - when you need to return a boolean, which is returned on a condition, there is no need to explicitly say .. ? true : false, simply return the condition result
Harness the power of regular expressions - when matching strings, captures are set to $n variables - you can use this to check your delimiter more elegantly:
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
def supports?(delim)
ValidDelimiters.include? delim
end
def delimiter
return ',' unless @numbers.match(%r{^//(.)})
delim = 1ドル
supports?(delim) ? delim : fail('Unsupported delimiter')
end
Edit - with the new code
Naming - choose names that convey the meaning of the member - @numbers implicitly say it is a group of numbers, although it is actually a string... better name might be input_expression or something like that.
Redundant code with side-effects - your constructor accepts a string as input, which you assign to @numbers. This is not part of the requirements, and you probably don't have tests for that, but it may have serious impact on your result!
Also, for each time you call add, the input is concatenated to the end of the previous result (also not part of the requirements), which will break your code.
Assume nothing - Ruby's to_i method is very liberal - anything that is not a number will be parsed to - 0, so an input like "1,2,goat,3" will give a valid result of 6...
When state is redundant - when assuming each add is supposed to be a separate calculation - no state is needed. You can simply pass the expression around to your helper methods, and return the result. You can even turn your class into a module , with all methods being class methods, and the usage will be `StringCalculator.add('//+\n1+2')
Some of the code seems to be missing - I can't see where you assign @numbers. I'm going to assume that it is set to "//+\n1+2" or the like.
DRY - twice in your code you calculate the delimiter (@numbers[2,1]), either pass it as an argument, or make it a method.
Btw, you seem to have a syntax error there, you might have meant to say supports? ? @numbers... instead.
Be clear, not clever you show good control in ranges, array, chars, etc., but still the following will be much clearer to future readers of your code (you included!)
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
(if you don't like to see escaped sequences, you can use %{!"\#$%&'()*+,-.:;<=>?@} instead)
Be Ruby - when you need to return a boolean, which is returned on a condition, there is no need to explicitly say .. ? true : false, simply return the condition result
Harness the power of regular expressions - when matching strings, captures are set to $n variables - you can use this to check your delimiter more elegantly:
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
def supports?(delim)
ValidDelimiters.include? delim
end
def delimiter
return ',' unless @numbers.match(%r{^//(.)})
delim = 1ドル
supports?(delim) ? delim : fail('Unsupported delimiter')
end
Some of the code seems to be missing - I can't see where you assign @numbers. I'm going to assume that it is set to "//+\n1+2" or the like.
DRY - twice in your code you calculate the delimiter (@numbers[2,1]), either pass it as an argument, or make it a method.
Btw, you seem to have a syntax error there, you might have meant to say supports? ? @numbers... instead.
Be clear, not clever you show good control in ranges, array, chars, etc., but still the following will be much clearer to future readers of your code (you included!)
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
(if you don't like to see escaped sequences, you can use %{!"\#$%&'()*+,-.:;<=>?@} instead)
Be Ruby - when you need to return a boolean, which is returned on a condition, there is no need to explicitly say .. ? true : false, simply return the condition result
Harness the power of regular expressions - when matching strings, captures are set to $n variables - you can use this to check your delimiter more elegantly:
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
def supports?(delim)
ValidDelimiters.include? delim
end
def delimiter
return ',' unless @numbers.match(%r{^//(.)})
delim = 1ドル
supports?(delim) ? delim : fail('Unsupported delimiter')
end
Edit - with the new code
Naming - choose names that convey the meaning of the member - @numbers implicitly say it is a group of numbers, although it is actually a string... better name might be input_expression or something like that.
Redundant code with side-effects - your constructor accepts a string as input, which you assign to @numbers. This is not part of the requirements, and you probably don't have tests for that, but it may have serious impact on your result!
Also, for each time you call add, the input is concatenated to the end of the previous result (also not part of the requirements), which will break your code.
Assume nothing - Ruby's to_i method is very liberal - anything that is not a number will be parsed to - 0, so an input like "1,2,goat,3" will give a valid result of 6...
When state is redundant - when assuming each add is supposed to be a separate calculation - no state is needed. You can simply pass the expression around to your helper methods, and return the result. You can even turn your class into a module , with all methods being class methods, and the usage will be `StringCalculator.add('//+\n1+2')
Some of the code seems to be missing - I can't see where you assign @numbers. I'm going to assume that it is set to "//+\n1+2" or the like.
DRY - twice in your code you calculate the delimiter (@numbers[2,1]), either pass it as an argument, or make it a method.
Btw, you seem to have a syntax error there, you might have meant to say supports? ? @numbers... instead.
Be clear, not clever you show good control in ranges, array, chars, etc., but still the following will be much clearer to future readers of your code (you included!)
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
(if you don't like to see escaped sequences, you can use %{!"\#$%&'()*+,-.:;<=>?@} instead)
Be Ruby - when you need to return a boolean, which is returned on a condition, there is no need to explicitly say .. ? true : false, simply return the condition result
Harness the power of regular expressions - when matching strings, captures are set to $n variables - you can use this to check your delimiter more elegantly:
ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"
def supports?(delim)
ValidDelimiters.include? delim
end
def delimiter
return ',' unless @numbers.match(%r{^//(.)})
delim = 1ドル
supports?(delim) ? delim : fail('Unsupported delimiter')
end