2
\$\begingroup\$

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)}
  end
  images.map { |uri| uri['src'] }.reject { |uri| uri =~ /avatar/ }
end
\$\endgroup\$
2
  • \$\begingroup\$ Perhaps some added white space could help \$\endgroup\$
    – Alex L
    Commented Jul 31, 2014 at 6:26
  • \$\begingroup\$ Between proxy_port and doc would help a bit. \$\endgroup\$
    – user27606
    Commented Jul 31, 2014 at 6:31

1 Answer 1

0
\$\begingroup\$

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 = …
end

(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)
    …
  end
end

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|
  img['src']
end.select do |uri|
  uri =~ %r{^https?://(\d+|media)}i and uri !~ /avatar/
end
\$\endgroup\$
5
  • \$\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
    Commented Jul 31, 2014 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\$ Commented Jul 31, 2014 at 8:11
  • \$\begingroup\$ I see, what about the regex? I've never seen i and !~ is that valid? \$\endgroup\$
    – user27606
    Commented Jul 31, 2014 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\$ Commented Jul 31, 2014 at 18:00
  • \$\begingroup\$ Nice, thanks for the great advice once again. \$\endgroup\$
    – user27606
    Commented Jul 31, 2014 at 18:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.