patternrubyModerate
Automatic flowchart generator
Viewed 0 times
automaticflowchartgenerator
Problem
I wanted to see graphically what my programme was doing so I wrote this automatic flowchart generator:
Some testcases:
Generates the flowchart:
```
class String
def alpha_value
self .log("Start")
.chars .log("Split char by char")
.map(&:alphabet_position) .log("Map by position in the alphabet")
class Object
def log(previous_method)
if not DEBUGGING
return
end
repr = if self.class == Enumerator then self.to_a else self end
if [Array, String].include?(repr.class) and repr.length > 100
repr = repr.first(20) + [". . ."] + repr.last(20)
end
if previous_method.downcase == 'start'
self.tap {|x| puts """
#{repr}
"""}
else
self.tap {|x| puts """
|
| #{previous_method}
|
V
#{repr}
"""}
end
end
endSome testcases:
class Integer
def is_digits_pow_sum?(exp)
self .log("start")
.digits .log("Digits")
.map{|x| x**exp} .log("Each digit to the power of #{exp}")
.sum .log("Sum")
.equal?(self) .log("Equals the start? (#{self})")
end
end
441.is_digits_pow_sum?(3)Generates the flowchart:
441
|
| Digits
|
V
[4, 4, 1]
|
| Each digit to the power of 3
|
V
[64, 64, 1]
|
| Sum
|
V
129
|
| Equals the start? (441)
|
V
false```
class String
def alpha_value
self .log("Start")
.chars .log("Split char by char")
.map(&:alphabet_position) .log("Map by position in the alphabet")
Solution
You're a Python native, aren't you? I can tell from the
Each tip assumes that you've already applied the last.
Also, I'm writing this review from the perspective of Ruby 2.2.2 -- some methods that existed when you wrote this don't exist any more, and some methods that do what you want better have been created. I suggest looking through the Ruby docs for the updated methods, since there's no way I caught everything.
-
Ruby indents are two spaces, not four.
-
You may have noticed a bug -- even though you seem to be specifying a single multiline string, it's printing out a bunch of newlines between the beginning and the end. That's because
To fix both issues, you can use a heredoc:
-
In multiline blocks, use
-
Actually, why are you using
-
In Ruby, we have the ternary operator (
-
But even that's unnecessary -- firstly,
Of course, this is a bad idea. What happens when someone tries to use your
Since, after all, you'll be printing it, and it's far better to let the designers of an object decide how it ought to be printed, rather than trying to wrest control from the developer using the library.
-
You know,
-
What is this?
You already forced
For more information about
-
-
What happens when someone wants to log a method called
and
One bonus of this (combined with #8) is that you can totally get rid of
-
Now, there are a couple of other things: Very rarely do people want to log stuff to the console (i.e. where
""" multiline strings and the four-space indents and really unidiomatic whitespacing and if/then/else/end instead of ternary and if not instead of unless and use of blocks instead of statements and wow this is really unidiomatic ;-;Each tip assumes that you've already applied the last.
Also, I'm writing this review from the perspective of Ruby 2.2.2 -- some methods that existed when you wrote this don't exist any more, and some methods that do what you want better have been created. I suggest looking through the Ruby docs for the updated methods, since there's no way I caught everything.
-
Ruby indents are two spaces, not four.
-
You may have noticed a bug -- even though you seem to be specifying a single multiline string, it's printing out a bunch of newlines between the beginning and the end. That's because
""" [stuff] """ is interpreted not as a single string but as three: "", " [stuff] ", and "". Then, it outputs each of those on a new line, which results in a bunch of extra spacing. It also doesn't eliminate the indentation, so it ends up being moved really far to the right, too.To fix both issues, you can use a heredoc:
else
self.tap {|x| puts <<-END.gsub(/^\s*\||#@.*$/, '')
|
| |
| | #{previous_method}
| |
| V
|
| #{repr}
END}
end-
In multiline blocks, use
do/end instead of {/}:else
self.tap do |x|
puts <<-END.gsub(/^\s*\||#@.*$/, '')
|
| |
| | #{previous_method}
| |
| V
|
| #{repr}
END
end
end-
Actually, why are you using
self.tap at all? Just do the puts, then return self after:else
puts <<-END.gsub(/^\s*\||#@.*$/, '')
|
| |
| | #{previous_method}
| |
| V
|
| #{repr}
END
end
self-
In Ruby, we have the ternary operator (
condition ? a : b). Use it instead of the one-line if/then/else/end construction:repr = (self.class == Enumerator) ? self.to_a : self-
But even that's unnecessary -- firstly,
to_a returns an Array, not an Enumerable. Secondly, calling to_a on an Array will return the unmodified array, so you can just write:repr = self.to_aOf course, this is a bad idea. What happens when someone tries to use your
.log with an IO? Should the entire contents of the IO be put into an array? That seems really stupid to me. Personally, I would instead haverepr = self.to_sSince, after all, you'll be printing it, and it's far better to let the designers of an object decide how it ought to be printed, rather than trying to wrest control from the developer using the library.
-
You know,
repr is really a crappy name. What does it mean? You can feel free to have long variable names in Ruby; just make sure they're in snake_case.-
What is this?
if [Array, String].include?(repr.class) and repr.length > 100
repr = repr.first(20) + [". . ."] + repr.last(20)
endYou already forced
repr to be a String (and in your code, it was already an Array). Why are you checking that again? Plus, if it's a String, then it'll fail when you try to call the nonexistent first on it! Besides, Array and String aren't the only classes with a .length method. This is how I'd do it (keeping in mind that in #6 I converted it to a String):repr = repr.length > 100 ? "#{repr[0..20]}. . .#{repr[-20..-1]}" : reprFor more information about
String#[], see the docs. I'd also recommend pulling this out into a method -- the method name is up to you, but I'd personally call it shorten or trim_extra_chars or something like that.-
previous_method really isn't a very good name. It's not necessarily method that came before, after all -- it's an operation that led to the current state. It could well have been something that isn't a method! I'd call it previous_operation.-
What happens when someone wants to log a method called
start? For example, say they have an object representing a task that's built over several chained method calls, then started with start, which returns the result of the task. They'll try to .log('start'), but it'll instead give them this weird thing that they didn't expect! I'd recommend splitting it into two methods -- then, instead of an if/else and a default value, you can have:def log_start
#etc.
endand
def log(previous_operation)
#etc.
endOne bonus of this (combined with #8) is that you can totally get rid of
repr.-
Now, there are a couple of other things: Very rarely do people want to log stuff to the console (i.e. where
puts goes). I'd recommend adding support for outputting to other streams, though there are a couple of ways to do it. One of themCode Snippets
else
self.tap {|x| puts <<-END.gsub(/^\s*\||#@.*$/, '')
|
| |
| | #{previous_method}
| |
| V
|
| #{repr}
END}
endelse
self.tap do |x|
puts <<-END.gsub(/^\s*\||#@.*$/, '')
|
| |
| | #{previous_method}
| |
| V
|
| #{repr}
END
end
endelse
puts <<-END.gsub(/^\s*\||#@.*$/, '')
|
| |
| | #{previous_method}
| |
| V
|
| #{repr}
END
end
selfrepr = (self.class == Enumerator) ? self.to_a : selfrepr = self.to_aContext
StackExchange Code Review Q#86012, answer score: 11
Revisions (0)
No revisions yet.