Please critique my code

Dev Zone Code
1

In my quest to automate my email - I've started small. I work as a Jr. SysAdmin and people frequently email me things that really should have been sent to our ticket queue. At this point I'm so lazy I'm tired of forwarding the emails and replying that I'm sending it to our queue.

Therefore, I've written (stole) the following ruby code - which allows me to drap (drag + drop) an email into a folder in my inbox - have the ruby script run on a cron and ship it out. So far I'm able talk to my inbox get to the "send to ticket queue" folder, read all emails extract all the info I need to construct my forward and response. Then I can fire off the mail successfully.

What I'd like is constructive criticism. For example, I really don't like lines 36 and 37 where I have two return statements returning arrays. Also, I'd like to better iterate through my "sendEmail" while loop. Something better than using the "bodyArray" length.

I'm going to add to this script to move the emails into another folder "sent to ticket queue" after this scripts does it's thing.

Anyway, be brutal if you want - just help me make it better.

Thanks!

  1 #!/bin/ruby
  2 
  3 require 'net/imap'
  4 require 'net/smtp'
  5 
  6 
  7 USERNAME = "myUser"
  8 PASSWORD = "neverGonnaGetIt"
  9 
 10 bodyArray = []
 11 nameArray = []
 12 
 13 
 14 #scans emails in "Inbox/.toRT" and builds arrays
 15 def getmsgs(username, password, emailBodyArray, emailNameArray)
 16         at="@"
 17         imap = Net::IMAP.new("imap.myOrg.com",993,true)
 18         imap.login(username, password)
 19         imap.select("Inbox/.toRT")
 20         imap.search(["ALL"]).each do |msg|
 21                 envelope = imap.fetch(msg, "ENVELOPE")[0].attr["ENVELOPE"]
 22                 sender = envelope.from[0].mailbox
 23                 host = envelope.from[0].host
 24                 emailaddress = sender + at + host
 25                 body = imap.fetch(msg,'BODY[TEXT]')[0].attr['BODY[TEXT]']
 26 
 27                 emailBodyArray.push(body)
 28                 emailNameArray.push(emailaddress)
 29 
 30 #               puts body
 31 #               puts sender
 32         end
 33 
 34         imap.logout
 35         imap.disconnect
 36         return emailBodyArray
 37         return emailNameArray
 38 end
 39 
 40 def sendEmail(username, password, bodyToSendArray, nameToSendArray)
 41         smtp = Net::SMTP.new 'smtp.myOrg.com',587
 42         smtp.enable_starttls
 43         smtp.start('myOrg.com',username, password, :login)
 44         $i = 0
 45         $num = bodyToSendArray.length
 46                 while $i < $num do
 47                         msg = "Subject: Hello \n\n requestor: #{nameToSendArray[$i]} \n\n Thank you for your message.  I have forwarded your request to our ticketing system ([email protected]).  This will ensure the fastest response     possible. \n\n Orignal Message: \n #{bodyToSendArray[$i]}"
 48                 smtp.send_message(msg, '[email protected]', [nameToSendArray[$i], '[email protected]'])
 49                         $i +=1
 50                 end
 51 end
 52 
 53 
 54 getmsgs(USERNAME,PASSWORD,bodyArray, nameArray)
 55 sendEmail(USERNAME,PASSWORD,bodyArray, nameArray)
~
2 Likes
2

I'd start off with putting some logic into separate functions and giving them a descriptive name. It's going to help with building on top of this later down the line and it's easier to read for other programmers and yourself later on. For instance line 22 to 24 as getEmail(envelope) to give an example. As you split things up, the problem of the 2 return statements is probably going to sort itself out.

I haven't worked with Ruby yet so I can't really help with language specific stuff which could be improved. If Ruby has a for-loop you might be able to shorten lines 44 through 50 with that.

2 Likes
3

