gr2m / initials

extract initials from and add initials to names
https://gr2m.github.io/initials
MIT License
44 stars 14 forks source link

Avoid same initials for similar names #14

Closed gr2m closed 9 years ago

gr2m commented 9 years ago

I'd like to invite new contributors to this repository or open source in general to send a PR for this issue. Please let me know if you need any guidance, and if you want to give it a try, comment on the issue so others will now that it's taken.

You will need to install git & Node.js to work on this issue. Here are the steps you will need to set up the project locally, run the tests to see the problem, and fix it once you're done :)

  1. Fork this repository, see http://guides.github.com/activities/forking/ for instructions
  2. Clone it to your computer, see https://guides.github.com/activities/forking/#clone
  3. Open the folder of the repository in your terminal
  4. Run npm install
  5. uncomment this test: https://github.com/gr2m/initials/blob/906502d4730c65f3ae6f43a7105f8581d4c5388a/test/initials-test.js#L43
  6. Run npm test. You should see something like this: screen shot 2015-09-07 at 10 43 20
  7. Find a way to resolve the problem. Keep running npm test until all tests are green
  8. Push your changes and start a Pull Request, see http://guides.github.com/activities/forking
varjmes commented 9 years ago

Tweeting about this from @yourfirspr at 3PM (GMT), this is a great issue and exactly the kind of thing I want. Great balance between not giving the answer, but guiding you on your way.

👌

Ketouem commented 9 years ago

Just to get a bit of context, what is t.plan used for ?

gr2m commented 9 years ago

@Ketouem t.plan is part of https://www.npmjs.com/package/tape, see https://www.npmjs.com/package/tape#t-plan-n. You define the amount of tests (assertions), which is especially useful if you do async tests. Here we could remove t.plan and put t.end to the very end, it would have the same effect

sylvesterwillis commented 9 years ago

On line 177 of initials.js, there is a call to Array.splice inside of the loop which removes elements inside of the possibleInitials array causing the loop to skip some items. If we run the loop from end to beginning this issue is resolved, but the output of initials(['Moe Min', 'Moe Minutes']) becomes ["Moe Min", "MMinutes"] which is different than the expected value of the current test. Would you rather go with this approach and edit the test accordingly or keep tackling it to get an output that matches the current test?

gr2m commented 9 years ago

@sylvesterwillis good idea with running backwards! But Moe Min is not really helpful as initials for Moe Min, but maybe your changes is progress in the right direction? Wanna start a pull request, and we discuss there?

sylvesterwillis commented 9 years ago

Sure, I'll be on the train for a bit but will be able to talk more in about ~2hrs or so. My PR doesn't include any changes to the test since we'll be talking more about it.

gr2m commented 9 years ago

I think one way to approach the problem is that instead of ignoring initials that might be valid for both entirely, we could only allow them once.

For example, for simpler debugging, add only this test at the very end to the test/initials-test.js file:

test.only('debug', function (t) {
  t.deepEqual(initials(['Moe Minutes', 'Moe Min']), ['MoM', 'MMi'], '["Moe Minutes", "Moe Min"] ☛ ["MoM", "MMi"]')
})

then in initials.js, log initialsForNamesMap in line 170, before the // initialsForNamesMap comment, then run

node test/initials-test.js

The output will look something like this:

TAP version 13
# debug
{ 'Moe Minutes': [ 'MM' ], 'Moe Min': [ 'MM' ] }
{ 'Moe Minutes': [ 'MoM', 'MMi' ], 'Moe Min': [ 'MoM', 'MMi' ] }
{ 'Moe Minutes': [ 'MoMi', 'MoeM', 'MMin' ],
  'Moe Min': [ 'MoeM', 'MMin', 'MoMi' ] }
{ 'Moe Minutes': [ 'MMinu', 'MoeMi', 'MoMin' ],
  'Moe Min': [ 'MoMin', 'MoeMi' ] }
{ 'Moe Minutes': [ 'MoMinu', 'MoeMin', 'MMinut' ],
  'Moe Min': [ 'MoeMin' ] }
{ 'Moe Minutes': [ 'MoeMinu', 'MMinute', 'MoMinut' ],
  'Moe Min': [] }
{ 'Moe Minutes': [ 'MMinutes', 'MoeMinut', 'MoMinute' ],
  'Moe Min': [ 'Moe Min' ] }
not ok 1 ["Moe Minutes", "Moe Min"] ☛ ["MoM", "MMi"]

If we'd check for the amount of possible different initials, we could end with

{ 'Moe Minutes': [ 'MoM', 'MMi' ], 'Moe Min': [ 'MoM', 'MMi' ] }

And give Moe Minutes the initials MoM, and Moe Min the initials MMi. I guess for that we would need to add an extra check before these lines

I think there are different ways to achieve that, it's quite a puzzle though. I'm looking forward to see what we'll come up with

gr2m commented 9 years ago

resolved via https://github.com/gr2m/initials/pull/16, thanks @javimolla & @Charlotteis :v: