DeChamp

Posted on Feb 5, 2019 • Edited on Aug 12, 2021

Clean up your code with these tips!

#discuss #tips #javascript #tutorial

TL:DR

See bottom;

The breakdown

After reading the discussion If/else or just if?, it made me think that it might be helpful for some of you, for me to share a few patterns I like to follow that helps me keep my code smaller and cleaner.

It's so easy to stick to the patterns/styles of coding that you're used to or that you learned from the very beginning. This will hopefully challenge you to see if you can improve.

I'll walk you through a typical scenario and how to clean it up, by using an example from a recent piece I was working on.

Here is how you might see some developers write it.

Note: I'm coding in es6 for the example.

My code needs to confirm hostname is available, check if it's a localhost site I'm working with and if so then set my cookies to non-secure. This is required because otherwise, chrome and some other browsers will not allow cookie storage via localhost when marked as secure. See here buried deep down in the stackflow

let secure;

// Check if window.location.hostname is set.
if (
    typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname')
) {
    const domain = window.location.hostname;

    if (domain == "localhost" || domain == "127.0.0.1") {
        secure = false;
    } else {
        secure = true;
    }
} else {
    secure = true;
}

export default new Cookie({secure: secure});

Now a few things you might notice right off the bat. Such as getting rid of both the "else" statements by setting the top let secure to let secure = true.

How about the use of property value shorthand? {secure: secure} now becomes {secure}.

let secure = true;

// Check if window.location.hostname is set, 
if (
    typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname')
) {
    const domain = window.location.hostname;

    if (domain == "localhost" || domain == "127.0.0.1") {
        secure = false;
    } 
}

export default new Cookie({secure});

We cleaned it up a bit, but we can definitely do better. Any time you see a sub "if" statement, you should ask yourself "how can I clean this up?". It's rare you need to have sub "if" statements if you know how to avoid them.

Let's first break the sub "if" statement out and put it below the main "if" statement.

We can do that by initializing our "domain" variable above to "null" or "false" (I prefer null, feel free to discuss), then set the domain to the hostname if it's available via window.location.hostname.

Next, we update our sub "if" to now check for "domain" having a truthy value on top of the original check against localhost/127.0.0.1.

That reminds me, lets clean up that check for the localhost/127.0.0.1 with some regex. domain == "localhost" || domain == "127.0.0.1" now becomes /^(localhost|127\.0\.0\.1)$/.test(domain)

If you don’t like regex, you can use this smooth tip from vojtechp to make it even easier to read.

const localDomains = [ 'localhost', '127.0.0.1' ];
const isLocal = domain && localDomains.includes(domain);

or you could do a cleaner version mentioned by miniscruff with Set

const localDomains = new Set([ 'localhost', '127.0.0.1' ])
const isLocal = localDomains.has(domain)

If you're wondering why you should declare variables before using them, read Always Declare Local Variables.

This leaves us with the below.

let secure = true;

// Declared domain variable as a let
let domain = null;

// Check if window.location.hostname is set, 
if (
    typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname')
) {
    domain = window.location.hostname;
}

// Moved out and now checks additionally "domain" is set
if (domain && /^(localhost|127\.0\.0\.1)$/.test(domain)) {
    secure = false;
} 

export default new Cookie({secure});

Hopefully by now you're starting to see how each time, we improve the code a little more.

So how far can we take this? Let's see what else we can do.

One major improvement to my coding style, I learned off of a random blog. I really wish I could give them credit, but unfortunately it was so long ago, I forget who it was.

What they showed was to move the logic out of if statements and assign them to variables, when it involves 2 or more values. I'll have to write up another post about this, as you can get really creative with it, but for now we'll keep it simple.

So we now would go from

if (
    typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname')
)

const hostnameAvailable = typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname');

Where the power lies in this, is that you can start to clean up your if statements to be more readable, or even move away from them all together (within reason).

So knowing this new trick we move forward with that in mind.

If you are paying close attention, you will see we are building the "if" statements, off of each other... So it feels like maybe we could take advantage of the Ternary Operator.

Combine the new trick we learned about moving out the "if" logic to variables, with ternary and you can do this!

