ElixirStudyGroup / assemble_the_minions

A playground for creating an army minions to do our bidding
2 stars 5 forks source link

Feature/timions-can-count #5

Closed timiyay closed 6 years ago

timiyay commented 8 years ago

Adds the basic arithmetic functions. Mostly straightforward, though still feeling tentative after one pass through the GenServer docs.

Had problems with the doctest statements I tried to add to my add, subtract, etc functions. If I tried to assert the console output like this:

iex> AssembleTheMinions.Minion.current_count(:stuart)
1
iex> AssembleTheMinions.Minion.count(:stuart, 2)
iex> AssembleTheMinions.Minion.current_count(:stuart)
3

It looked like the values were cumulative, meaning the above doctest for current_count was affecting the value of current_count(:stuart) - it wasn't resetting between doctests.

timiyay commented 8 years ago

Lots of repetition in my code, in the patterns to delegate to GenServer.handle_cast, and also in running:

new_state = Map.update(state, :count, n, &(&1 + n))
{:noreply, new_state}

Worth extracting that? It's still pretty compact, and the basic use case (delegating to + - / *) contributes to the repetition. Would probably be different for real-life codez

mfeckie commented 8 years ago

@timiyay

It looked like the values were cumulative, meaning the above doctest for current_count was affecting the value of current_count(:stuart) - it wasn't resetting between doctests

That's interesting, hey? Why do you think that would be?

mfeckie commented 8 years ago

@timiyay Tell me more about what you mean about repetition.

Worth extracting that?

What would be the benefit?

rant alert

I'm very very comfortable with repetition. The whole DRY mantra feels like it's gone too far in many places. Although you're repeating some of the 'words' the activities are different.

Map.update is the abstraction you're leaning on and the function that gets passed to it is the 'variable' part. That seems like a totally legitimate thing to do.

timiyay commented 8 years ago

@mfeckie agree

Have gone too far with DRY in the past, due to assumed moral obligation. Happier with self-contained methods where possible.

For this exact case - the actual code in this commit - I wouldn't see any use in extracting either.

Doctest thing will require some RTFD from me. I'm guessing that something (mix or exunit?) is running a separate test process with each doc test run sequentially? Or is the state for :stuart shared with the actual unit tests? I'm guessing no...

timiyay commented 8 years ago

By repetition, I meant that the code for the handle_cast methods were identical, except for 1 character (the arithmetic operator)

mfeckie commented 8 years ago

Yep, it's the 1 important character though!

In fairness it's possible to make it drier, but should you? I don't think it adds anything to A) readability and B) performance