search

esbtools / event-handler

Notification in, document out.
GNU General Public License v3.0
3 stars 6 forks source link

Also check if new event supersedes existing event (fixes #24) #25

Closed alechenninger closed 8 years ago

kahowell commented 8 years ago

LGTM. Merge when ready.

Unrelated: this isn't pressing but I think the logic in LightblueDocumentEventRepository (and maybe other places) is getting complex enough that it might help to refactor some of it into smaller methods for clarity. This is something I'd like to see us look as time permits (outside the scope of this PR).

Example: the logic underneath if (previouslyOptimizedEvent.isSupersededBy(newOrMergerEvent)) could be extracted into a method: supersedePreviousEntity. I think to change it now would be a mistake though, as it'd make the code a little inconsistent... My 2 cents.

alechenninger commented 8 years ago

Yeah I could definitely see something like that being useful as private methods in that type.

I go back and forth though on whether inlining is better for locality (not having to jump around code to see the details), or extracting methods is better for "glanceability" (being able to see what code does at a high level quickly). The question is, does "supersedePreviousEntity" really say what the "code does" enough? I can go either way, I'd probably need an unbiased perspective to make that call.

Thanks for the quick review!