10
\$\begingroup\$

I recently wrote a name generator that uses a DTMC underneath (I asked about it here) and, since I'm not entirely confident I did it right, I wrote a script to check my code, or at least its output.

It works pretty well, but, being new to the language, I want to know how to make it more idiomatic. Performance boosts (in terms of speed and memory efficiency) would also be a plus, but since this is just a simple test script they're not as important.

# arguments: '-dDELIMITER'
if ARGV[0] == '-h'
    [
            'Should be used in the form:',
            '<invocation of name_gen.rb> | <ruby> name_gen_test.rb -d<delimiter>',
            'The delimiter MUST be specified in name_gen.rb and it MUST NOT be ``.'
    ].each { |line| puts line }
end

DELIMITER = ARGV[0] || abort('You must specify a delimiter as the sole command-line argument')

connections = Hash.new { |hash, key| hash[key] = Hash.new 0 }
start = Hash.new 0

until (cur_line = STDIN.gets).nil?
    cur_line.chomp!
    individual_syllables = cur_line.split DELIMITER
    individual_syllables.each_with_index { |from, index|
        start[from] += 1 if index == 0
        connections[from][individual_syllables[index + 1] || !!false] += 1
    }
end

# % of start per syllable
# % of connections to each syllable it connected to

puts 'Start:'
total_start_count = start.values.inject(:+).to_f
max_len = start.keys.inject (0) { |memo, cur|
    (cur.length > memo) ? cur.length : memo
}
start.each { |text, percent|
    puts "  #{text.ljust max_len} : #{(percent * 100 / total_start_count).round.to_i}%"
    # Get the percent -> Truncate -> convert to string -> justify
}
puts

END_MARKER = '[end]'
puts 'Connections:'
connections.each { |from, links|
    total_connection_count = links.values.inject(:+).to_f
    max_len = links.keys.inject(END_MARKER.length) { |memo, cur|
        ((cur ? cur : '').length > memo) ? cur.length : memo
    }
    puts "  #{from}:"
    links.each { |to, probability|
        next unless to
        puts "    #{to.ljust max_len} : #{(probability * 100 / total_connection_count).round.to_i}%"
    }
    puts "    #{END_MARKER} : #{(links[false] * 100 / total_connection_count).round.to_i}%"
}

This is the name generator; this script is meant to be used something like this (on Windows, at least):

ruby name_gen.rb dict.txt 10000 -d_ | ruby name_gen_test.rb _

if the dictionary of syllables is located at 'dict.txt'.

Here's an example dictionary file:

a|1|1|b,2;c,2
b|0|3|a,0;c,2
c|0|0|a,1;b,1

And an example output for the script:

Start:
  a : 100%

Connections:
  a:
    c     : 40%
    b     : 40%
    [end] : 20%
  c:
    a     : 50%
    b     : 50%
    [end] : 0%
  b:
    c     : 40%
    [end] : 60%

For anyone interested, the final code is available here.

\$\endgroup\$
6
\$\begingroup\$
  1. The ruby style guide suggests to use 2 spaces per indentation level.
  2. You can use heredocs for multi-line strings, keep in mind that they preserve white space, here is a nice trick that could be used:

    help = <<-END.gsub(/^\s+\|/, '')
      |Should be used in the form:,
      |<invocation of name_gen.rb> | <ruby> name_gen_test.rb -d<delimiter>,
      |The delimiter MUST be specified in name_gen.rb and it MUST NOT be ``.
              END
    help.each_line { |line| puts line }
    
  3. for multiline blocks please use do...end instead of {...}:

    individual_syllables.each_with_index do |from, index|
    start[from] += 1 if index == 0
    connections[from][individual_syllables[index + 1] || !!false] += 
    end
    
  4. I don't understand why you use !!false, since !!false == false. Also I don't think it is needed here.

  5. Do not put a space between a method name and the opening parenthesis:

    max_len = start.keys.inject (0)
    
  6. Avoid nested ternary operators, it makes the code hard to understand.

    ((cur ? cur : '').length > memo) ? cur.length : memo
    # becomes
    cur ||= ''
    [cur.length, memo].max
    
\$\endgroup\$
  • \$\begingroup\$ I used !!false because RubyMine kept giving me a warning if I tried to use just !!false. The four spaces are actually tabs that I converted to the default so that StackOverflow wouldn't complain. For #6, would [(cur || '').length, memo].max also be fine? I'd prefer to keep it to one line if possible. Aside from that, once I get a chance, I'll apply these changes, test a bit, and accept this answer. Thanks! \$\endgroup\$ – Nic Hartley May 26 '15 at 14:33
  • 1
    \$\begingroup\$ Er, typo: It gave me a warning if I tried to use just false. Looking back at it, I'm not sure it's necessary; I could just change links[false] to links[nil] and that whole bit goes away. \$\endgroup\$ – Nic Hartley May 26 '15 at 14:50
  • \$\begingroup\$ bbatsov's guide sucks \$\endgroup\$ – Nakilon May 26 '15 at 17:56
  • 1
    \$\begingroup\$ @Nakilon actually it's a comunity driven guide. I don't see why someone would dislike such style. \$\endgroup\$ – Mhmd May 26 '15 at 17:59
  • \$\begingroup\$ @Mhmd, community was enough careful to not make any (potentinally bad) style propaganda, until the least careful kid made his awful snippet of bad advises. Now community have to spend years on making it less awful -- this correction produces so awesomely pointless rules like: "Q: do I write P or R? A: write P, or R." \$\endgroup\$ – Nakilon May 26 '15 at 18:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.