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

Emails, emails everywhere

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

Problem

My job entails me to send a lot of emails. I'm really bad at typing so I decided to create a program to do it for me.

I've created an email generation tool (originally posted here.) I've changed everything around so that I was able to push the tool to Github, so now you can actually run it, and fall in love with it's magic and wonder. The entire run able program can be found here. To run this program you need to do the following:

  • Download the zip (only works on Windows)



  • Extract the zip file to your desktop



  • Create a directory called email



  • Move all the files from email-tool-4 to the email directory



  • cd into that directory and run the program



The test module is where the issues lay, so what I'm looking for is the following:

  • Is there any way to make test runs more object-oriented?



  • How could I improve speed? This program is pretty slow (mostly because it's Ruby), but is there a way I can make it a little quicker, such as with threading?



  • Can I copy to the clipboard with Ruby instead of using a third party tool?



Main source file:

```
#!/usr/local/bin/env ruby

# Require the files that requires all the other files and the initialize the constants

require_relative '../email/lib/lists/require/require'
require_relative '../email/lib/lists/require/constants'

# Optparse to append the flags into the OPTIONS constant

OptionParser.new do |opts|
opts.on('--tutorial', 'First time with the program..? Run this and find out how to use it') { |o| OPTIONS[:tutorial] = o }
opts.on('--help', 'Generate the help page') { |o| OPTIONS[:help] = o }
opts.on('-t INPUT[=INPUT]', '--type INPUT[=INPUT]', 'Specify the type of email to be generated') { |o| OPTIONS[:type] = o }
opts.on('--example', 'Gives an example of a generic email that was created using this program') { |o| OPTIONS[:example] = o }
opts.on('--version', 'Displays the version number of the program') { |o| OPTIONS[:version] = o }
opts.on('--test', 'Run the test modules.') { |o| OPTIONS[:test

Solution

Avoiding raiseing just to rescue immediately

def verify_digits(int)
  begin
    if !(int[/^\d{7}$/])
      raise TicketNumError
    else
      int
    end
  rescue TicketNumError
    ''
  end
end


You raise TicketNumError just to rescue it a few lines later, just avoid raising it for simplicity:

def verify_digits(int)
  (int[/^\d{7}$/]) ? int : ''
end


Functionality of this new un-bloated version should be identical to the original version. (I swapped the condition putting the positive first because it felt more intuitive)

Native methods

Using native ruby methods is a bit less concise but more readable:

DIGITS = "1234567890"
(int.length == 7 && int.all {|x| DIGITS.include?(x)}) ? ...


Boolean test function

In fact a better design would be an is_valid_ticket? function that just returns true or false because return an int if valid and an error string if invalid looks really weird / not easily re-usable:

def is_valid_ticket?(ticket)
  ticket.length == 7 && ticket.all {|x| DIGITS.include?(x)}
end

Code Snippets

def verify_digits(int)
  begin
    if !(int[/^\d{7}$/])
      raise TicketNumError
    else
      int
    end
  rescue TicketNumError
    '<Invalid ticket number>'
  end
end
def verify_digits(int)
  (int[/^\d{7}$/]) ? int : '<Invalid ticket number>'
end
DIGITS = "1234567890"
(int.length == 7 && int.all {|x| DIGITS.include?(x)}) ? ...
def is_valid_ticket?(ticket)
  ticket.length == 7 && ticket.all {|x| DIGITS.include?(x)}
end

Context

StackExchange Code Review Q#137995, answer score: 4

Revisions (0)

No revisions yet.