14
\$\begingroup\$

The following code works but is a mess. But being totally new to Ruby I have had big problems trying to refactor it into something resembling clean OOP code. Could you help with this and explain what you are doing?

require 'nokogiri'
require 'open-uri'
require_relative 'db.rb'        # Not used
require 'mechanize'
require 'digest/md5'
require 'colorize'
require 'win32console'

# Get the Base URL for Groups
# baseURL = '<some site>'
# The Initial Search Point for Groups

#Base URL
baseURL = '<some site>'
pageURL = '<some site>'
doc = Nokogiri::HTML(open(pageURL))

# Search for nodes by css
pages = doc.css('.pages')
thumbs = doc.css('.thumb')
startingPages = pages.css('a').first.text
totalPages = pages.css('a').last.text.to_i
currentPage = 1


## DO THE FIRST PAGE
thumbs.each do |thumb|
imgSrc = thumb.css('.t_img').first['src']
file = open(imgSrc).read.unpack('H*').first 
agent = Mechanize.new
image = agent.get(imgSrc)
md = Digest::MD5.hexdigest(*[file.to_s])
sha = Digest::SHA2.hexdigest(*[file.to_s])
title = thumb.css('.t_img').first['alt']
fileType = image.header["content-type"].to_s
lastModified = image.header["last-modified"].to_s
fileSize = image.header["content-length"].to_s
driver = "Prowler"
source = $pagesPath.to_s
binary = file
fileName = image.filename

# ||, -, *, /, <>, <, >, ,(comma), =, <=, >=, ~=, !=, ^=, (, ) Illegal characters for  sql queries. Check title for these before inserting into the database
title = title.gsub("'", "")


begin
# connect to the MySQL server
dbh = DBI.connect('dbi:ODBC:DamoclesProwler', 'user', 'pw') 
sqlText =   "INSERT INTO prowlerRunningResults(ProwlerDriver, SourceURL, Title, DestinationURL, FileSource, FileType, FileName, LastModified, FileSize, HashMD5, HashSHA256, ImageBinary) 
VALUES ('#{driver}', '#{source}', '#{title}', 'NA', '#{imgSrc}',  '#{fileType}',  '#{fileName}',  '#{lastModified}', #{fileSize.to_i},   '#{md}',   '#{sha}',   '#{binary}'   )"

  dbh.do( sqlText )
  puts "Record has been created for".green + "\t" + sha.yellow
  dbh.commit
rescue DBI::DatabaseError => e
  if(e.errstr == "23000 (3604) [Microsoft][ODBC SQL Server Driver][SQL Server]Duplicate key was ignored.")
    puts "Ignoring Duplicate File".cyan
  else
  puts "An error occurred".red
  puts "Error code:    #{e.err}".red
  puts "Error message: #{e.errstr}".red
  # check update first
  dbh.rollback
  end
ensure
# disconnect from server
dbh.disconnect if dbh
end
end 

puts "--------------------------PAGE 1-----------------------------------"

(2..totalPages-1).each do |k|
$pagesPath = URI.join(baseURL, "/category/0/All/ctr/" + k.to_s + "/")
rdoc = Nokogiri::HTML(open($pagesPath))
tthumbs = rdoc.css('.thumb')
tthumbs.each do |mthumb|
title =  mthumb.css('.t_img').first['alt']
imgSrc = mthumb.css('.t_img').first['src']
file = "0x" + open(imgSrc).read.unpack('H*').first 
agent = Mechanize.new
fop = agent.get(imgSrc)
md = Digest::MD5.hexdigest(*[file.to_s])
sha = Digest::SHA2.hexdigest(*[file.to_s])
fileType = agent.head(imgSrc)["content-type"].to_s
lastModified = agent.head(imgSrc)["last-modified"].to_s
fileSize = agent.head(imgSrc)["content-length"].to_s
driver = "Prowler"
source = $pagesPath.to_s
binary = file
fileName = fop.filename
title = title.gsub("'", "")

  begin
  # connect to the MySQL server
  dbh = DBI.connect('dbi:ODBC:DamoclesProwler', 'user', 'pw')
  sqlText =   "INSERT INTO prowlerRunningResults(ProwlerDriver, SourceURL, Title, DestinationURL, FileSource, FileType, FileName, LastModified, FileSize, HashMD5, HashSHA256, ImageBinary) 
  VALUES ('#{driver}', '#{source}', '#{title}', 'NA', '#{imgSrc}',  '#{fileType}',  '#{fileName}',  '#{lastModified}', #{fileSize.to_i},   '#{md}',   '#{sha}',   '#{binary}'   )"

  dbh.do( sqlText )
  puts "Record has been created for".green + "\t" + sha.yellow
  dbh.commit
rescue DBI::DatabaseError => e
 if(e.errstr == "23000 (3604) [Microsoft][ODBC SQL Server Driver][SQL Server]Duplicate key was ignored.")
    puts "Ignoring Duplicate File".cyan
  else
  puts "An error occurred".red
  puts "Error code:    #{e.err}".red
  puts "Error message: #{e.errstr}".red
  # check update first
  dbh.rollback
  end
ensure
# disconnect from server
  dbh.disconnect if dbh
end
end 
puts "-----------------------------PAGE #{k.to_s}------------------------------------"
sleep(2)
end

I would normally, in other languages, have the Database code in a separate file. How would I do that in Ruby, and what else in Ruby is there that I should be doing?

The code goes to a website - hardcoded Loads the site into doc which is then parsed using nokogiri. The pagination for the site is loaded into pages so that we can work out how many pages we are going to process... this is the link at the top or bottom of the page with something like goto: page 1, 2, 3, 4, ... 600. next the page looks for any images on the page - thumbs (thumbnails) with the code put into thumbs variable. Next the first page is processed...

  1. go through each thumbnail
  2. Get its location
  3. download the file
  4. Hash it
  5. Get info like size, name, type
  6. Save the info and file into the database
  7. Repeat the process for pages 2 .. lastpage
\$\endgroup\$
  • 1
    \$\begingroup\$ Spacing immediately comes to mind. Start a new indentation level after each each do. \$\endgroup\$ – Schism Jul 4 '14 at 17:15
5
\$\begingroup\$

Object-oriented design may be a noble goal, but I think you have more urgent needs. The three most obvious problems I see are:

Indentation and spacing

Your indentation is pretty haphazard. Be sure to indent each block consistently. Without indentation, the code is incomprehensible and unmaintainable.

DBI misuse and SQL injection vulnerability

A comment says that you are connecting to a MySQL database, but the error handling is for Microsoft SQL Server. That's weird, isn't it?

You're connecting to and disconnecting from the database once per page. That's wasteful. The entire program should keep reusing the same database connection.

Most importantly, you should never construct SQL statements by string concatenation or interpolation, since that is what leads to SQL injection vulnerabilities. Instead, you should use parameterized queries:

dbh = DBI.connect('dbi:ODBC:DamoclesProwler', 'user', 'pw')
sth = dbh.prepare('INSERT INTO prowlerRunningResults(ProwlerDriver, SourceURL, Title, DestinationURL, FileSource, FileType, FileName, LastModified, FileSize, HashMD5, HashSHA256, ImageBinary)
                   VALUES (?, ?, ?, 'NA', ?, ?, ?, ?, ?, ?, ?, ?);')
sth.execute(driver, source, title, imgSrc, fileType, fileName, lastModified, fileSize.to_i, md, sha, binary)

The use of 'NA' is odd. Usually, NULL would be used to indicate the lack of a value.

Code duplication

The bulk of the program is repeated code. Such code should live in a function, not just to reduce duplication, but to give it a named purpose.

def scrape_page(dbh, agent, url)
  …
end

In fact, very little code should float freely outside functions, and such code should reveal at a glance what the outline of the entire program is.

require …

baseURL = '<some site>'
pageURL = '<some site>'

begin
  dbh = DBI.connect(…)
  agent = Mechanize.new

  scrape_page(dbi, agent, pageURL)
  2.upto(totalPages-1) do |k|
    scrape_page(dbi, agent, URI.join(baseURL, "/category/0/All/ctr/#{k}/"))
  end
ensure
  dbh.disconnect if dbh
end
\$\endgroup\$
  • \$\begingroup\$ Ok - the formatting is a result of copy and paste into the StackExchange editor... it's not actually like that on my machine. I don't know how to get an external def as mentioned in my question otherwise I would have been doing that. The comment about the MySql is incorrect - I did mention it is MSSql. Thanks for the head's up on how to do parameters in ruby - I looked everywhere for that! -- Hmmm it appears I didn't mention MSSql - my mistake :D \$\endgroup\$ – Dave Gordon Jul 5 '14 at 17:15
5
\$\begingroup\$

I can't speak to Ruby but I see an improvement that could be done to your MySQL code. Since this operation loops multiple times and passes a query to MySQL each time, you would considerably improve your database execution by using a stored PROCEDURE and passing parameters to it. The purpose of this (besides simplifying the code) is that MySQL would store the execution plan with the procedure after it runs for the first time.

So instead of this:

sqlText =   "INSERT INTO prowlerRunningResults(ProwlerDriver, SourceURL, Title, DestinationURL, FileSource, FileType, FileName, LastModified, FileSize, HashMD5, HashSHA256, ImageBinary) 
VALUES ('#{driver}', '#{source}', '#{title}', 'NA', '#{imgSrc}',  '#{fileType}',  '#{fileName}',  '#{lastModified}', #{fileSize.to_i},   '#{md}',   '#{sha}',   '#{binary}'   )"

You could do this (once) in MySQL (or pass it from Ruby, doesn't matter):

DELIMITER |
DROP PROCEDURE IF EXISTS MyProcedure| -- Use a relevant name
CREATE PROCEDURE MyProcedure -- Use a relevant name
  (
  IN driver VARCHAR, -- Note you can specify character limit if you use VAR(50) or other number
  IN source VARCHAR,
  IN title VARCHAR,
  IN imgSrc VARCHAR,
  IN fileType VARCHAR,
  IN fileName VARCHAR,
  IN lastModified VARCHAR,
  IN [fileSize.to_i] VARCHAR, -- Using brackets due to period character
  IN md VARCHAR,
  IN sha VARCHAR,
  IN binary VARCHAR
  );
AS
BEGIN

INSERT INTO prowlerRunningResults(ProwlerDriver, SourceURL, Title, DestinationURL, FileSource, FileType, FileName, LastModified, FileSize, HashMD5, HashSHA256, ImageBinary)
VALUES (driver, source, title, 'NA', imgSrc,  fileType,  fileName,  lastModified, [fileSize.to_i], md, sha, binary);
END|
DELIMITER ;

Note you can use something else as DELIMITER it's a wild card. I've seen // or $$ used frequently, or you can even use GO as long as it is not ;.

Then in your Ruby script you would just do this instead of what you have now:

sqlText =   "CALL MyProcedure('#{driver}', '#{source}', '#{title}', 'NA', '#{imgSrc}',  '#{fileType}',  '#{fileName}',  '#{lastModified}', #{fileSize.to_i},   '#{md}',   '#{sha}',   '#{binary}'   )"
\$\endgroup\$
  • \$\begingroup\$ It's a simple insert into a single table. Why complicate it by creating a stored procedure to do the same thing? \$\endgroup\$ – 200_success Jul 5 '14 at 10:28
  • \$\begingroup\$ Simplify the Ruby script, if nothing else. But I think it is good practice to use a stored procedure for repeat operations. \$\endgroup\$ – Phrancis Jul 5 '14 at 10:30
  • \$\begingroup\$ @200_success this could be debated endlessly, but using a sproc guarantees a cached plan will be used. Otherwise, depending on the db's config, it may or may not use a cached plan. \$\endgroup\$ – RubberDuck Jul 5 '14 at 11:52
  • \$\begingroup\$ A cached plan for a single-table insert? \$\endgroup\$ – 200_success Jul 5 '14 at 11:53
  • \$\begingroup\$ Yes. I don't know how MySQL handles it, but some RDBMS will cache as hoc queries like this and only reuse them if the text matches exactly. It causes cache bloat and extra overhead to calculate a new query plan every time it's executed. You're answer doesn't suffer from this @200_success, because you're using a prepared query. There are other reasons to use sprocs rather than prepared queries though. Like I said, it could be debated endlessly. I say let the database dev do the database development. \$\endgroup\$ – RubberDuck Jul 5 '14 at 12:12

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.