Last active
August 29, 2015 14:15
-
-
Save jamesladd/09741a50a79f293ff21b to your computer and use it in GitHub Desktop.
Original ansi.rb requiring more than 12 test to prove to_ansi method works as expected vs new version requiring 3 tests to prove to_ansi method works as expected.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# - ORIGINAL - Requires 12 test cases the provide to_ansi does what it should | |
# 1. Test empty commands map returns "" | |
# 2. 11 Tests to test each if/elsif. | |
# | |
class Clear; end | |
class HideCursor; end | |
class ShowCursor; end | |
class SetPos < Value.new(:row, :column); end | |
class FG < Value.new(:red, :green, :blue); end | |
class BG < Value.new(:red, :green, :blue); end | |
class Default; end | |
class Inverse; end | |
class Reset; end | |
def to_ansi(commands) | |
commands.map do |command| | |
if command == Clear then escape "2J" | |
elsif command == HideCursor then escape "?25l" | |
elsif command == ShowCursor then escape "?25h" | |
elsif command == SetPos then escape "#{line + 1};#{column + 1}H" | |
elsif command.is_a? FG then foreground(command) | |
elsif command.is_a? BG then background(command) | |
elsif command == Default then escape "39?49m" | |
elsif command == Inverse then escape("7m") | |
elsif command == Reset then escape("0m") | |
elsif command.is_a? String then command | |
else raise RuntimeError.new("BUG: didn't understand ANSI command #{command.inspect}") | |
end | |
end.join("") | |
end | |
# - NEW - Requires 3 test cases to prove to_ansi does what it should. | |
# 1. Test empty commands map returns "" | |
# 2. Test command is handled (ie: mock handler etc) | |
# 3. Test command is not handled (ie: RuntimeError is raised) | |
# | |
# handlers - a hash of Handler instances with a default handler | |
# for missing keys to handle case where command is not recognized. | |
# (Raises RuntimeError.) | |
# | |
# Personally I'd break the 'handlers' line out into a method. | |
# | |
# type_of is a helper to determine the class of the command or in | |
# the case of a String instance return String. Maybe there is a method | |
# elsewhere to help w this. | |
# | |
handlers = Hash.new default_handler | |
handlers[Clear] = ClearHandler.new | |
# add other initialisations for other handlers here. | |
handlers[Reset] = ResetHandler.new | |
def to_ansi(commands) | |
commands.map do |command| | |
handlers[type_of(command)].execute command | |
end.join("") | |
end |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I assume that your 12 estimated tests for my version include a test for
to_ansi([])
, plus each of the 11if
branches. Your proposed version contains none of that code, and therefore none of those tests. If you actually write the classes for each of the 11 ANSI commands, all 11 of those little expressions (escape "2J"
,escape "?25l"
, etc.) will be reintroduced and will need to be tested.You now have 11 handler tests plus the three tests that you propose for
to_ansi
itself, making 14. The ANSI translation rules will be spread across a minimum of six times the vertical space of myto_ansi
function, since each single-lineif
/elsif
becomes a newline plus aclass
line, adef
line, the trivial body, and twoend
s. You also have to define thetype_of
method, which didn't exist before.If you actually write this code out, you'll find that it's six or seven times the length of mine, has an identical external interface, explodes the logic such that it doesn't fit on a laptop screen at once, and requires at least as many tests to fully cover all 11 cases (and even more tests than my code if you include your extra 3 tests).
There are 11 independent paths. They cannot be merged. (OK, unless you construct a
commands
array that includes multiple commands that are covered in one test case, but that applies equally to either solution). Simply moving the code to other classes doesn't reduce the number of test cases.I've suggested multiple times that you actually write the full code and answer, for yourself, how to cover it in fewer than 11 tests. That's still my suggestion. Write all of those little handler classes out and ask yourself how
escape "2J"
,escape "?25l"
, etc. get covered. If your answer is "I don't need to cover them because they're basically constants", then you've discovered the reason that I didn't write tests for this code.