search

smondet / trakeva

Transactions, Keys, and Values
http://www.hammerlab.org/docs/trakeva/master/index.html
24 stars 3 forks source link

Improve benchmarks #12

Closed smondet closed 9 years ago

smondet commented 9 years ago

Review on Reviewable

rleonid commented 9 years ago

Seems fine.

  1. At 650 lines of code, I would have separated main.ml before adding new logic.
  2. Writing your own Test framework makes it harder to grok. Took me a while to get used to check being an named assert.
  3. I'm not certain that this code needs to be so monadic since about half of your binds are just returning unit.
  4. debug_mode is always commented out, so what's the point of defining it?
smondet commented 9 years ago
  1. At 650 lines of code, I would have separated main.ml before adding new logic.

OK will split: https://github.com/smondet/trakeva/issues/14

  1. Writing your own Test framework makes it harder to grok. Took me a while to get used to check being an named assert.

That's a far more general problem :)

Finding a test frameworkd that doesn't suck more than 30 lines hacked together; I haven't (in any language BTW).

The word assert has a very specific meaning in ocaml (evan emacs colors it differently). Test.check seems OK for now and stresses the fact that it's just a function (no compiler / syntax support).

  1. I'm not certain that this code needs to be so monadic since about half of your binds are just returning unit.

Those functions return (unit, 'some-error-type) Result.t Lwt.t; that's much more than unit and we cannot escape it (that's how Lwt itself works).

  1. debug_mode is always commented out, so what's the point of defining it?

Well, for debugging :) (I've used them a lot lately)