Avoiding boolean traps in es6

Say that we have a ECMAScript6 class like so:

export default class WPAdminLogonPage extends BaseContainer {
	constructor( driver, visit = false ) {
		const jetpackSite = config.get( 'jetpacksite' );
		const wpAdminURL = `https://${jetpackSite}/wp-admin`;
		super( driver, by.css( '#loginform' ), visit, wpAdminURL );
		this.ensureWPAdminStandardLogonShown();
	}

	ensureWPAdminStandardLogonShown() {
		const self = this;
		self.driver.findElement( by.tagName( 'body' ) ).getAttribute( 'class' ).then( ( bodyClasses ) => {
			if ( bodyClasses.indexOf( 'jetpack-sso-form-display' ) > -1 ) {
				return driverHelper.clickWhenClickable( self.driver, by.css( '.jetpack-sso-toggle.wpcom' ) );
			}
		} );
		return self.waitForPage();
	}
}

which we instantiate like so:

this.wpAdminLogonPage = new WPAdminLogonPage( driver, true );

This seems pretty straightforward.

But now imagine we’re adding a new scenario and I have a case where we don’t want to call the ensureWPAdminStandardLogonShown method during constructor.

There’s two ways I see that we could achieve this:

  1. Removing the call to this.ensureWPAdminStandardLogonShown() from the constructor and call this in previous spots where the class is instantiated; or
  2. Adding a second parameter to the constructor that specifies whether the instance should call the ensureWPAdminStandardLogonShown() method

Option 1 is a bit risker as it’s not backwards compatible and we need to make sure all the existing objects are updated to no longer pass the parameter and more importantly explicitly call the ensureWPAdminStandardLogonShown() method on each instance.

Option 2 is ‘safer’ as it’s backwards compatible, so let’s say we just add the second parameter forceStandardLogon and default it to true as that’s what it used to do:

export default class WPAdminLogonPage extends BaseContainer {
	constructor( driver, visit = false, forceStandardLogon = true ) {
		const jetpackSite = config.get( 'jetpacksite' );
		const wpAdminURL = `https://${jetpackSite}/wp-admin`;
		super( driver, by.css( '#loginform' ), visit, wpAdminURL );
		if ( forceStandardLogon === true ) {
			this.ensureWPAdminStandardLogonShown();
		}
	}

	ensureWPAdminStandardLogonShown() {
		const self = this;
		self.driver.findElement( by.tagName( 'body' ) ).getAttribute( 'class' ).then( ( bodyClasses ) => {
			if ( bodyClasses.indexOf( 'jetpack-sso-form-display' ) > -1 ) {
				return driverHelper.clickWhenClickable( self.driver, by.css( '.jetpack-sso-toggle.wpcom' ) );
			}
		} );
		return self.waitForPage();
	}
}

And we can call it like so for our new instance where we don’t want to call the internal method any more:

const wpAdminLogonPage = new WPAdminLogonPage( driver, false, false );

Do you see the issue?

The issue now is that this isn’t very readable code: we’ve got two boolean values one after another and it’s hard to know which one means what. This is known as a boolean trap.

Luckily we’re using ES6 which supports named parameters via ‘destructuring’. This makes our code much clearer to read. Instead of adding just another parameter as a boolean, we use an object:

export default class WPAdminLogonPage extends BaseContainer {
	constructor( driver, visit = false, { forceStandardLogon = true } = {} ) {
		const jetpackSite = config.get( 'jetpacksite' );
		const wpAdminURL = `https://${jetpackSite}/wp-admin`;
		super( driver, by.css( '#loginform' ), visit, wpAdminURL );
		if ( forceStandardLogon === true ) {
			this.ensureWPAdminStandardLogonShown();
		}
	}

	ensureWPAdminStandardLogonShown() {
		const self = this;
		self.driver.findElement( by.tagName( 'body' ) ).getAttribute( 'class' ).then( ( bodyClasses ) => {
			if ( bodyClasses.indexOf( 'jetpack-sso-form-display' ) > -1 ) {
				return driverHelper.clickWhenClickable( self.driver, by.css( '.jetpack-sso-toggle.wpcom' ) );
			}
		} );
		return self.waitForPage();
	}
}

This means it’s much clearer to call, as we can clearly see what the second value is:

const wpAdminLogonPage = new WPAdminLogonPage( driver, false, { forceStandardLogon: false } );

If we didn’t want to worry about backwards compatibility we could also move the visit into the object:

export default class WPAdminLogonPage extends BaseContainer {
	constructor( driver, { visit = false,  forceStandardLogon = true } = {} ) {
		const jetpackSite = config.get( 'jetpacksite' );
		const wpAdminURL = `https://${jetpackSite}/wp-admin`;
		super( driver, by.css( '#loginform' ), visit, wpAdminURL );
		if ( forceStandardLogon === true ) {
			this.ensureWPAdminStandardLogonShown();
		}
	}

	ensureWPAdminStandardLogonShown() {
		const self = this;
		self.driver.findElement( by.tagName( 'body' ) ).getAttribute( 'class' ).then( ( bodyClasses ) => {
			if ( bodyClasses.indexOf( 'jetpack-sso-form-display' ) > -1 ) {
				return driverHelper.clickWhenClickable( self.driver, by.css( '.jetpack-sso-toggle.wpcom' ) );
			}
		} );
		return self.waitForPage();
	}
}

Which gives us by far the easiest to read and understand code:

const wpAdminLogonPage = new WPAdminLogonPage( driver, { visit: false, forceStandardLogon: false } );

What do you think? Have you ever come across a boolean trap like this?

Author: Alister Scott

Alister is an Excellence Wrangler for Automattic.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s