Beta

Abstract Model Magic

28 of 31nrgarg
Description
Loading description...
Functional Programming
Refactoring
View
AllIssues3QuestionsSuggestions3Show Resolved
  • Please sign in or sign up to leave a comment.
  • JohanWiltink Avatar

    ( JS )

    Error message for failing Magic.proto.register instanceof Function assumes it does not exist. There are other reasons this test could fail.

    Same for instance.name.

  • JohanWiltink Avatar

    ( JS )

    Testing for prototype methods depends on them being enumerable. It also does not actually test register is the only one.

  • JohanWiltink Avatar

    ( JS, possibly others )

    Example tests would be nice.

  • Voile Avatar

    Needs random tests.

  • Voile Avatar

    Do you actually need the register method for this task anyway? It can be easily done inside the MagicModel constructor.

  • taw Avatar

    It's worded in extremely confusing way and lacks unit tests.

    • nrgarg Avatar

      Can you specify what was confusing about the details?

      Also - what cases do you think should be covered for the unit tests (that haven't been already)?

    • taw Avatar

      Well, ruby unit test box is empty, and description doesn't say if it's supposed to have setters, if attributes can be added outside initializer etc.

      The "no attributes holding data, no instance variables being passed around" part is also somewhat confusing. Is it expecting #methods #responds_to? etc. to work or just #name etc.

    • nrgarg Avatar

      Hm, how bout one more test case then?

      a = MagicModel.new({})
Test.assert_equals(a.methods - Object.methods, [:register])

      Since :name and :initialize are Object methods, we check to make sure :register is the only method that exists on the instance.

      With regards to the instance variables part - the idea is that you don't use attr_accessor or explicitly define the name method and use an instance variable to pass around the initialized name value

      i.e.

      def initialize init
    @name = init[:name]
end

def name
    @name
end

      And there is a test to make sure that there are no instance variables initialized.

    • taw Avatar

      Well, test it explicitly if that's requirement, preferably in unit test box too.

    • nrgarg Avatar

      The method assertion is now in. The instance variable test was already there though.

  • wthit56 Avatar

    Man this kata was confusing... You leave a whole ton of stuff out, and add some things which are just wrong/confusingly worded.

    • The register, get, and set should do, and what arguments they should expect, should be stated somewhere.
    • The constructor's init object may be undefined, despite your note that the "object expects to be initialized with the name property".
    • You say "attributes should not be directly exposed in the instance", by which you mean the methods should be directly exposed in the instance.
    • "DO NOT make a name prototype method. Thats not how javascript works." Wat?
    • You put "self" everywhere in the stub, but no "self" variable is needed. And if it would be needed because of the way the coder is implementing things, then they should know that var self = this; is necessary for it to work.
    • You don't seem to actually test the register, get, and set methods.

    I hope these don't sound too harsh; I just became very frustrated with this kata, writing elaborate methods to avoid adding anything to the instance, then failing miserably, slowly coming round to the fact that I understood pretty much 1% of the description overall.

    I hope these suggestions will be helpful, at least for others to see if they get stuck.

    • nrgarg Avatar

      Nope - definitely open to criticism. It's the first kata I've ever written and definitely needs polishing.

      • I've updated the boilerplate code and removed all of the self references. That was more of a personal coding style that was bleeding in to the end result - not exactly something I need to impose.

      • I've updated the description to elaborate on the tasks at hand and how each prototype method works.

      • Updating the test cases to actually test those prototype methods...Yeah I can't believe I skipped that entirely. On it :)

    • nrgarg Avatar

      Also, I love your solution. This is basically the reason why I love this site :). Essentially, the getter and setter methods are obsolete. Since real work is happening in the "register" method, should I just remove the dependency on get and set ?

    • nrgarg Avatar
      Suggestion marked resolved by nrgarg 10 years ago
  • Abbe Avatar

    This comment has been hidden.