let secure = true;

const hostnameAvailable = typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname');
const domain = hostnameAvailable ? window.location.hostname : null;
const isLocal = domain && domain.match(/localhost|127\.0\.0\.1/);

if (isLocal) {
    const secure = false; 
}

export default new Cookie({secure});

But, DeChamp... that "if" statement! Oh right, we can fix that too. Take advantage of flipping truthy/falsy with logical not "!". Take a look at the double logical not, on that same page as well.

const hostnameAvailable = typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname');
const domain = hostnameAvailable ? window.location.hostname : null;
const isLocal = domain && /^(localhost|127\.0\.0\.1)$/.test(domain);
const secure = !isLocal;     

export default new Cookie({secure});

Wow, that looks so much better!

Wrap up

We learned that declaring variables with values up top, can help eliminate "else" statements.
Breaking out sub "if" statements.
Moving out the "if" logic to variables, and then using ternary, can help make it easier to read and chain together.
Regex allows further clean up, when doing multiple checks on a single variable.
Using "not" operator to flop boolean values.

More tips!

// instead of this
if (foo === 'a') {
    bar = 'first';
} else {
    bar = 'second';
}

return bar;


// use return to clean it up!

if (foo === 'a') {
    return 'first';
}

return 'second';

// instead of this

const foo = bar ? bar : baz;

// do this
const foo = bar || baz;

This one was provided via user Kayis

let secure = true;

try {
  secure = !window.location.hostname.match(/localhost|127\.0\.0\.1/);
} catch (e) {}

export default new Cookie({ secure });

Add yours in the comment and I'll add them here and give you credit!

TL:DR

from

let secure;

// Check if window.location.hostname is set.
if (
    typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname')
) {
    const domain = window.location.hostname;

    if (domain == "localhost" || domain == "127.0.0.1") {
        secure = false;
    } else {
        secure = true;
    }
} else {
    secure = true;
}

export default new Cookie({secure: secure});

const hostnameAvailable = typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname');
const domain = hostnameAvailable ? window.location.hostname : null;
const isLocal = domain && /^(localhost|127\.0\.0\.1)$/.test(domain);
const secure = !isLocal;     

export default new Cookie({secure});

Followup

I really hope you learned some new tricks. It can be a lot of fun to see how small and clear you can get your code. I wouldn't be surprised if some of my smart readers, teach me even more tricks!

Did you like my post? Have questions? Did I miss something or make a mistake? Let me know!

--DeChamp

Varymade LLC.

Current projects are https://charactergenerator4000.com and https://coder.exchange. Please check them out and let us know your thoughts.

Top comments (39)

Basti Ortiz • Feb 5 '19 • Edited

Gotta admit, that does indeed look a hundred times more readable (assuming you're familiar with regular expressions).

I would just like to chime in on this line of code in the final example:

const domain = hostnameAvailable ? window.location.hostname : false;

I think it would be better if the fallback value was null instead of false. It doesn't change anything about the program, but semantically, null makes more sense because the variable domain expects a string value. If we were to stick with using false, then I don't think it makes sense that a domain name is a Boolean value. I think it would make more sense to say that it is null, or rather nonexistent.

But then again, I might just be nitpicking here. Nonetheless, it still is a hundred times more readable than the initial code.

DeChamp • Feb 5 '19

I like your argument and I agree with you. Thank you for the feed back btw!

Basti Ortiz • Feb 5 '19

No problem, man! Great article. It made me think of how I could refactor my code in a similar way.

DeChamp • Feb 5 '19

I’m glad to hear! I will add more to it tomorrow as I wasn’t able to list all my tricks. But this should be a good start. Thank you!

K • Feb 5 '19

The best way to get the right answer on the internet is not to ask a question; it's to post the wrong answer. - Cunningham's Law

My try on this:

let secure = true;

try {
  secure = !window.location.hostname.match(/localhost|127\.0\.0\.1/);
} catch (e) {}

export default new Cookie({ secure });

DeChamp • Feb 5 '19

Added this to the tips! Gave you credit.

DeChamp • Feb 5 '19

I have seen this and used it in the past. I am less likely to use this but it still gets the job done! And much smaller. Thanks for your feedback.

K • Feb 5 '19

Mind to elaborate? 🙃

DeChamp • Feb 5 '19

Sure! First I'd like to say that I find nothing wrong with your solution. I just prefer to avoid using try/catch as a way to soak up possible errors unless I plan on dealing with those errors.

I use try/catch when intentionally handling errors that would be thrown. Example borrowed from Mozilla for lack of valid example off top of head.

try {
  myroutine(); // may throw three types of exceptions
} catch (e) {
  if (e instanceof TypeError) {
    // statements to handle TypeError exceptions
  } else if (e instanceof RangeError) {
    // statements to handle RangeError exceptions
  } else if (e instanceof EvalError) {
    // statements to handle EvalError exceptions
  } else {
    // statements to handle any unspecified exceptions
    logMyErrors(e); // pass exception object to error handler
  }
}

Tillman Dickson, Jr. • May 13 '19

I am clearly opinionated here :)

This works and it reads clearly, but it is devoid of specificity.
You desire to control flow on state, not on error. Error is normally flow control failed.

The code tells nothing about what is expected, only it might fail in certain generic ways.

Try excepts are essentially GOTOs, as flow control they smell?

People will point to the performance hit, but creating the additional stack frame and even unwinding it is usually significant.

The variability of being able to optimize the code in the protected frame varies, so can hit perf noticably or nearly none.

Unless you are using ES7 try blocks are synchronous, so careful with this approach.

My real concern it is backwards and -to me- lazy. If your state is bad in a known detectable way why continue much less spread it to myroutine which may incur other side effects due to caller pollution. Then finding it assuming myroutine has some protection is up the call stack we go.

There are cases where this is the only real choice and there are cases where detection is expensive and or very rare so the trade-off might be compelling. Otherwise, if never approve the PR.

DeChamp • Jun 12 '19

I appreciate that you don’t just accept it and you have an difference of opinion. The world would be boring if no one challenged others. I would love to see you write out the code to show what each of your points refer to.

DeChamp • Feb 5 '19

I am curious, with the "cunningham's law" post, are you stating that you think I'm wrong on my post? If so I'd be happy to hear why you think so.

K • Feb 5 '19 • Edited

No, I didn't think you were wrong. I had the impression you were searching for a more concise solution 🤣

DeChamp • Feb 5 '19 • Edited

It was to show others how they can clean up their code to make it smaller and easier to read. Yours definitely does that. lol. Thanks for providing feedback.

Alex Lohr • Feb 5 '19 • Edited

Three minor additions: you can omit the ternary in your case in favor of a logical gate and you should use /regex/.test(string) instead of string.match(/regex/) if you don't want to do anything with the result and lastly, you don't want domains like localhost.mydomain.com to be interpreted as secure, so better make sure that regex captures the whole domain string - so now it looks like this:

const hostnameAvailable = typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname');
const domain = hostnameAvailable && window.location.hostname;
const isLocal = domain && /^(?:localhost|127\.0\.0\.1)$/.test(domain);
const secure = !isLocal;

export default new Cookie({secure});

I would even think that using a direct comparison for the domain would be faster and more readable than the regex, so now it becomes:

const hostnameAvailable = typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname');
const domain = hostnameAvailable && window.location.hostname;
const isLocal = domain === 'localhost' || domain === '127.0.0.1';
const secure = !isLocal;

export default new Cookie({secure});

DeChamp • Feb 5 '19 • Edited

Aw! you're so right! I should have used test vs match, also I should have added in ^$ for an exact match. I'll update this! Part of the fun was to show people they can resort to regex when doing multiple test against a single variable. If it was another scenario of checking the domain against lets say 3 or more possible values, then regex is a quick way to solve this.

Thank you for the great feed back.

Alex Lohr • Feb 5 '19

You're welcome. I'm usually rather eager to solve things with regex myself. So I usually think again: "will a regex make that code better?" and try it without the regex. In ~50% of the cases, I keep the regex. For the sake of having an example, I feel it's justified. For live code, I would have removed it.

DeChamp • Feb 5 '19

I like your clear view on this. I agree that the regex is over kill when we are only checking two values.

Andrew Harpin • Feb 6 '19

While this obviously works, I personally don't like this style of code.

Ok it takes up less screen real estate, but it's operation is not obvious.

Fundamentally code is a document and a document is useless if it is difficult or impossible to read.

Alex Lohr • Feb 6 '19

What is difficult to read it what is obvious to you strongly depends on what you're used to. Using logical gates is pretty common for example in Python.

So basically your judgement depends on your own bias for a certain style.

Patrick Tingen • Feb 6 '19

I am not familiar with JS since I write in another language, but your example is clear enough to follow.

Still, I did not find that the examples after the first improvement got any better. To me, "more readable code" is not necessarily "shorter code" and - again - I am not very familiar with JS, but your final result is not very readable to me. You abstract away code in a const, and I am an advocate of doing that but these constructs are so small that I find it gains little. Personally I would have stopped after the first improvement. Improving the code is good, but remember that better is the enemy of good enough ;)

DeChamp • Feb 6 '19

For some of the people out there, this may not be a style they like. I also could have also went in to more detail as to why abstracting out the logic to constants can help clean up the code, with more scenarios. But for many people, they like this and it helped them. So it’s really about taking what works for you and leaving the rest behind.

VojtechP • Feb 6 '19

Regex is not much readable when there are more strings.

const isLocal = domain && /^(localhost|127\.0\.0\.1)$/.test(domain);

This is for me better solution:

const localDomains = [ 'localhost', '127.0.0.1' ];
const isLocal = domain && localDomains.includes(domain);

DeChamp • Feb 6 '19

This is also a very nice solution. I’ll add it to the tips! Thank you. I know that for some people, regex can be a eyesore. But there is beauty in its power.

miniscruff • Oct 13 '19

Not much difference but I like to use sets for these comparisons for faster lookups.

const localDomains = new Set([...])
localDomains.has(...)

DeChamp • Oct 14 '19

I actually also like this as a solution. Thank you!

rhymes • Feb 5 '19

Yay for a more declarative style of programming, nice walkthrough! :)

A tip for future reference, you can highlight code blocks by appending the name of the language, in this case javascript at the end of the three backticks, like this:

this is the result:

const hostnameAvailable = typeof window !== 'undefined'
    && window.hasOwnProperty('location')
    && window.location.hasOwnProperty('hostname');
const domain = hostnameAvailable ? window.location.hostname : null;
const isLocal = domain && domain.match(/localhost|127\.0\.0\.1/);
const secure = !isLocal;     

export default new Cookie({secure});

DeChamp • Feb 5 '19

Ha! I knew that one but assumed the site didn’t handle it. Dummy me for assuming. Thanks for the compliment and tip!

Mikael Klages • Feb 5 '19 • Edited

It could be made even prettier if js had null-propagation and elvis, IE:

const secure = window
    ?.location
    ?.hostname
    ?.match(/localhost|127\.0\.0\.1/)
    ?: true;

(excuse my bad coding, writing on phone)

DeChamp • Feb 5 '19

I tried this and it did not work for me. I'd love to see a work example as I have seen this pattern before but couldn't remember to do it.

Alex Lohr • Feb 5 '19

It's not supported in current ECMAscript. However, a standard proposal called "optional chaining" is currently accepted: github.com/tc39/proposal-optional-...

DeChamp • Feb 5 '19

Wow I hadn't caught up to that. Very interesting share. Curious to see if it gets implemented.

Mihail Malo • Feb 8 '19

Out of curiosity, before reading the article I copied the initial example and tried my best on it myself.

This is what I ended up with:

const needsSecure = () => {
  const hostname =
    typeof window !== "undefined" &&
    window.location &&
    window.location.hostname
  if (!hostname) return true

  if (hostname === "localhost" || hostname === "127.0.0.1") return false

  return true
}

const secure = needsSecure()

export default new Cookie({ secure })

On one hand, I feel ridiculous I have a function that will only run once.
On the other, there are no let bindings in the file.

I did consider Array.prototype.includes() but elected not to use it for just two elements.

I don't think the RegEx has good readability, especially having had to escape the dots.

