Skip to content

Instantly share code, notes, and snippets.

@JoshCheek
Created April 11, 2016 04:04
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save JoshCheek/7fee6a79eaecb353ed352ef947416be4 to your computer and use it in GitHub Desktop.
Save JoshCheek/7fee6a79eaecb353ed352ef947416be4 to your computer and use it in GitHub Desktop.
Some refactorings from a code review.

You've already written several tests https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/test/run_test.js, it's probably worth dropping them in a real test suite and adding some assertions so that it doesn't require interpreting the output.

https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L28 you can prob do the new syntax like ...props so that you can use multiple properties when selecting

The string interpolation on the id is probably not necessary. https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L33 ie whereTable[ids[i]] probably works.

https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L31-L32 seems to be a select and a map, looks like lodash has a "filter" method for that https://lodash.com/docs#filter and a map method https://lodash.com/docs#map

Actually, the map is only necessary b/c you want to work with the values instead of the ids, so ask for them instead of keys https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L30 lodash has a values method https://lodash.com/docs#values or, looks like it's part of JS https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/values so you can maybe get it using babel or something. Then that bit is just a select.

Guard clause here would shorten and flatten the method: https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L41

This interpolation isn't necessary https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L52

This one can delegate to writeToTable https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L43

So can this one https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L53

This one can delegate to getTable https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L24

So can this one https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L58

And this one https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L70-L71

Don't need interpolation here: https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L72

I'd probably put this at a different key, maybe "metadata" or something, b/c otherwise it looks like an entry within the table https://github.com/selfup/rejs/blob/7def485717b2adf52ad7662857d8eada828fd4b3/re.js#L52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment