Skip to content

Instantly share code, notes, and snippets.

@Integralist

Integralist/1. Refactoring - Original.js

Last active February 23, 2020 17:34
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save Integralist/7702823 to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save Integralist/7702823 to your computer and use it in GitHub Desktop.
Download ZIP
The importance of refactoring...
Raw
1. Refactoring - Original.js
var isProxyBased = (/S40[\w]{3,5}Browser|Opera\sMini\//i).test(navigator.userAgent);
if (('querySelector' in document && 'localStorage' in window && 'addEventListener' in window && !isProxyBased) || (isIE > 6 && document.getElementById('js-holepunched'))) {
// do stuff
}
Raw
2. Refactoring - Refactored.js
var cutsTheMustard = 'querySelector' in document && 'localStorage' in window && 'addEventListener' in window,
isNotProxyBased = !/S40[\w]{3,5}Browser|Opera\sMini\//i.test(navigator.userAgent),
modernDevice = cutsTheMustard && isNotProxyBased,
holepunched = isIE > 6 && document.getElementById('js-holepunched');
if (modernDevice || holepunched) {
// do stuff
}
Raw
3. Refactoring - Notes.md

I refactored this code by making just a couple of very basic tweaks...

  1. move all logic out of the conditional and into variables
  2. removed unnecessary syntax cruft from isProxyBrowser
  3. give the variables descriptive names
  4. further break down the logic (e.g. group the checks for 'cutting the mustard' and 'being a proxy based browser')
  5. reversed the isProxyBased logic so it becomes isNotProxyBased (this meant we didn't clutter modernDevice with extra syntax and made it as readable as possible)
  6. use selected variables within the conditional
Raw
4. Refactoring - Extra refactoring.md

Is there any thing more we could do with this code to make it as simple as possible to understand?

Well, one small tweak we could make would be to simplify the conditional even further. We could do this by creating another variable (again with a helpfully descriptive identifier) to hold the check of modernDevice || holepunched, like so...

var cutsTheMustard = 'querySelector' in document && 'localStorage' in window && 'addEventListener' in window,
    isNotProxyBased = !/S40[\w]{3,5}Browser|Opera\sMini\//i.test(navigator.userAgent),
    modernDevice = cutsTheMustard && isNotProxyBased,
    holepunched = isIE > 6 && document.getElementById('js-holepunched'),
    enhancedExperience = modernDevice || holepunched;
 
if (enhancedExperience) {
    // do stuff
}

One thing to be aware of is: refactoring can actually be a bad thing if you don't know why you're doing it. If you're just blindly following predefined rules of refactoring then the above small tweak could end up making the code harder to read.

Imagine if we were in a situation where there wasn't a logical association between modernDevice and holepunched. We could end up creating a variable like passesOurChecks (this is a poor example I know). Which would mean this new variable would actually make the code slightly harder to read because it wouldn't be descriptive enough and would likely mean the reader would need to check what the value of that variable was to better understand the code.

Just something to keep in mind.

@Integralist
Copy link
Author

Integralist commented Nov 29, 2013

@robmiller in a typical file it would be yes. I normally have an Object literal with methods for each piece of unique functionality which lets me abide by SRP (the Single Responsibility Principle).

This example is actually from an inline chunk of code that's dumped out by a back-end system directly into an inline script file (eurgh!) so it's a bit like "polishing a turd", but moving that stuff into functions would be trivial (although I feel for the sake of an inline chunk of code that's a lot of extra cruft in comparison -> it all comes down to context)...

function cutsTheMustard(){
    return 'querySelector' in document && 'localStorage' in window && 'addEventListener' in window;
}

function isNotProxyBased(){
    return !/S40[\w]{3,5}Browser|Opera\sMini\//i.test(navigator.userAgent);
}

function modernDevice(){
    return cutsTheMustard() && isNotProxyBased();
}

function holepunched(){
    return isIE > 6 && document.getElementById('js-holepunched');
}

function enhancedExperience(){
    return modernDevice() || holepunched();
}

if (enhancedExperience()) {
    // do stuff
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment