{"action":"created","issue":{"url":"https://api.github.com/repos/adamhooper/js-sorted-set/issues/4","labels_url":"https://api.github.com/repos/adamhooper/js-sorted-set/issues/4/labels{/name}","comments_url":"https://api.github.com/repos/adamhooper/js-sorted-set/issues/4/comments","events_url":"https://api.github.com/repos/adamhooper/js-sorted-set/issues/4/events","html_url":"https://github.com/adamhooper/js-sorted-set/issues/4","id":114176575,"number":4,"title":"RedBlackTreeStrategy throws if value is already in set?","user":{"login":"mxdubois","id":1776075,"avatar_url":"https://avatars.githubusercontent.com/u/1776075?v=3","gravatar_id":"","url":"https://api.github.com/users/mxdubois","html_url":"https://github.com/mxdubois","followers_url":"https://api.github.com/users/mxdubois/followers","following_url":"https://api.github.com/users/mxdubois/following{/other_user}","gists_url":"https://api.github.com/users/mxdubois/gists{/gist_id}","starred_url":"https://api.github.com/users/mxdubois/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mxdubois/subscriptions","organizations_url":"https://api.github.com/users/mxdubois/orgs","repos_url":"https://api.github.com/users/mxdubois/repos","events_url":"https://api.github.com/users/mxdubois/events{/privacy}","received_events_url":"https://api.github.com/users/mxdubois/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-10-30T00:49:42Z","updated_at":"2015-10-30T18:18:05Z","closed_at":null,"body":"Here's the offending line of code: https://github.com/adamhooper/js-sorted-set/blob/master/src/SortedSet/RedBlackTreeStrategy.coffee#L68\r\n\r\nI would expect the set to no-op when the value is already in the set. Is there a reason it throws? Or perhaps that just came over from the original source?"},"comment":{"url":"https://api.github.com/repos/adamhooper/js-sorted-set/issues/comments/152607461","html_url":"https://github.com/adamhooper/js-sorted-set/issues/4#issuecomment-152607461","issue_url":"https://api.github.com/repos/adamhooper/js-sorted-set/issues/4","id":152607461,"user":{"login":"adamhooper","id":10812,"avatar_url":"https://avatars.githubusercontent.com/u/10812?v=3","gravatar_id":"","url":"https://api.github.com/users/adamhooper","html_url":"https://github.com/adamhooper","followers_url":"https://api.github.com/users/adamhooper/followers","following_url":"https://api.github.com/users/adamhooper/following{/other_user}","gists_url":"https://api.github.com/users/adamhooper/gists{/gist_id}","starred_url":"https://api.github.com/users/adamhooper/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adamhooper/subscriptions","organizations_url":"https://api.github.com/users/adamhooper/orgs","repos_url":"https://api.github.com/users/adamhooper/repos","events_url":"https://api.github.com/users/adamhooper/events{/privacy}","received_events_url":"https://api.github.com/users/adamhooper/received_events","type":"User","site_admin":false},"created_at":"2015-10-30T18:18:05Z","updated_at":"2015-10-30T18:18:05Z","body":"This is by design.\r\n\r\nThere's a problem with making that error optional: there are _lots_ of ways to handle this case and they all make sense. When you try to add a element `a2` to a set that already contains `a1` which compares as its equal, which of the following should happen:\r\n\r\n1. `a1` is replaced by `a2`.\r\n2. `a2` is silently ignored.\r\n3. `a2` is silently ignored, iff `a1 === a2`; otherwise, we throw an exception.\r\n4. `a2` is added alongside `a1` iff `a1 !== a2`.\r\n5. `insert()` returns `false` and `a2` is ignored.\r\n6. we throw an exception.\r\n\r\nI hate the first four options because they encourage buggy code: the differences are so subtle that developers will assume they know what's happening when they actually don't. C++ STL chooses 2; Java, C# and Scala choose 5; JavaScript objects (if you're treating them as hashes/sets of Strings) choose 2; Ruby has undefined behavior, but the implementation seems to be 2; ditto Python.\r\n\r\nOption 2 is compelling, but it hinders some use cases. I originally used js-sorted-set to maintain a huge list of unique points; the exception helped me pinpoint where I was making mistakes. (In other words: after 10 `insert()`s, I expect `forEach()` to call its callback 10 times.) I could put a `contains()` call before each `insert()`, but that would give a performance hit; in contrast, the exception comes for free.\r\n\r\nOption 5 is compelling, but that makes for an unintuitive API. `insert()` shouldn't return anything, just by its name.\r\n\r\nSo I chose Option 6. Crashing during development is far better than embedding subtle edge cases in production.\r\n\r\nPerhaps we should add new convenience methods: `testAndInsert()` (5 above, inverted) and/or `insertUnlessContains()` (2 above).\r\n\r\nI welcome a pull request."}}