I'm not a ruby person either, but the thing that jumps out at me is that OP is referencing a global array, bodyArray and nameArray. This isn't bad, per se, but it's just not the best practice. The problem here is that when you get into more advanced code with multiple threads, you can get two methods trying to manipulate the array at the same time which can cause segaults and other problems. Global variables should be avoided whenever possible.

My recommendation would be in def getmsgs, drop the body and name arrays from the arguments list, create them locally and return them at the end of the method. To return multiple objects in ruby, (I'm pretty sure, I'm rusty with this) you just do return item1, item2 and then in the line where you call the method, you do item1, item2 = method(arg1,arg2,argetc...) Then you can pass the return values to a later method.

Also, if you could, please don't include line numbers in your code. It's easier for us to import and run on our own machines if we don't have to use awk to pull out the line numbers at the beginning of the code. Also, we can just quote your code if we want to reference it.

EDIT: On your two return statements, you're actually only using one of them. A return directive tells the software to exit the method, so the second one will never be reached.

1 Like
4

Thanks @timothee and @SgtAwesomesauce!

I understand not everyone uses ruby, but just the general "programming best pratices" (globals, switching the while > for loop, etc) is really what I was hoping for. So thanks for your insights!

I'll see how I can implement those suggestions, but one thing I was considering was creating a struct called email and having it contain two arrays and just return the struct. I haven't worked with structs in much depth, but I was thinking that would be a way to create one data type which contains multiple datatype and only return once, since apparently I'm already only doing that.

Thanks again for the feedback! I'm telling you, it may take a few years, but I'll get most of my email automated.

I'll update the code when I get a few free minutes.

1 Like
5

By struct are you talking about a named array? something like: 

{
    "address": addressList,
    "body": bodyList
}

I'm not too familiar with the Ruby lingo, If you were planning on doing something like this, it's not a bad idea, it keeps everything organized, but it may be overkill for this implementation.

More than happy to help people learn!

6

You stepped from bash to ruby! i like!

7

I had the same problem stripping the line numbers out. I'm not really proud of this regex, but I was able to copy the code into gedit and then run this and it gave me the file formatted:

sed -i 's/^\s..\s//g' email.rb

Pasting it into vi did not work out so well...

But I can see the pain of line numbers, if you're not on a unix box.

PS refactoring this code - will post shortly.

1 Like
8

Awesome. Let's see how it turns out.

9

I added the struct to the "getmsg" function and return it (Only one return statement). Deleted global array variables. Call functions at the bottom, by first setting a variable equal to the getMsg and pass that variable into the sendEmail function.

This is probably really crappy ruby code, but I'm not a rubist yet. However, it works and in all likelihood - probably borderline OK for a first ruby script. I'd still like critiques on how to make it better.

@SgtAwesomesauce I think that a struct and named array or possibly, more so, JSON object are very closely related in concept. 

#!/bin/ruby

require 'net/imap'
require 'net/smtp'


USERNAME = "NOPE"
PASSWORD = "NOPE"

#Opens IMAP connection to email account
#scans emails in "Inbox/.toRT" for emails
#Creates struct of each email info for sendEmail function
def getmsgs(username, password)
	localEmail = Struct.new(:bodyArray, :nameArray)
	localBodyArray = []
	localNameArray = []
        at="@"

        imap = Net::IMAP.new("imap.myOrg.com",993,true)
        imap.login(username, password)
        imap.select("Inbox/.toRT")
        imap.search(["ALL"]).each do |msg|
                envelope = imap.fetch(msg, "ENVELOPE")[0].attr["ENVELOPE"]
                sender = envelope.from[0].mailbox
                host = envelope.from[0].host
                emailaddress = sender + at + host
                body = imap.fetch(msg,'BODY[TEXT]')[0].attr['BODY[TEXT]']

                localBodyArray.push(body)
                localNameArray.push(emailaddress)

        end

        imap.logout
        imap.disconnect
	
	rtnStructEmail = localEmail.new(localBodyArray, localNameArray)
	
	return rtnStructEmail
end