I also noticed that you used === for comparing to "undefined" but == for the domain.

Is there a reason you are using hasOwnProperty instead of just the truthiness of the objects? Is it more performant?
Myself, in the process of writing this, I went back and changed it, and was happily surprised that

// the expression
window.location.hasOwnProperty("hostname") &&
window.location.hostname
// collapsed to
window.location.hostname

DeChamp • Feb 11 '19

So I loved that you took try at it. You did good!

Doing a function is typically a sure thing. It's never stupid to abstract out to a function. It's a great habit to practice. Even if it only gets used once, you are completely right about it helping to avoid having to do things such as declaring additional variables and also making the assignment highly clear const secure = needsSecure() vs const secure = !isLocal;.

Now far as this part, you can do 1 better.

const needsSecure = () => {
  const hostname =
    typeof window !== "undefined" &&
    window.location &&
    window.location.hostname
  if (!hostname) return true

  if (hostname === "localhost" || hostname === "127.0.0.1") return false

  return true
}

this instead, removing an additional if statement.

const needsSecure = () => {
  const hostname =
    typeof window !== "undefined" &&
    window.location &&
    window.location.hostname

  if (hostname && (hostname === "localhost" || hostname === "127.0.0.1")) return false

  return true
}

With the RexEx, that was more of an option to show others that you could bring in RegEx to do your checking. But you and others have pointed out, that my goal was to make the code more readable... so to a lot of people, it's hard to read so therefore it did not make the code "easier to read".

Far as strict comparison, vs non-strict. That is a bad habit I need to break. You caught me red-handed lol. Stick with using strict comparison, and only use non-strict as needed.

Now for the final part, hasOwnProperty is there for a good reason. I am wanting to specifically check for a Boolean answer, so hasOwnProperty will give me that. You can definitely do it your way and it'll work. When it's a single variable, that is how I do it.

When I'm digging down in to an object I prefer to be more intentional with it by using hasOwnProperty

Mihail Malo • Feb 12 '19

So then, something like this?

typeof window !== "undefined" &&
  window.hasOwnProperty("location") &&
  window.location.hostname

I guess.

It's actually really unclear how to do such checks, you have to keep in mind what failures you actually expect to have.
On one hand, if you expect an object, truthiness gets rid of null, undefined, false (we'd be getting the false from the typeof equality, as well as hasOwnProperty) as well as less importantly 0, and "".

// OH NO
;({ location: undefined }.hasOwnProperty("location")) // true
;({ location: null }.hasOwnProperty("location")) // true
// Better I guess
!!({ location: undefined }).location // false

But on the other hand, if you expect a number, you will have weird failure on 0, which is very commonly a normal number. In which case you often check for undefined.

const fn1 = (num = 1) => num // How would we write this without default notation?
const fn2 = num => num || 1 // NO, YOU DIE!
const fn3 = num => (num === undefined ? 1 : num) // it's okay to not use typeof x === "undefined" in this context, it's a declared argument.

But it doesn't solve everything!

const window = { location: { hostname: { valueOf: () => "localhost" } } }
const hostname = typeof window !== "undefined" && window.location && window.location.hostname
""+hostname === "localhost" // The triple equals won't save you now, once string concatenation happened!

Maybe we should always use typeof :/
But what is your next action if hostname is an object? Pretending it wasn't set and defaulting to secure?

const needsSecure = () => {
  const hostname =
    typeof window === "object" &&
    typeof window.location === "object" &&
    typeof window.location.hostname === "string" &&
    window.location.hostname

  if ((hostname && hostname === "localhost") || hostname === "127.0.0.1")
    return false

  return true
}

Honestly, truthiness seems like the best bet to me, I just need to keep track of when it can be a 0 or "" or a literal significant false and it'll be okay.
I think I'll just keep using that.

> (undefined).potatoes
//TypeError: Cannot read property 'potatoes' of undefined
> (null).potatoes
//TypeError: Cannot read property 'potatoes' of null
> (true).potatoes
undefined // And it will never be anything truthy on a non-object, right? RIGHT?
> (true).valueOf
[Function: valueOf] // OH GOD DAMMIT!!