Beta

Basic array operations - WoW Server

7Jejek
Description
Loading description...
Algorithms
Arrays
View
AllIssues4QuestionsSuggestionsShow Resolved
  • Please sign in or sign up to leave a comment.
  • Voile Avatar
     
    Should properly remove character by class & spec: expected [ Array(15) ] to deeply equal [ Array(15) ]

    You need to set chai.config.truncateThreshold = 0 like this:

    import { assert, config } from "chai";
config.truncateThreshold = 0;
  • Voile Avatar

    The tests require the players to be kept track in a specific order (specifically, ordered by insert time ascending. Otherwise remove will yield different results). This is not mentioned, and very unexpected: the server should have a collection (and not an array) of players, which does not have a notion of implicit order.

    • Jejek Avatar

      What do you think about adding incremential Id or some kind of creation timestamp to Character and modification of description to:

      • return values ordered by Id/time
      • remove values in creation order
    • Voile Avatar

      Either that, or just specify that it's meant to be stored as a list and filtering is done as list filtering, so sequentially. Array only appears in the title and nowhere in the description.

    • Jejek Avatar

      Updated description, hope it will be now clear.

      Issue marked resolved by Jejek 2 years ago
  • Voile Avatar

    Some test cases look like they are written with no proper knowledge of JS/TS:

    server.remove("Troll 12", 5);

    This does not do what you expect: it's calling server.remove("Troll 12", 5, undefined, undefined) instead of server.remove("Troll 12", undefined, undefined, 5). It only passes the typing test because Class.Dh's enum value is also 5 (aka it's actually server.remove("Troll 12", Class.Dh, undefined, undefined)).

    In general this will be a pain for everyone to write code against this, including you. It'd be better to use a special filter object that has every field optional, then have count be the second argument.

    • Jejek Avatar

      You are right.

      It's relict of old interface before (new ideas) changes applied. I missed this one to updated by undefined params.

      And you are right, for normal use the filter object would make sense. I kinda didnt want to add more boiler plate things and didnt expect (at first) so "many" params.

      Test fixed.

      Issue marked resolved by Jejek 2 years ago
  • Voile Avatar

    There are lots of things missing in the tests and description:

    • It's not mentioned that all filtering/finding of name should be case-insensitive
    • Also remove/removeFirst does not test the above even though find does
    • Description says If count is undefined do NOT remove characters, but this is not tested. But then why is count optional when being undefined just has no effects? It seems like a poorly designed API. (In fact the tests are already misusing this API, this will be a separate issue)
    • Sample tests do not test a proper implementation of remove/removeFirst at all, since a default no-op implementation will pass the tests chosen to be put into
    • Needs test cases with empty string name for all find/remove APIs to catch improper optional checking
    • Jejek Avatar

      Thanks for feedback!

      Working on fixes.

      • modified description

      • Added missing tests.

      • Changed base logic (undefined count should be replaced with default value: 1) (and modified description)

      • Added sample tests for removes

      • about empty name.. it seems kinda logic, but.. noone said that empty string is invalid name. so.. empty string as filter doesnt seems to be sth which (right now) should be treated as optinal.

      i wonder if its better to change description (so empty string is invalid name) or add validation if someone (by mistake) didnt treated it as optional

      Issue marked resolved by Jejek 2 years ago
    • Voile Avatar

      about empty name.. it seems kinda logic, but.. noone said that empty string is invalid name. so.. empty string as filter doesnt seems to be sth which (right now) should be treated as optinal.

      The point is that checking for undefined should be explicit, and not just checking on truthy/falsy values, since empty string is also falsy. There will definitely be someone doing this if you don't specifically test for it ;-)

    • Jejek Avatar

      Oh, now i understand what you meant.

      Today i learned one more thing about js. Thanks. Will fix.