7
\$\begingroup\$

Regarding this question, OP has stated his code works under SpiderMonkey 1.8.5, but not in NodeJS. I've looked at the SpiderMonkey 1.8.5 page and it states that;

Note that version 1.8.5 is outdated. The latest version of SpiderMonkey can always be found on the source download page.

In an effort to try and correctly review this code, I usually first copy the original code onto my machine and create test cases to replicate the expected functionality. That way, once I refactor the code into a 'nicer' form, I can be sure the code still works.

However, I tried to run this code on IoJS v2.3.4, which is an implementation of the v8 engine that supports ES6, in particular, it supports generators (which is the use of the yield keyword in tandem with function* to create potentially infinite sequences).

Under this environment, running the code results in a syntax error:

/home/dan/code-review/96661-project-euler-52-permuted-multiples/original.js:9
      yield start;
            ^^^^^
SyntaxError: Unexpected identifier
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:413:25)
    at Object.Module._extensions..js (module.js:448:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:471:10)
    at startup (node.js:117:18)
    at node.js:948:3

In ES5 (and lower versions, which is what SpiderMonkey runs), the yield keyword is considered a reserved word, and any ES5-compliant JavaScript engines should raise the error Unexpected reserved word: yield. And in the ES6 engine - which is when the yield keyword came into standard parlance, I'm getting a SyntaxError: Unexpected identifier. Indeed, the latest stable release of node (node v0.12.7) gives us a similar error:

dan@dan-BB-35708A:~/code-review/96661-project-euler-52-permuted-multiples$ nvm i stable
######################################################################## 100.0%
Now using node v0.12.7 (npm v2.11.3)
/home/dan/code-review/96661-project-euler-52-permuted-multiples/original.js:9
      yield start;
            ^^^^^
SyntaxError: Unexpected identifier
    at exports.runInThisContext (vm.js:73:16)
    at Module._compile (module.js:443:25)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3

As far as I am concerned, this question has non-working code, but OP states it does work (for him) on SpiderMonkey 1.8.5. Should we allow this question because it works on a particular version of a browser engine (one I am convinced has a bug because it's not standards compliant given its treatment of yield) or should it be closed due to non-working code?


Interesting, I've just tested this code on SpiderMonkey 2.4.0 (only version I can seem to find on the Ubuntu package repository) and the code does actually work.

dan@dan-BB-35708A:~/code-review/96661-project-euler-52-permuted-multiples$ js24 original.js
142857
\$\endgroup\$
8
\$\begingroup\$

There are only two reasons this question should be closed as broken code.

  1. The OP is saying it is broken in a particular environment (but not others) and is asking for help making it work in these environments.
  2. You are certain that the code is actually broken in the environment that the OP is claiming the code works in.

Different projects have different constraints. For whatever reason, the OP may be required, for his project, to be using SpiderMonkey 1.8.5 and whatever browser version he claims it works on. And the constraint of his project states that this is the only environment that it must work in.

If that is the case, and it actually does work in the OP's specified environment, I can't see how the code is broken.

Arguably, the question could potentially use some more clarity on what environments the OP requires working for perhaps (since he has partially stated some environments he is aware it does not work in).

But consider iOS. With every version of iOS, Apple deprecates methods, and eventually your iOS 5.0 code isn't going to run in iOS 9.0 (depending on what the code is). Does that mean when iOS 9.0 comes out, I should sort through all of the iOS 5.0 questions and close them as broken if they contain code that won't work on iOS 9.0? This seems unquestionably unreasonable.

The OP can specific specific environments & constraints for their code. As long as it works in these environments & constraints, it's not broken code.

\$\endgroup\$
  • \$\begingroup\$ I would agree that if OP absolutely has to only support SpiderMonkey 1.8.5 (or whatever else), then it is working code. However, his post states no such requirement - it's a project Euler question - and it's syntactically incorrect JavaScript as per the ES5 specification. The only reason it would ever work is by a bug or incorrect implementation of the ES5 specification. Would the appropriate course of action be to review the code but leave a warning indicating that the code would not be invalid in other environments (such as Chrome, both NodeJS/IoJS and possibly Chakra)? \$\endgroup\$ – Dan Pantry Jul 12 '15 at 14:27
  • 3
    \$\begingroup\$ I'm not commenting on how you should answer. I added some comments about iOS, which I'm more familiar with. If today someone posted some ancient form of Objective-C that would compile in Xcode 3, but wouldn't compile in Xcode 6.4 or Xcode 7, I wouldn't be voting to close the question as broken (unless I felt like finding a version of Xcode 3 and prove it was broken there), but I also wouldn't feel the urge to answer an outdated and ultimately irrelevant question. Just because the question isn't off-topic doesn't mean it's worth answering. \$\endgroup\$ – nhgrif Jul 12 '15 at 14:30
5
\$\begingroup\$

If the OP claims the code works on some version X of software Y, and you cannot reproduce that given environment, and you don't have reasonable suspicion that the OP is lying, then the code shouldn't be considered broken. (Innocent until proven guilty.)

If version X of software Y is obsolete, that doesn't make the code broken either. But it might mean that reviewing it will not be worth your time.

Your effort to reproduce the OP's environment is admirable. This is also the way I like to answer questions: reproduce, test, improve, re-test, answer. Sometimes it's just too hard to reproduce, and after some time of tinkering I just have to accept defeat and move on.

Bottom line: OP's code seems to be working, leave it open.

\$\endgroup\$
0
\$\begingroup\$

In this situation, where it seems likely that the code is broken, but the OP claims to be targeting an environment that is difficult to reproduce, I would have no hesitation in:

  • Politely requesting that the OP make the code more portable and easier to review. In most cases, the obscure environment is not necessary.
  • Down-voting the question. I want to discourage people from posting code that is hard to run and hard to review (even if on-topic), and the down-vote is the way to do that.
  • Not putting any more work into answering the question until (if ever) the OP improves the code.
\$\endgroup\$

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .