Proposal: Block stub throw implementations in two-fer analyzer

Programming JavaScript
1

Hi maintainers :wave:

I’d like to propose a small improvement to the JavaScript analyzer
for the two-fer exercise.

Problem:
Currently, placeholder implementations like:

export function twoFer() {
throw new Error(“Please implement”);
}

are treated as valid solutions by the analyzer.

Proposal:

  • Detect functions whose body is a single throw new Error(...)
    with common stub messages (e.g. “Please implement”)
  • Block such submissions
  • Provide an actionable comment instructing students to remove
    the placeholder code

Implementation:

  • Added a reusable hasStubThrow utility
  • Added an actionable comment for stub throws
  • Integrated the check into the two-fer analyzer flow
  • All existing tests pass

If this approach is approved, I can link the existing PR for review.

Thanks!

1 Like
2

I can 100% get behind something like this. I’m thinking we should probably consider if this check can be applied in general to all exercises, but it might need some rework to get there.
My motivation is that I’ve often seen solutions that just leave the placeholder throw after the return and it’s definitely not a good practice to be teaching.

1 Like
3

I thought the analyzer only runs after the tests pass. If that’s the case, the analyzer would never see code like that. Am I mistaken?

4

That’s my understanding as well. The tests also can’t pass if javascript/exercises/practice/two-fer/two-fer.js at 55274239b22ed18bf0da0183833415c157e8a2ea · exercism/javascript · GitHub isn’t removed. That line instructs students to remove it, so we’re already doing what the proposal suggests but at an earlier point.

Cool-Katt’s idea is a sound one about flagging an unreachable placeholder throw. I see that fairly regularly when mentoring JS solutions, and that would be handled nicely by the analyzer.

1 Like
5

@IsaacG you are missing the following case that @Cool-Katt means and @samirhssn is trying to address:

export function foo() {
  return 42
  throw new Error('old stub error that is left here');
}

So yes, the analayzer doesn’t produce if the tests fail, but the tests would not fail in this case.

I am in favour of adding this to the analyzer to all exercises.

1 Like
6

That case would be nice to flag. That case is not the code in the original post nor would it be handled by the OP’s PR, though. My comment was about what the OP mentioned and PRed.

Having the analyzer detect unreachable code (even if it is only a very specific line of unreachable code) would definitely be nice, though!

2 Likes
7

Yes, that and the information you already provided that the analyzer only needs to consider passing solutions should be it.

Dead code detection would be awesome, but what’s already good enough is to see if the stub throw is present. I think we wanted to normalize the error message in JS (right @Cool-Katt ?) So the check would be really easy.

2 Likes