Ad
  • Custom User Avatar

    This comment is hidden because it contains spoiler information about the solution

  • Custom User Avatar

    Resolved with update fork to Node 18

  • Custom User Avatar

    It's an error to me simply because authors/translators should never be rolling their own shuffle/random when we have lodash available for JS, I'll change it.

  • Custom User Avatar

    @hobovsky You make a good point, and I don't disagree, but I think the bigger issue is that katas, much like Science experiments, should have as much of a controlled environment as possible to ensure the user can focus on the variable of solving the problem and not figuring out an error unrelated to the problem. Not that this isn't a good lesson for the user to learn, it's just not the focus of the kata and should be avoided. If part of the training was ensuring the user wasn't using for...in for this reason then yes, this would absolutely be user error.
    Also, the kata should be held to a higher standard than the user's code, so the Array.prototype.shuffle addition is an error with the kata (IMO).

  • Custom User Avatar

    There is no reason for the "for in" not to work here, even if other constructs are available. This is a kata issue, not a user issue.

  • Custom User Avatar

    Yes, extending built-in types should be avoided, but let me ask again: isnt it that a programmer who decided to use for...in does things incorrectly, because the for...in simply does not work in a way which allows to iterate over arrays (or anything else) reliably? By using potentially misbehaving features, user brings the problem on themselves - especially in face of the fact that there are easily available replacements which work correctly (like for...of).

    I still think that using a wrong language construct (i.e. for...in) is solver's error, unless, for some reason, for..in (i.e. "iterate over all existing keys") would be the solution for this problem, but I don't think it is?

  • Custom User Avatar

    You are correct, as stated in the Stackoverflow link. for..in should be avoided for this reason as should adding properties to built-in Object prototypes.

  • Custom User Avatar
  • Custom User Avatar

    I might be not a JS person, but I think that for...in should not be used for exactly this reason, and should be replaced with for...of?

    
    let arr = ['a', 'b'];
    Array.prototype.shuffle = () => {};
    for (let i of arr) { console.log(i); }
    // console:
    // a
    // b
    

    I think it's not a kata issue, but a misuse of a language feature? Unless, for some reason, you need to iterate exactly over indices and not actual values? Is this a case in this kata?

  • Custom User Avatar

    [javascript] 'shuffle' exposed as index on arrays.

    For users having this issue:
    The quick fix for this in your code is to change your for..in loops to this:

    // old code:
    for (let i in voters) {
      // throwing error of voters[i] being undefined because i === 'shuffle'
    }
    // new code:
    for (let [i] of voters.entries()) {
      // Unchanged code
    }
    

    For Kata Author:
    In test cases, this is getting added to the Array object's prototype:
    Array.prototype.shuffle=function(){...}
    As a result, 'shuffle' gets added as an "index" in for..in loops.
    Example:

    let arr = ['a', 'b'];
    for (let i in arr) { console.log(i); }
    // console:
    // 0
    // 1
    Array.prototype.shuffle = () => {};
    for (let i in arr) { console.log(i); }
    // console:
    // 0
    // 1
    // shuffle
    

    This will inadvertently cause an error in a user's solution who are using a for..in loop in their solution.

    for (let i in voters) {
      let topVote = voters[i][0]; // throws an error because voters['shuffle'] is undefined
    }
    

    In general, it's best to avoid modifying the prototype of built-in objects. Instead just create a standalone shuffle helper function, or if you absolutely need to add it to the Array prototype, use the Object.defineProperty method:

    Object.defineProperty(Array.prototype, 'shuffle', {
      value: () => { /* add code here */ }
    });
    

    Relevant Stackoverflow: https://stackoverflow.com/a/948379/354317

  • Custom User Avatar

    Also, in the instructions here:

    This reader exposes only one method : getChunk()

    Returns the following fragment of text from the file it is reading
    Returns a string of random size
    Returns at least one char
    Returns an empty string when finished

    I would just get rid of "Returns at least one char" Maybe just have this:

    This reader exposes only one method : getChunk()
    Returns a fragment of text from the file it is reading as a string.
    The fragment can be any length, including 0 (empty string).
    If the fragment is an empty string, the reader has reached the end of the file, and will only return an empty string on subsequent calls.

    I think I understand why it says it will return at least one char, but I think it means to say, "if doesn't return an empty string, the string will be a minimum of 1 char" which is kind of redundant info and confusing.

  • Custom User Avatar

    but what about this test case:

    testX:
    {
      text: "A\n\nB",
      words: 2,
      chars: 2,
      lines: 2 // or 3?  If 3, then an "empty line" is a line, and thus test4 in your example should have 1 line.
    }
    
  • Custom User Avatar

    Empty string is a special case which doesn't really make sense in a reader.
    But anyhow, that would means nothing, thus :

          test4:
          {
            text: "",
            words: 0,
            chars: 0,
            lines: 0
          }
    

    i do agree this is in conflict with the "Returns at least one char" instruction
    (i don't remember if this was in the description i specified 9 years ago or if someone added that line since then)
    but defensive programming is always recommended and those values are easily testable

    as soon as there is one char, you get a line and a word

          test5:
          {
            text: "A",
            words: 1,
            chars: 1,
            lines: 1
          }
    

    but there is a special weird case, what if the text as chars, but it only contains spaces ?
    ...
    there is no word!

          test6:
          {
            text: " ",
            words: 0,
            chars: 1,
            lines: 1
          },
    
  • Custom User Avatar

    From the instructions:

    This reader expose only one method : getChunk()
    
    Returns the following fragment of text from the file it is reading
    Returns a string of random size
    Returns at least one char
    Returns an empty string when finished
    

    In the instructions, it states that the getChunk function exposed by the reader "Returns at least one char".
    One of the test cases, the first chunk received by the parse method is an empty string (so the entire string it was reading must be an empty string), thus making the charCount, wordCount, and lineCount expected to be 0.
    Even if it was allowed that the first string received by getChunk was an empty string, there are other tests where there are empty lines that count as a line, so wouldn't an empty string test case still have a lineCount of 1?

    Example:

    chunk = 'foo\n\nbar'
    expected result:
    charCount: 6
    wordCount: 2
    lineCount: 3
    

    and if that's the case, then shouldn't this be true:

    chunk = ''
    charCount: 0
    wordCount: 0
    lineCount: 1
    
  • Custom User Avatar

    Unfortunately that's how CW test runner is written, so I can't overwrite it ;-)

  • Loading more items...