For some reason, I feel like this is a bit messy and could be cleaner. Any suggestions?

I'm selecting any image files ending in .png or .jpg, and removing any image source files that contains avatar (meaning it's a header image or an avatar).

def self.images(url)
  proxy_addr = 'http://localhost:'
  proxy_port = 8080
  doc = Nokogiri::HTML(open(url), proxy: "#{proxy_addr}#{proxy_port}")
  images = doc.css('img[src$="jpg"], img[src$="png"]').select do |uri|
    uri['src'] =~ %r{^http://(\d+|media)}
  images.map { |uri| uri['src'] }.reject { |uri| uri =~ /avatar/ }
  • \$\begingroup\$ Perhaps some added white space could help \$\endgroup\$ – Alex L Jul 31 '14 at 6:26
  • \$\begingroup\$ Between proxy_port and doc would help a bit. \$\endgroup\$ – user27606 Jul 31 '14 at 6:31

There is no point in hard-coding proxy_addr and proxy_port separately, then concatenating them using interpolation. On the other hand, it would be useful to make the proxy configurable. One way is to use a default parameter:

def self.images(url, proxy='http://localhost:8080')
  doc = …

(Personally, I'd choose to have proxy default to nil, but the formulation above preserves compatibility with your existing code.)

You appear to be using the open function that is provided by OpenURI. You have a bug: the proxy needs to be specified as an option to open rather than to Nokogiri#HTML. Furthermore, opening the stream like that makes it impossible to close it properly. I suggest using a block instead:

def self.images(url, proxy=nil)
  open(url, proxy: proxy) do |io|
    doc = Nokogiri::HTML(io)

Your regular expression should be case-insensitive when it looks for http:, as recommended by RFC 3986 Sec 3.1:

An implementation should accept uppercase letters as equivalent to lowercase in scheme names (e.g., allow "HTTP" as well as "http") for the sake of robustness but should only produce lowercase scheme names for consistency.

Furthermore, hostnames are case-insensitive, as stated in RFC 1035 Sec 2.3.3 (though clarified by RFC 4343 Sec 2):

For all parts of the DNS that are part of the official protocol, all comparisons between character strings (e.g., labels, domain names, etc.) are done in a case-insensitive manner.

Also consider allowing URLs that start with https:.

The parameter to your two blocks should not be named uri, but rather img, after the <img> tags that your CSS-style selector requested.

The way you sequence select, map, and reject is awkward. I suggest

doc.css('img[src$="jpg"], img[src$="png"]').map do |img|
end.select do |uri|
  uri =~ %r{^https?://(\d+|media)}i and uri !~ /avatar/
  • \$\begingroup\$ You always give solid advice, thanks for the insight. I'll review the post again when I wake up. It's late! One last thing, could you explain why defaulting the proxy to nil is a better alternative and the regex i and uri !~? \$\endgroup\$ – user27606 Jul 31 '14 at 8:09
  • \$\begingroup\$ If no proxy is specified, I'd expect it to make a direct HTTP request, just like any computer would behave out of the box if you didn't configure it. \$\endgroup\$ – 200_success Jul 31 '14 at 8:11
  • \$\begingroup\$ I see, what about the regex? I've never seen i and !~ is that valid? \$\endgroup\$ – user27606 Jul 31 '14 at 17:54
  • \$\begingroup\$ /pattern/i means ignore case. !~ is a negative regexp match operator. You could also write it as … and not uri =~ /avatar/ instead. \$\endgroup\$ – 200_success Jul 31 '14 at 18:00
  • \$\begingroup\$ Nice, thanks for the great advice once again. \$\endgroup\$ – user27606 Jul 31 '14 at 18:15

Your Answer

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