Wed 06 Sep 2017
Reading time: (~ mins)
After resurrecting Snappy from the grave of my personal projects in the last blog post, the next steps is where we get to stretch our brain muscles. Makeover time!
Before digging into the refactor I want to grab two of my favourite gems that I use on every ruby project. The first is the pry gem which is actually a replacement for the standard interactive ruby REPL. The main use I have for this gem is its ability for inline debugging! Instead of putting print statements everywhere all you need now is a:
binding.pry
and suddenly you have a REPL session with the current scope and state of the running program. This is great to quickly test values, methods and assumptions at any point of execution. The next powerhouse of a gem is rubocop. This gem helps keep your code clean by following ruby community guidelines from code style, conditional complexity and everything in between. A quick:
rubocop -a
in your apps directory will check your code for common mistakes and automatically fix most of them! You can then manually dive in for the nasty ones that were uncovered. This is especially useful for code consistency on large projects or when programming with others.
Now the refactoring begins! Here is the repo in the state at which I began to make these changes if you want to follow along. I let rubocop do its thing which ends up fixing a tons of small code smells all across the project. The first part of code that I want to tackle myself is the caching logic. It looked very confusing and I can do a better job at making it more obvious of what it's actually accomplishing.
File: snappy.rbdef cached? image s = Snapshot.first(:name => image) if s if s.updated_at.to_time < Time.now - 1 * 60 snap @image, params[:url], '1440', '900' snap '2' + @image, params[:url], '640', '960', true s.updated_at = Time.now s.save enddef cached?(image) screen_shot = Snapshot.first(name: image) if screen_shot refresh_snap(screen_shot)trueelse falseend end
Pretty obvious beginner signs of useless variables names, unnecessary nesting of conditionals, and not returning proper values based on the expected behaviour of methods ending with ?.
Instead of using the mysterious s variable I'll be more explicit and use screenshot. It immediately becomes more readable when using it as a conditional expression! :cached? doesn't need to worry about what to do with the screenshot so I move that out into a new method, :refresh_snap. Much cleaner now and returning boolean values as would be expected.
The new method will hold the code to get a fresh screenshot of a pre-existing snap if it is more than a minute old. This will prevent the app from always being busy if someone spams a certain url. What I want to fix here is to exit as early as possible if our condition is not met. This can be done easily with a guard clause. Also there is no reason to multiply 1 by 60, so this is the method we end up with:
File: snappy.rb
def refresh_snap(image)
return unless image.updated_at.to_time < Time.now - 60
snap image, params[:url], '1440', '900'
snap '2' + image, params[:url], '640', '960', true
image.updated_at = Time.now
image.save
end
I would like to clean up :snap to make it more readable. Rubocop helps me out by changing the {...} block into a do...end block and I fix a bit more styling by collapsing the Phantonjs.run params into a single line. The last tweak is again to use a guard clause instead of the clunky if block for timeouts:
File: snappy.rbPhantomjs.run( './public/script/peek.js', url, x, y, "./public/#{name}.png", agent, ) { |k| p k if k.chomp.include? 'timeout' redirect to('/timeout') return end store @image if k.chomp.include? 'done'}Phantomjs.run('./public/script/peek.js', url, x, y, "./public/#{name}.png", agent) do |k| return redirect to('/timeout') if k.chomp.include? 'timeout' store @image if k.chomp.include? 'done' end
Moving further down the next stink is the after block. This will occur after each request and delete stale photos. The easy cleanup here is to precompute the time that I want to use to make a decision and use proper variable names:
File: snappy.rbafter dos = Snapshot.all :created_at.lt => Time.now - 2 * 60 * 60 * 24 * 7 JSON.parse(s.to_json).each { |s| File.delete "./public/#{s['name']}.png"images = Snapshot.all :created_at.lt => Time.now - 1_209_600 JSON.parse(images.to_json).each do |image|beginFile.delete "./public/2#{s['name']}.png"File.delete "./public/#{image['name']}.png" File.delete "./public/2#{image['name']}.png"rescue 'file not found' end} s.destroyend images.destroyend
Next is a cleanup to hide the Sinatra error message from end users. There is no reason they need to see the guts of the app, and for me it's easy enough to dive into the Heroku logs to find problems:
error dohalt 500, 'Sorry there was a nasty error - ' + env['sinatra.error'].messagehalt 500, 'Sorry there was a nasty error'end
After some testing, I begin to realize there is a pretty big flaw with this app. Images don't seem to be kept for very long on the Heroku servers. After some googling I find that images uploads go into temporary storage which means every time the app sleeps and wakes up again everything is missing. An easy fix is just to set the max lifetime for my images to be 30 minutes, which is the time a Heroku app will stay awake without any activity. I change :refresh_snap timing to every 3 minutes (180 seconds) and reduce the after hooks timing to every half hour (1800 seconds).
With some prying and poking it also turns out that my before filter is useless. I can't seem to figure out what is doing and removing it doesn't cause any issues. In that case it's 4 lines of code that I get to throw away and forget about, sweet!
Before I tackle async screenshots I want to make sure my app is serving as efficiently as possible. I know the task itself will still be slow so the least I can do is make everything else lighting fast. Google is king here and they provide a sweet tool to test what's wrong with your site's speed as well as giving a comprehensive list of everything you need to fix:
1. Download PageSpeed Insights optimized images and css for my site
2. Inline all custom css and deferrer loading non essential css
3. Use Rack::Deflater to compress my responses
4. GET 99/100 FOR BOTH MOBILE AND DESKTOP SCORES
Finally all that is left to do is make the actual screen grabbing async so the user is not waiting on an empty white screen for the server to respond.
The first step is to make every part of the app use my api route. This means deleting a lot of code, hurray! All calls to :cached? and :snap that are not in the api route are deleted. Sadly our newly refactored method :refresh_snap is also deleted as it's not needed. The api will always grab a screenshot unless the image is 3 minutes fresh. Here are the changes for brevity.
The vanilla JS implementation of an ajax is pretty straight forward:
File: screen.erb
window.onload = function(e){
var xhr = new XMLHttpRequest();
xhr.open('GET', '/api/<%= params[:image]%>');
xhr.send(null);
xhr.onreadystatechange = function () {
var DONE = 4; // readyState 4 means the request is done.
var OK = 200; // status 200 is a successful return.
if (xhr.readyState === DONE) {
if (xhr.status === OK) {
resp = JSON.parse(xhr.responseText);
document.images[0].style.height = 'auto';
document.images[1].style.height = 'auto';
document.images[0].src=resp['desktop'].replace('\/desktop','');
document.images[1].src=resp['mobile'].replace('\/mobile','');
} else {
alert('Timeout'); // An error occurred during the request.
}
}
};}
And some changes to the DOM to have a loading gif until the server responds with content
TADA! Snappy is now running...snappily ;) Be sure to dig around the code I linked for these last few steps since I glossed over it. It's barebones AJAX so if you are unsure what is going down, don't be afraid to Google it out.