[Ruby]DNS Lookups - Need help improving

[lucid]

I have this code here:

Code: (ruby) [Select]

#!/usr/bin/env ruby


require 'resolv'
require 'socket'


class Resolver


    def initialize(hostname = nil)
        @hostname = hostname
    end


    def is_host?(reg)
        @hostname =~ /\w\.[a-zA-Z]{2,3}/
    end


    def is_ip?(reg)
        @hostname =~ /(?:\d{1,3}\.){3}\d{1,3}/
    end


    def main
        hosts = []

        ARGV.empty? ? printf("Missing arguments!\n") : hosts << @hostname

        begin
            hosts.each do |r|
                if is_host?(r)
                    puts Socket.getaddrinfo(r, nil)[0][2]
                elsif is_ip?(r)
                    puts Resolv.new.getname(r)
                end
            end

        rescue Resolv::ResolvError
            puts "Bad IP address!"
        rescue SocketError
            puts "Bad hostname!"
        end
    end

end


res = Resolver.new(ARGV[0])
res.main
It is working just fine. What it does is accept one argument, and based on the formatting of the argument it's does an appropriate lookup. So for example, if I type: ./dnslookup.rb evilzone.org, then it returns the IP address of evilzone.org. On the other hand, if I type:
./dnslookup.rb 5.9.107.151, then it returns the domain name.

The code works just fine. I'm mainly just looking for input regarding how well it's been coded, and if it could be coded more efficiently, or utilize better methods. I've grown rather unsatisfied with my code recently and am trying to make more interesting, more dynamic, and basically more mature code. Too often I feel like I'm just using a ton of if statements to do the heavy lifting, and while it works, it's unsatisfying. So any input is appreciated.

[flowjob]

Just small details...

• In "is_host" you only allow TLDs of 2-3 chars length, a lot of new TLDs are longer than that tho
• Add an "else" in case of invalid input that seems neither to be a hostname nor an IP
• In "is_ip" IPv6 won't pass the test

[lucid]

Yeah definitely need to work on the regex a bit, I'm aware. What else? Is there a more interesting/dynamic way to code this? What else can be improved?

EDIT: I noticed that the library I use to resolve IP addresses seems to use nslookup. Most of the time, when you use nslookup with an IP it returns the full domain name. Or, in the case of evilzone's IP address it outputs Evilzone.org. I can't imagine an effective and accurate way to parse the results so that it only returns the url, e.g. comcast.net, not, urlrw01.cable.comcast.com.

[Fur]

One thing I can think of is the reusability of your code. If you were to change the interface to a gui or something then you'd have to change the Resolver class. I can't really think of a good way to say it but it would be a good idea to design your classes to be used by other code, not the user.
How about instead having a method called something like lookup(host) that returned a string and left exception handling up to the client code?

To expand on what Flowjob said, you probably want to throw an invalid argument exception if the target is neither a host or ip.

As for the ip address validation, the IPAddress gem exists for this (source).

[lucid]

How about instead having a method called something like lookup(host) that returned a string and left exception handling up to the client code?

Hopefully I'm not being thick-skulled, but could you expand on this a little bit?

[Fur]

Hopefully I'm not being thick-skulled, but could you expand on this a little bit?

Instead of having the main method in Resolver, instead have a method that just performs the lookup. You should keep your main logic separate from your ui, so you would move your main method to somewhere outside Resolver. That way, larger projects and alternative user interfaces can use the class easily.

I can't really explain it well, so here's some shitty Ruby that probably doesn't work:

Code: (Ruby) [Select]

require 'resolv'
require 'socket'

class Resolver # Just assume all methods in this class are static.
    def is_host?(reg)
        @hostname =~ /\w\.[a-zA-Z]{2,3}/
    end
   
    def is_ip?(reg)
        @hostname =~ /(?:\d{1,3}\.){3}\d{1,3}/
    end
   
    def resolve(target)
        if is_host?(target)
           return Socket.getaddrinfo(target, nil)[0][2]
       elsif is_ip?(target)
           return Resolv.new.getname(target)
       else
           raise ArgumentError, "Target is neither a hostname or an IP address."
       end
    end
end

def main
    hosts = []
    ARGV.empty? ? printf("Missing arguments!\n") : hosts << @hostname

    begin
        hosts.each do |target|
            Resolver.resolve(target)
        end
       
        # Really you'll want to print the verbose error messages for exceptions. Would you rather have "something went wrong" or what was actually going wrong printed?
        rescue Resolv::ResolvError
            puts "Bad IP address!"
        rescue SocketError
            puts "Bad hostname!"
        end
        resuce ArgumentError
            puts "Target is neither a hostname or an IP address"
        end
    end
end
main()
Oh, and by "client code" I meant the code that's going to call it (e.g. user interfaces, other projects).

[lucid]

I guess how I can see why that's better. Thanks. I'll try and apply this to everything.