Function to determine the appropriate dimensions for a close button

Question

How can I reduce the cyclomatic complexity of this CloseButtonDimensions function?

export function isMobile():boolean {
    return (/Android|webOS|iPhone|iPad|iPod|BlackBerry|iemobile|Windows Phone/i.test(navigator.userAgent) );
}

get CloseButtonDimensions(): Size {
    const IS_MOBILE: Boolean = isMobile();

    let mediaType:String     = this.options.tag.media_type;
    let adWidth:number       = this.options.tag.size.width;
    let adHeight:number      = this.options.tag.size.height;   

    let closeButtonHeight:number;
    let closeButtonWidth:number;

    if( (mediaType == "VIDEO") && !IS_MOBILE && ((adWidth >= 640 && adHeight >= 480) || (adWidth >= 480 && adHeight >= 360)) ) {
        closeButtonWidth  = 15;
        closeButtonHeight = 15;
    } else if( ((mediaType == "VIDEO")  && (adWidth >= 300 && adHeight >= 250)) ||
               ((mediaType == "BANNER") && (adWidth == 300 && adHeight == 250)) ||  
               ((mediaType == "BANNER") && !IS_MOBILE && (adWidth == 468 && adHeight == 60)) ) {
        closeButtonWidth  = 11;
        closeButtonHeight = 11;
    } else if( (mediaType == "VIDEO")  &&  IS_MOBILE && (adWidth >= 320 && adHeight >= 240) ||
               (mediaType == "BANNER") && !IS_MOBILE && (adWidth == 728 && adHeight == 90) ) {
        closeButtonWidth  = 13;
        closeButtonHeight = 13;
    } else if((mediaType == "BANNER") && !IS_MOBILE && (adWidth == 468 && adHeight == 60)) {
        closeButtonWidth  = 13;
        closeButtonHeight = 13;
    } else if( (mediaType == "BANNER") && IS_MOBILE && ((adWidth == 320 && adHeight == 50) || (adWidth == 300 && adHeight == 50)) ) {
        closeButtonWidth  = 8;
        closeButtonHeight = 8;
    }  

    return {
        width: closeButtonWidth,
        height: closeButtonHeight
    }
}

mediaType that I can get in the function are VIDEO and BANNER.

Ad dimensions (widthxheight) for video are (these can be greater than or equal to):

• Desktop: "640x480", "480x360", "300x250"
• Mobile: "300x250", "320x240"

Ad dimensions (widthxheight) for banner are (these will be exact not greater nor equal):

• Desktop: "728x90", "468x60", "300x250"
• Mobile: "300x250", "320x50", "300x50"

IS_MOBILE will be true/false depending on the device.

Questions:

1. How can I approach such complex if else statements while coding?
2. How can I make code more readable especially for junior developers or when I make commits to repos so others can understand better?

Answer 1

First, a few general notes about types.

• Don't use String and Boolean. Use string and boolean instead. See the Typescript Handbook
• If you know what mediaType must be, type it to require this.

type Media = 'VIDEO' | 'BANNER';

let m1: Media = 'VIDEO'; // OK
let m2: Media = 'video'; // Error: Type '"video"' is not assignable to type 'Media'.

Now on to your function. I believe your biggest problem here is that you are trying to do too much in a single function. As it appears you are using this in a class which can have two different types -- VIDEO and BANNER. You likely have other similar methods where the behavior is slightly different for each case. An abstract parent class for common functionality with a separate class for BANNER and VIDEO elements might a good idea.

To avoid assuming too much, I have left all of the logic in a single method for now, though splitting this up would greatly help with the method length.

The way I would do this is to avoid if statements as much as possible to make it simpler to extend the configuration in the future. With your current approach, what happens if the customer says they want to add a video ad with the dimensions of 300x300 on desktop? With this in mind, here is the approach I would take. It is a bit longer than your approach, but the cyclomatic complexity is 6, and adding further configurations does not increase the complexity. Splitting this up into two separate methods getCloseButtonBannerDims and getCloseButtonVideoDims is also a trivial change to shorten the method.

get CloseButtonDimensions(): Size {
    const IS_MOBILE = isMobile();

    let mediaType: Media = this.options.tag.media_type;
    let adWidth: number = this.options.tag.size.width;
    let adHeight: number = this.options.tag.size.height;   

    // Sizes must be listed in decreasing order for this to work
    let sizes: Array<Size & {adWidth: number, adHeight: number}>;

    if (mediaType == "VIDEO") {
        if (!IS_MOBILE) {
            // Desktop:- "640x480", "480x360", "300x250"
            sizes = [
                { adWidth: 640, adHeight: 480, width: 15, height: 15 },
                { adWidth: 480, adHeight: 360, width: 15, height: 15 },
                { adWidth: 300, adHeight: 250, width: 11, height: 11 },
            ];
        } else {
            // Mobile:-  "300x250", "320x240"
            sizes = [
                { adWidth: 300, adHeight: 250, width: 11, height: 11 },
                { adWidth: 320, adHeight: 240, width: 13, height: 13 },
            ];
        }

        // Find the first ad where both the width and the height are
        // Greater than or equal to the expected size
        let {width, height} = sizes.find((size) => adWidth >= size.adWidth && adHeight >= size.adHeight);

        return { width, height };
    }

    // We know mediaType must be BANNER as we handled all VIDEO types above
    if (!IS_MOBILE) {
        // Desktop:- "728x90", "468x60", "300x250"
        sizes = [
            { adWidth: 728, adHeight: 90, width: 13, height: 13 },
            { adWidth: 468, adHeight: 60, width: 11, height: 11 },
            { adWidth: 300, adHeight: 250, width: 11, height: 11 },
        ]
    } else {
        // Mobile:-  "300x250", "320x50", "300x50"
        sizes = [
            { adWidth: 300, adHeight: 250, width: 11, height: 11 },
            { adWidth: 320, adHeight: 50, width: 8, height: 8 },
            { adWidth: 300, adHeight: 50, width: 8, height: 8 },
        ];
    }

    let {width, height} = sizes.find((size) => adWidth == size.adWidth && adHeight == size.adHeight);

    return { width, height };
}

For your more general questions, if you have a chain of if statements where all (or even several) check the same thing (!IS_MOBILE and mediaType == "BANNER"), it is probably a good idea to pull that check out and test it by itself before checking the other conditions (dimensions in this case). It might even be worth pulling all of the checks after that condition into their own method. The guiding principle here is to avoid doing more than one thing at once.

As for making code more readable for junior developers, trying to avoid doing more than one thing at once will help a lot. Try to make it so that when you go back in 6 months and revisit the code, there is a clear path through it. The fewer things you have to remember to understand a line of code, or a method, the better.