#Creates SMTP connection
#Creates email response for each email retrieved and
#Uses the info from the Struct to personalize each message
def sendEmail(username, password, emailStruct)
        smtp = Net::SMTP.new 'smtp.myOrg.com',587
        smtp.enable_starttls
        smtp.start('myOrg.com',username, password, :login)
        $i = 0
        $num = emailStruct.bodyArray.length
                while $i < $num do
                        msg = "Subject: Hello \n\n requestor: #{emailStruct.nameArray[$i]} \n\n Thank you for your message.  I have forwarded your request to our ticketing system ([email protected]).  This will ensure the fastest response     possible. \n\n Orignal Message: \n #{emailStruct.bodyArray[$i]}"
                smtp.send_message(msg, '[email protected]', [emailStruct.nameArray[$i], '[email protected]'])
                        $i +=1
                end
end


Email = getmsgs(USERNAME,PASSWORD)
sendEmail(USERNAME,PASSWORD, Email)

I'll probably tackle that nasty while loop next and make it a for loop like @Timothee suggested

10

That looks good.

You're commenting your code, which is good.

The logic in there is obviously good, since it's working. I'm not sure what else can be done to improve it.

Another way you could have done this is to make the sendEmail method not take a list of emails, but only take one, then you handle all the iteration in the getmsgs method, calling sendEmail from within getmsgs. But this is probably a bit faster, since you're only configuring smtp once in this current method.

11

I haven't been a Rubyist for very long but, hopefully I can help out a little bit.

I would suggest using the shovel operator to insert items into your array instead of .push. The shovel operator is used for single items where as .push is used for multiple items. You can also use string concatenation for the email address like so:

 emailaddress = "#{sender}@#{host}"

A struct is called a hash in ruby. Instead of instantiating it at the top of your method, I would do the following:

def getmsgs(username, password)
	localBodyArray = []
	localNameArray = []

        imap = Net::IMAP.new("imap.myOrg.com",993,true)
        imap.login(username, password)
        imap.select("Inbox/.toRT")
        imap.search(["ALL"]).each do |msg|
                envelope = imap.fetch(msg, "ENVELOPE")[0].attr["ENVELOPE"]
                sender = envelope.from[0].mailbox
                host = envelope.from[0].host
                emailaddress = "#{sender}@#{host}"
                body = imap.fetch(msg,'BODY[TEXT]')[0].attr['BODY[TEXT]']

                localBodyArray << body
                localNameArray << emailaddress

        end

        imap.logout
        imap.disconnect
	
        {bodyArray: localBodyArray, nameArray: localNameArray)
end

Ruby implicitly returns the last thing it reads in a method and explicitly returning things is not typically done.

For your second method, you're using $signs which instantiate global variables. Global variables shouldn't really be used in most instances.

This takes out the global variables and simplifies your loop:

               emailStruct.bodyArray.length.times do |i|
                  msg = "Subject: Hello \n\n requestor: #{emailStruct.nameArray[i]} \n\n Thank you for your message.  I have forwarded your request to our ticketing system ([email protected]).  This will ensure the fastest response     possible. \n\n Orignal Message: \n #{emailStruct.bodyArray[i]}"
                  smtp.send_message(msg, '[email protected]', [emailStruct.nameArray[i], '[email protected]'])
                end

Those are a few things that stuck out to me. This is also my first post so... I hope it's helpful.

1 Like
12

You're talented in putting a lot of words in one place.

1 Like
13

@TagMe Thanks! That was really insightful and really, really helpful. I appreciate your post!

1 Like
14

You too @FaunCB! I appreciate the positivity!

15

@TagMe Aren't USERNAME AND PASSWORD globals, also - since they aren't declared in any of the functions' scope, too? If they are, any suggestions on not making them global? Could I create a def main at the end of the script, declare those constants within that function, and then just call getmsg and sendEmail from within main?

16

I don't know man
it looks very numbered, like you're one of those neat freaks.
and I guess the text is a little small
but other than that
cool

This is meant as a joke