HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyModerate

Automatic flowchart generator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
automaticflowchartgenerator

Problem

I wanted to see graphically what my programme was doing so I wrote this automatic flowchart generator:

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
end


Some 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 """ 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_a


Of 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 have

repr = self.to_s


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, 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)
end


You 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]}" : repr


For 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.
end


and

def log(previous_operation)
    #etc.
end


One 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 them

Code Snippets

else
  self.tap {|x| puts <<-END.gsub(/^\s*\||#@.*$/, '')
                  |
                  |    |
                  |    |    #{previous_method}
                  |    |
                  |    V
                  |
                  | #{repr}
                END}
end
else
  self.tap do |x|
    puts <<-END.gsub(/^\s*\||#@.*$/, '')
      |
      |    |
      |    |    #{previous_method}
      |    |
      |    V
      |
      | #{repr}
    END
  end
end
else
  puts <<-END.gsub(/^\s*\||#@.*$/, '')
    |
    |    |
    |    |    #{previous_method}
    |    |
    |    V
    |
    | #{repr}
  END
end
self
repr = (self.class == Enumerator) ? self.to_a : self
repr = self.to_a

Context

StackExchange Code Review Q#86012, answer score: 11

Revisions (0)

No revisions yet.