search

nieldlr / hanzi

HanziJS is a Chinese character and NLP module for Chinese language processing for Node.js
http://hanzijs.com
MIT License
376 stars 56 forks source link

Refactor some functions in lib/dictionary.js #23

Closed jsoendermann closed 7 years ago

jsoendermann commented 10 years ago

Hi, I refactored some stuff, have a look at it and merge if you like :) The tests still pass and test.js produces the same result as before, but I might have misunderstood some parts of your code, so better check all the changes. Very cool project, by the way, thanks for writing this!

nieldlr commented 9 years ago

Hi @jsoendermann!

Wow, I'm so incredibly sorry for only getting around to this now. It's been in the back of my mind for a while and the urge hit me today to finally dig into this. :)

When I started looking at your PR I realized that I couldn't quite remember if some of your changes might break some things. So I spent some time writing a stronger test suite for dictionary.js' methods. Thanks so much for this. I realized with a library like this we need stronger testing to make sure any core changes will work out smoothly.

After writing the tests, I pulled down your PR and almost all of them passed. Except for one test, which failed because of this commit: https://github.com/jsoendermann/Hanzi/commit/2556e6a0637282c7512970e2bb800f0975af79d3

The test that failed was forcing a script type on the definitionLookup: 

it('should fail looking up a traditional definition with a simplified character', function(){
    assert.deepEqual(hanzi.definitionLookup('爱', 't'), undefined);
});

In your change it still returns the definition, where it should be undefined. :)

I love all your other changes! Definitely made me scratch my head this morning looking at my old code. Yours feels so much cleaner. I'd love to pull this in. Would you perhaps be up trying to fix the definitionLookup function, and then I can pull this in? :)

I can also definitely take a stab at changing that one if you're bit stuck for time. I know it's been a while as well, so the code might not be so fresh in your mind.

jsoendermann commented 9 years ago

Hi @nieldlr, Hanzi.js is a very cool project that I'm happy to contribute to! I added another commit that hopefully fixes this issue.

nieldlr commented 9 years ago

Hi @jsoendermann, sorry for the delay here!

I tried pulling in the changes, but it seems that it is broken on my side? It gives me an error:

/Users/nieldlr/Coding/Projects/Hanzi/lib/dictionary.js:69
    } else if (scripttype === "t") {
      ^^^^
SyntaxError: Unexpected token else

Not sure if I'm doing something wrong on my side? :) After digging in a bit more I came to a refactor of something like this that passed all the tests:

    if (!scripttype) {
        if (determineIfSimplified(word)) {
            return dictionarysimplified[word];
        } else {
            return dictionarytraditional[word];
        }
    }

    if (scripttype === "s") return dictionarysimplified[word];

    if (scripttype === "t") return dictionarytraditional[word];

    return undefined;

Would love to hear what you think of that? Does that seem good to you?

Also, I just realized this after I pulled down another PR. I think we're settling on tabs (size 4) as our spacing option. Would you perhaps be up for switching the indentation back to this option? Terribly sorry for only noticing this now & throwing you about here.

Keen to see PR in the module :D

jsoendermann commented 9 years ago

Hi @nieldlr, Sorry about the error, I wrote the code in a hurry and missed a {. Your version is much nicer anyway! I'll look at the indentation as soon as I have some free time.