klawhorn / marketo-dedupe Goto Github PK

If I understand correctly, you're reading in the given json file, duplicating it to a file named sourceData.json, and then using that sourceData.json file to start the deduping. Can you do the deduping without creating the sourceData.json file at all? In other words, can you eliminate the duplicateSourceFile() step completely? You should be able to keep the json data in memory as an object and pass that to the deduping function, which is nice because it doesn't create a duplicate file and it won't modify the original file.

Excellent work writing tests! But one tiny thing is missing - your tests use mocha, but your package.json doesn't list mocha in devDependencies. So an npm install doesn't install it, and therefore the first time running npm run test will fail because mocha isn't installed. Go ahead and add it to devDependencies (rather than dependencies because it is only used during development)

Some minor suggestions for things I've noticed in your tests:

1. In tests/filter.js you have two tests. The second one should be renamed to something different than the first, because it's checking for something different. And what is the second test trying to do? As of now it's just checking to see that the resulting array has a length of more than 0, which is fine. But even more useful are tests like "the resulting array still has a length of 2" and "if there are two entries with the same id, the resulting array has a length of 1".

2. You have some tests where the goal is to verify that an array is returned, which is good. But what you actually do is verify that an object of type object is returned, it'd be good to update that to check that an object of type Array is returned, to be more specific. For example, if something was buggy and a string was returned the test would pass.

3. In your appendReason test for logging.js you describe a nice goal of the test for checking for the reasonForRemoval string, but the actual checks you're doing don't test for that.

Ah style...being consistent with whitespaces and indentations and variable naming capitalization/underscores, single quotes for strings, semicolons...it's something that's important partly because of OCD-ness, and partly because of issues that arise in unexpected ways. It's not even as important which style you choose, more important to be 100% consistent.

I highly recommend some automated help, check out JSHint and/or standard. These can be run as part of your tests, and you can also get Sublime Text plugins for them which will show you the style problems directly in Sublime Text.

Another tiny suggestion, you never have to write if (id===true), it's simpler and equivalent to write if (id)

When running the app from the command line you want to require a filename to be passed as an argument, but it's nice to have a more explicit error message when the user accidentally doesn't specify one. This is such a common case where users don't give the correct arguments, there are lots of ways in node to help deal with that, here's a good place to start:
http://stackoverflow.com/a/34782300/1459845

On line 14 of filter.js I think you're missing an underscore, shouldn't it be the same as line 22? Can't tell yet if this causes a bug, but definitely a good opportunity for writing a test for that.