search

decrypto-org / blockchain-course

An interactive course on blockchain science and engineering
MIT License
14 stars 11 forks source link

Add .fail() function for early judgement failure #46

Closed dionyziz closed 5 years ago

dionyziz commented 5 years ago

I think the asymmetry here is fine. In case of failure, this should propagate up the call stack as an exception, but there's no corresponding control flow in case of success -- we require the judge to return, not throw.

OrfeasLitos commented 5 years ago

Is javascript OK with using errors for control flow? This isn't python, errors could be too expensive for us.

OrfeasLitos commented 5 years ago

Now that I read the code again, I'm not sure I agree with this usage of errors. It is not obvious that this may throw. A reader can be misled to think that judge always returns BaseJudge.PASS.

dionyziz commented 5 years ago

@OrfeasLitos I don't understand your concern about errors being expensive. Why would exception be expensive in JS?

dionyziz commented 5 years ago

@OrfeasLitos Regarding the lack of clarity in the line of code you pointed to, this is the reason why the function was renamed to "ensureContainsDesiredOutput". Do you think this is insufficient?

OrfeasLitos commented 5 years ago

@OrfeasLitos I don't understand your concern about errors being expensive. Why would exception be expensive in JS?

The argument in favor of throwing is that once the judge decides that a solution is wrong, the execution path is very clear. It is not the case that throwing may lead to complex execution paths. This point also defeats my previous argument about speed, since each submission can cause at most one error.

The assymetry still bothers me a bit, but I'd be OK with it if we make clearer which parts of the code may throw. Regarding ensureContainsDesiredOutput(), we can add a // may throw IncorrectSolutionError comment over it. There may be a cleaner solution, but I can't come up with anything else that won't be too complex.

dionyziz commented 5 years ago

Sure, I can put that comment near ensureContainsDesiredOutput(), but this is in a different PR.