ElixirStudyGroup / assemble_the_minions

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

Implement count basic arithmetic #1

Closed DuBistKomisch closed 6 years ago

DuBistKomisch commented 8 years ago

Since it's "counting", I assumed only non-negative integers arithmetic was valid.

Testing with catch_error (or assert_raise) doesn't seem to work when only putting the guard clauses on the handle_cast, since it's done async, so I put them on both the send and receive side to be safe.

I've only skimmed the GenServer docs, so I'm not sure if I'm doing anything horribly wrong, but it seems to work!

mfeckie commented 8 years ago

The catch error part, in my mind, is unnecessary. Otherwise, this is a solid first round. As per comments, limiting the input for operations may be overkill and prevent valid operations. Consider limiting to is_number vs integer.

knightstick commented 8 years ago

Since it's "counting", I assumed only non-negative integers arithmetic was valid.

You can count backwards... And in units other than ones...

knightstick commented 8 years ago

@mfeckie I'm interested in hearing your thoughts on "letting it crash" in this situation, I'm currently thinking that I'd rather get an "arithmetic error" if you try and add a string, rather than a match error

DuBistKomisch commented 8 years ago

Spec says:

Minions aren't very clever

;)

mfeckie commented 8 years ago

@knightstick My instinct would be to disallow invalid inputs, rather then handle them via error catching.

If you don't handle and try to call add(:stuart, "A") you'd see that the process crashes and restarts. In doing so, it's reset to it's beginning state, so we lose the current_count. How acceptable we find that loss of data is for us to decide. It's part of what drives us to consider how we manage state. If you consider the case where the process crashes outside of a match error, server goes down, network unreachable etc., what should we do then?

So we can't prevent crashes, now what? Well the design decisions we make define how we recover from crashes, so if the count is super important to us, then we should probably consider a more permanent form of storage for it, whether that be in a file or a database or wherever.

This is clearly a bit of a hand wavy, but long winded way of saying, you need to consider crash recovery behaviour and design for fault tolerance by minimising state that is unrecoverable (when it's important)

knightstick commented 8 years ago

So, your thinking is that the process that calls add(:stuart "A") should crash, on the whole, and that should prevent at least on category of Minion crashes?

I can get on board with that.

My thoughts previously were that the "Arithmetic" error for, say, adding a string to an integer, was more descriptive, and therefore more developer-friendly, than the "Match error" if we use guards on our calls, but, I guess in that case, it'd crash the Minion, not the caller...

mfeckie commented 8 years ago

Kind of, I mean it would be sensible to limit the interface function with a guard.

def add minion, number) when is_number(number) do

So you reduce the likelihood of error, but less so for the handler

DuBistKomisch commented 6 years ago

Closing so this doesn't show up on my PR list.