Bug - : 05_sumALL: Returning ERROR with negative numbers

[Houndoom]

Complete the following REQUIRED checkboxes:

• [ o] I have thoroughly read and understand The Odin Project Contributing Guide
• [ o] The title of this issue follows the Bug - location of bug: brief description of bug format, e.g. Bug - Exercises: File type incorrect for all test files

The following checkbox is OPTIONAL:

• I would like to be assigned this issue to work on it

---

1. Description of the Bug:
The 4th test for this task checks that the function returns ERROR with negative numbers. However, the question did not state that the integers had to be non-negative, and it is in fact possible to obtain a proper answer even with negative numbers.

2. How To Reproduce:

1. Code without error checking for negative numbers.
2. 4th test fails

3. Expected Behavior:

1. Code without error checking for negative numbers.
2. 4th test should pass, with an answer of -45

4. Desktop/Device:

• Device: Virtual Machine
• OS: Ubuntu
• Browser: NIL
• Version: NIL

5. Additional Information:

[MahirMahdi]

Can you please please assign it to me? I've already passed all the tests. As soon as you assign it to me I'll make a pull request.

[MahirMahdi]

@Houndoom I've made a PR on the following issue. Please review it.

[Houndoom]

@Mahir1015 Thanks very much for looking into this! However, I am not a part of The Odin Project and hence am not able to assign issues or merge PRs, you may wish to approach someone on the discord who has the access to do so

[MahirMahdi]

@Houndoom Anytimee! Can you please provide me the link of discord server?

[Houndoom]

@Mahir1015 You can find it on this page: https://www.theodinproject.com/lessons/foundations-join-the-odin-community

[MahirMahdi]

@Houndoom Thanks!

[thatblindgeye]

@Houndoom would you be able to explain further the issue with the 4th test in this exercise? If a test fails, the intent is that a user refactors their function(s) until that test (and all previous tests) pass. In this case the test should pass by returning a string "ERROR" if any negative numbers are passed to the function.

[Houndoom]

@thatblindgeye There is nothing in the question that suggests that the task should fail for negative numbers. The term "integers" includes negative integers as well, and hence I would expect this function to work with negative integers. If the intention was for it to fail with negative integers, suggest to amend the question to state "positive integers".

[thatblindgeye]

@Houndoom Ah okay, I see what you mean. You're correct, we should either update the README to mention the function should only work for positive numbers, or add/update tests to work with negative numbers. Do you feel strongly one way or the other, coming from the point of view as a user?

[Houndoom]

@thatblindgeye I have no strong preference, but perhaps a slight preference for updating the README as it would be a good introduction to edge cases, and more learning is always good. If this route is taken, suggest to have an appropriate hint to guide the user to think about the edge case, and the desired return value of "ERROR" would have to be explicitly stated.

[thatblindgeye]

Sounds good to me! I'll open this for help wanted and update your original comment with what needs to be done

Actually as I type out a task list and think it over a little more, I'm thinking we may not really need to really mention this in the exercise description. While it isn't said at first that the function should only work for positive numbers, having a test that fails when a negative number is passed might have been intentional. It requires a user to have to refactor their function to continually pass the tests that may be added as edge cases are found.

So a function that just works with any numbers passed in at first is good as long as the first test passes, then test 2 and test 3. Once the user gets to test 4, it's kind of like, "Oh, we've found out we only need this function to work for positive integers and to error when any negative integer is passed in. Let me refactor my function so this test will pass now..." Which can be how writing functions and tests work in a real world scenario. You may not have every edge case thought of from the start, and the original spec may not include all the information (some info may not be known at that time). Once you have something created and the person who asked for it sees it, they may realize, "Oh you know what, we actually want to prevent this thing..."

[Houndoom]

That sounds reasonable. In a sense, we can take the tests as part of the specs. Personally, it feels off because rather than it being a case of not enough information, it seems like a contradiction to state integers when it actually means positive integers. In addition, in the context of a homework question, I would expect the description to state exactly what is required. There was also no indication that descriptions may be incomplete, and that tests may provide "additional information", and no explanation that this was meant to teach some of the considerations that might occur in a real world scenario, so it was just confusing to me.

Of course, that is my personal opinion. It does not change the fact that the aim is to write a code that passes all the tests, and the current formulation presents no practical impediment to this goal. If the Odin team does not feel that there is a problem, I will accept this decision.

[thatblindgeye]

I definitely get your point of view. Maybe it'd be beneficial if we mentioned somewhere in the README that certain tests may require the user to refactor their code or that the description in each exercise's README may not provide the full story. I personally might be more of the opinion that it kinda gets into semantics as to whether there's significant benefit in mentioning that vs a user discovering it on their own, but I'm but one man 😆

Let me ping @TheOdinProject/javascript to see if anyone else has opinions on this, and if any other users see this issue and have opinions definitely let us know

[MaoShizhong]

Had a look through and I see sensible points on both sides.
My personal opinion is in favour of editing the README to say

Implement a function that takes 2 positive integers...

At first, I thought people would just jump to doing everything in a conditional block if both params are positive integers, or perhaps even a guard clause for negative integers, and that this would therefore not run into the "refactor now you've bumped into a failing test" scenario.

But looking at the tests, if they did this then they would still fail because the test specifically expects the string 'ERROR' to be returned in that scenario. If they did what is described above (which would be somewhat likely), that test would receive undefined and thus fail.

So they'd still need to refactor to return the expected string.

How does that sound?

[thatblindgeye]

I'd be good with just updating the README with that quick change. @Houndoom would you be interested in making this update? I know you didn't check the box for being interested in your original post, but in case you've changed your mind.

[Houndoom]

Thanks @thatblindgeye for the offer, I'm not interested in performing the update