//todo

What's left

For the slides

title
grammar check
timeline

For me

? cards for slides

1. Clean code Concepts: the historical view

2. Examples of violation

3. Table-driven methods

4. Refactoring example

// TODO

Introduction

Data is here

will have to include the timeline

The easiest way!

Your function is too long!

If it's more than n* lines you are probably breaking SRP.

Robert Martin in his book Clean Code says it should be no more that 3 lines, crazy?

It would be wrong to put an exact number on that phrase. For some of you, it might be 10 and for others 30.

function deleteUser(user) {
    const date = new Date();
    const freshTime = date.getTime() - 1000 * 60 * 60 * 24 * 30;
    const contracts = getUserContracts(user);

    if(user.lastLogin <  freshTime && contracts.length === 0) {
        const devices = getUserDevices(user);
        devices.forEach((device) => {
            device.deleted = 1;
        });
        user.deleted = 1;
    } else {
        const validationMsg = `Some text: ${user.name}`;
        const logService = getLogService();
        logService.warn(validationMsg);
    }
}

Blocks are not extracted

function checkAndDeleteUser(user) {
    if (validForDelete(user)) {
        deleteUser(user);
    } else {
        invalidUser(user);
    }
}

With extracted blocks

function delete(post) {
  try {
   api.doSomethingWithPost(post);
   registry.doSomethingWithPost(post.id);
   ...
   if(checkSomething(post)) {
       someService.doSomeOptionalStuff(post)
   }
  }
  catch (e) {
   logger.logError(e);
  }
}

Extract Try/Catch Blocks

function delete(post) {
  try {
   deletePostAndAllReferences(post);
  }
  catch (e) {
   logError(e);
  }
}

Extract Try/Catch Blocks

Error Handling Is One Thing

Functions should do one thing. Error handling is one thing. That is why a function that handles errors should do nothing else.

function doHousework()
{
    cleanTheFloor();
    pourFlowers();

    dogs.forEach((dog) => {
        if(dog.hungry())
        {
            prepareFood();
            feed(dog);
        }
    });
}

Several levels of abstraction!

function doHousework()
{
    cleanTheFloor();
    pourFlowers();
    feedDogs();
}

One level of abstraction!

function cleanTheFloorCallTaxiGetUserData() {
    //
}

With that name, you can be sure that it is doing too many things.

It's hard to name a function or a class.

function cleanTheFloorPourFlowersCleanTables() {
    //
}

If each action is a part of a bigger process you just need a good name for it.

function cleanTheHouse() {
    //
}

If you don't know the name for that process you should ask you domain expert.

If the name complicated you should add it to the ubiquitous language of your project.

Passing a boolean into a function is a truly terrible practice.

doSomething(false);

Flag Arguments

They can be very confusing

widget.next(true);

widget.destroy(false);

With false still destroys your widget, but it leaves the DOM associated with the widget intact.

True says that the very first child widget will be returned if you hit the last one.

Two boolean arguments can’t be more fun

menu.stop(true, false);

cmp.setCentered(true, false);

Signature is setCentered(centered, autoUpdate)

Values refer to clear the animation queue or not and go to the animation or not, respectively.

doSomething({noDelay: true});

What can we do?

doSomething({ mode: "no-delay" });

doSomethingWithDelay();

doSomething()

This change will require some refactoring.

doSomething() {
    this._doSomething(true);
}

doSomethingWithDelay() {
    this._doSomething(false);
}

You can start with just hiding your boolean logic.

To fully remove your boolean logic.

1. Duplicate the function

2. Remove boolean logic in both of them.

3. Extract all shared code.

But you should also consider. Would it be better if we use * to remove boolean logic

Decorator pattern?
Strategy pattern?

function cookSalad() {
    prepare('onion');
    prepare('broccoli');
    prepare('cabbage');
}

What is wrong?

DO NOT EVER COPY AND PASTE CODE

function cookSalad() {
    const vegetables = ['onion', 'broccoli', 'cabbage']

    vegetables.forEach(vegatable => { 
        prepare(ingredient);
    };
}

const ONION = 'onion';
const BROCCOLI = 'broccoli'
const CABBAGE = 'cabbage';

function cookSalad() {
    const onionAvailable = checkIfAvailable(ONION);
    const onion = onionAvailable ? 
                      kitchen.get(ONION) : 
                      warehouse.get(ONION);
    prepare(onion);

    const broccoliAvailable = checkIfAvailable(BROCCOLI);
    const broccoli = broccoliAvailable ? 
                      kitchen.get(BROCCOLI) : 
                      warehouse.get(BROCCOLI);
    prepare(broccoli);

    const cabbageAvailable = checkIfAvailable(CABBAGE);
    const cabbage = cabbageAvailable ? 
                      kitchen.get(CABBAGE) : 
                      warehouse.get(CABBAGE);
    prepare(cabbage);
}

function cookSalad() {
    const ingredients = ['onion', 'broccoli', 'cabbage'];

    ingredients.forEach(ingredientType => {
        const available = checkIfAvailable(ingredientType);
        const ingredient = available ? 
                          kitchen.get(ingredientType) : 
                          warehouse.get(ingredientType);
        prepare(ingredient);
    };
}

const associatedNodes = 
    nodes.filter(node => node.id !== activeNodeId);

With ES 2015

const associatedNodes = 
    nodes.filter(function (node) {
        return node.id !== activeNodeId;
    });

Business rules in anonymous function

const associatedNodes = nodes.filter(node => 
    (node.id !== activeNodeId && node.sectionId === sectionId));

This little arrow function is a business rule.

It decides which nodes are considered associated.

const associatedNodes = selectAssociatedNodes(nodes, activeNodeId, sectionId);

That way our rule will live in a separate function.

function isAssociatedNode(activeNodeId, activeSectionId, node) {

    const isNotSameAsActive = node.id !== activeNodeId;
    const isSameSectionAsActive = node.sectionId === activeSectionId;

    return isNotSameAsActive && isSameSectionAsActive;
}

export function selectAssociatedNodes(nodes, activeNodeId, activeSectionId) {
    return nodes
            .map(isAssociatedNode.bind(null, activeNodeId, activeSectionId));
}

function isEdible() {
    return (
        this.expirationData > Date.now() && 
        this.approvedByEFCA === true && 
        this.approvedByFDA === true &&
        this.inspectorId !== null
    );
}

Many reasons to change!

1. How to check expiration date

2. How to check if it is approved

3. How to validate inspector

4. How to check if it is edible

function isEdible() {
    return (
        this.expirationData > Date.now() && 
        // is food approved
        this.approvedEFCA === true && 
        this.approvedByFDA === true &&
        // Validate inspector
        this.inspectorId !== null
    );
}

We can comment it

It's a bit better, at least now we know the intent behind those checks.

function isEdible() {
    return (
        this.expirationData > Date.now() && 
        this.approvedByEFCA === true && 
        this.approvedByFDA === true &&
        this.FDA.inspectors.length > 0 &&
        this.inspectors.find(inspector => 
                    inspector.accreditedByEFCA === true) &&
        this.inspectors.find(inspector => 
                    inspector.accreditedByFDA === true)
    );
}

Imagine that, validation requirements have changed.
And we end up making changes in isEditable function, isn't this confusing?

function isEdible() {
    return (
        this.isFresh() && 
        this.isApproved() && 
        this.hasValidInspectors()
    );
}

One reasons to change!

1. How to check if it is edible

Table-driven methods.

Lookup table.

Dispatch table.

function getMessage(player) {
    if (player.daysPlayed === 0) {
        return `Let's try it!`;
    } else if (player.daysPlayed === 1) {
        return `Greatest experience!`;
    } else if (player.daysPlayed >= 2 && player.daysPlayed < 10) {
        return `Keep going!`
    }
    /* ... */
}

Lookup table

function getDaysMsgTable() {
    return {
        0: `Let's try it!`,
        1: `Greatest experience!`,
        2: `Keep going!`,
        3: `Keep going!`
        /* ... */
    };
}
function getMessage(msgTable, player) {
    return msgTable[player.daysPlayed];
}

export default getMessage.bind(null, getDaysMsgTable());

Lookup table

function getBenefit(user) {

    if (user.type = 'NOVICE') {
        return (user.hours * 0.1) * POWER.NOVICE;
    }
    else if (user.type = 'SILVER') {
        const silverBonus = user.weeks * 0.42;
        return ((user.hours * 0.2) + silverBonus) * POWER.SILVER;
    }
    else if (user.type = 'GOLD') {
        const goldBonus = (user.goldnesNumber * 42) / user.weeks;
        const bonusPower = goldBonus > 500 ? POWER.GOLD : POWER.GOLD_ULTRA;
        return (user.hours * 0.3 + goldBonus) * bonusPower;
    }
}

Dispatch table

const benefitStrategies = {
    'NOVICE': (user) => {
        return (user.hours * 0.1) * POWER.NOVICE;
    },
    'SILVER': (user) => {
        const silverBonus = user.weeks * 0.42;
        return ((user.hours * 0.2) + silverBonus) * POWER.SILVER;
    },
    'GOLD': (user) => {
        const goldBonus = (user.goldnesNumber * 42) / user.weeks;
        const bonusPower = goldBonus > 500 ? POWER.GOLD : POWER.GOLD_ULTRA;
        return (user.hours * 0.3 + goldBonus) * bonusPower;
    }
};

function getBenefit(user) {
    const benefitStrategy = benefitStrategies[user.type];
    return benefitStrategy(user);
}

Dispatch table

function activeSlide(i) {
    var index = 0;
    if (isFinite(i)) {
        index = i;
    } else {
        if (i === "next") {
            if (current < maxIndex) {
                index = current + 1;
            } else {
                index = 0;
            }
        }
        if (i === "prev") {
            if (current > 0) {
                index = current - 1;
            } else {
                index = maxIndex;
            }
        }
    }

    /* ... Do something with index */
}

Dispatch table ex2

const handlers = getHandlers();

function activeSlide(i) {
    const handler = handlers[getTableKey(i)];
    const activeIndex = handler(i);

    /* ... Do something with index */
}

function getTableKey(i) {
    if (isFinite(i)) {
        return 'index';
    }
    return i;
}

function getHandlers() {
    return {
        'index': i => i,
        'next': () => {
            if (current < maxIndex) {
                return current + 1;
            }
            return 0;
        },
        'prev': () => {
            if (current > 0) {
                return current - 1;
            }
            return maxIndex;
        }
    };
}

Dispatch table ex2

let rate = 0;
if (gender == Gender.Female) {
    if (maritalStatus == MaritalStatus.Single) {
        if (smokingStatus == SmokingStatus.NonSmoking) {
            if (age < 18) {
                rate = 200.00;
            } else if (age == 18) {
                rate = 250.00;
            } else if (age == 19) {
                rate = 300.00;
            } else if (65 < age) {
                rate = 450.00;
            }
            /* .... */
        } else {
            if (age < 18) {
                rate = 250.00;
            } else if (age == 18) {
                rate = 300.00;
            } else if (age == 19) {
                rate = 350.00;
            } else if (65 < age) {
                rate = 575.00;
            }
        }
    } else if (maritalStatus == MaritalStatus.Married) {
        /* .... */
    }
}

Example is taken from code complete.

For more information you can check.

Code complete by Steve McConnell.
Chapter 18. Table-Driven Methods

function onError(res, apiMethod, params) {
    res.data = res.data || {};

    normalizeResponse(res);

    switch (res.status) {
        // Wrong headers
        case 0:
            res.data.errorMessage = ErrorReasons.WrongHeader.description;
            Toast.notify(res.data.errorMessage, res.data.errors).show();

            return Promise.reject(ErrorReasons.WrongHeader);

        // Bad Request
        case 400:
            // Error handling should be processed by request caller.
            // Common error message for this case
            res.data.errorMessage = ErrorReasons.BadRequest.description;

            return Promise.reject(res);

        // Unauthorized
        case 401:
            res.data.errorMessage = ErrorReasons.Unauthorized.description;
            Toast.notify(res.data.errorMessage, res.data.errors).show();

            return Promise.reject(ErrorReasons.Unauthorized);

        // Forbidden
        case 403:
            return Toast.prompt().show();

        // Internal Server Error
        case 500:
            return new Promise((resolve, reject) => {
                Toast.prompt(res.data.errors)
                    .show()
                    .then(() => {
                        return apiMethod(params)
                            .then((val) => {
                                resolve(val);
                            })
                            .catch((res) => {
                                return onError(res, apiMethod, params)
                                    .then(resolve)
                                    .catch(reject);
                            });
                    })
                    .catch(() => reject(res));
            });

        // Not Found
        case 404:
            return new Promise(function(resolve, reject) {
                res.data.errorMessage = ErrorReasons.NotFound.description;
                Toast.notify(res.data.errorMessage, res.data.errors).show();

                reject(res);
            });
    }
}

Example 1

function onError(res, apiMethod, params) {
    res.data = res.data || {};

    normalizeResponse(res);

    switch (res.status) {
        // Wrong headers
        case 0:
            return handleWrongHeaders({res});
        // Bad Request
        case 400:
            return handleBadRequest({res});
        // Unauthorized
        case 401:
            return handleUnauthorized({res});
        // Forbidden
        case 403:
            return handleForbidden();
        // Internal Server Error
        case 500:
            return handleInternalServerError({res, apiMethod, params});
        // Not Found
        case 404:
            return handleNotFound({res});
    }
}

function handleWrongHeaders({res}) {
    res.data.errorMessage = ErrorReasons.WrongHeader.description;
    Toast.notify(res.data.errorMessage, res.data.errors).show();

    return Promise.reject(ErrorReasons.WrongHeader);
}

function handleBadRequest({res}) {
    // Error handling should be processed by request caller.
    // Common error message for this case
    res.data.errorMessage = ErrorReasons.BadRequest.description;

    return Promise.reject(res);
}

function handleUnauthorized({res}) {
    res.data.errorMessage = ErrorReasons.Unauthorized.description;
    Toast.notify(res.data.errorMessage, res.data.errors).show();

    return Promise.reject(ErrorReasons.Unauthorized);
}

function handleForbidden() {
    return Toast.prompt().show();
}

function handleInternalServerError({res, params, apiMethod}) {
    return new Promise((resolve, reject) => {
        Toast.prompt(res.data.errors)
            .show()
            .then(() => {
                return apiMethod(params)
                    .then((val) => {
                        resolve(val);
                    })
                    .catch((res) => {
                        return onError(res, apiMethod, params)
                            .then(resolve)
                            .catch(reject);
                    });
            })
            .catch(() => reject(res));
    });
}

function handleNotFound({res}) {
    return new Promise(function(resolve, reject) {
        res.data.errorMessage = ErrorReasons.NotFound.description;
        Toast.notify(res.data.errorMessage, res.data.errors).show();

        reject(res);
    });
}

Blocks Are Extracted

const STATUS = {
    WRONG_HEADERS: 0,
    BAD_REQUEST: 400,
    UNAUTHORIZED: 401,
    FORBIDDEN: 403,
    INTERNAL_SERVER_ERROR: 500,
    NOT_FOUND: 404
};

function onError(res, apiMethod, params) {
    // res.data = res.data || {};
    // has gone into normalizeResponse, seems reasonable place for normalization
    normalizeResponse(res);

    switch (res.status) {
        case STATUS.WRONG_HEADERS:
            return handleWrongHeaders({res});

        case STATUS.BAD_REQUEST:
            return handleBadRequest({res});

        case STATUS.UNAUTHORIZED:
            return handleUnauthorized({res});

        case STATUS.FORBIDDEN:
            return handleForbidden();

        case STATUS.INTERNAL_SERVER_ERROR:
            return handleInternalServerError({res, apiMethod, params});

        case STATUS.NOT_FOUND:
            return handleNotFound({res});
    }
}

function handleWrongHeaders({res}) {
    res.data.errorMessage = ErrorReasons.WrongHeader.description;
    Toast.notify(res.data.errorMessage, res.data.errors).show();

    return Promise.reject(ErrorReasons.WrongHeader);
}

function handleBadRequest({res}) {
    // Error handling should be processed by request caller.
    // Common error message for this case
    res.data.errorMessage = ErrorReasons.BadRequest.description;

    return Promise.reject(res);
}

function handleUnauthorized({res}) {
    res.data.errorMessage = ErrorReasons.Unauthorized.description;
    Toast.notify(res.data.errorMessage, res.data.errors).show();

    return Promise.reject(ErrorReasons.Unauthorized);
}

function handleForbidden() {
    return Toast.prompt().show();
}

function handleInternalServerError({res, params, apiMethod}) {
    return new Promise((resolve, reject) => {
        Toast.prompt(res.data.errors)
            .show()
            .then(() => {
                return apiMethod(params)
                    .then((val) => {
                        resolve(val);
                    })
                    .catch((res) => {
                        return onError(res, apiMethod, params)
                            .then(resolve)
                            .catch(reject);
                    });
            })
            .catch(() => reject(res));
    });
}

function handleNotFound({res}) {
    return new Promise(function(resolve, reject) {
        res.data.errorMessage = ErrorReasons.NotFound.description;
        Toast.notify(res.data.errorMessage, res.data.errors).show();

        reject(res);
    });
}

Removed magic numbers

Removed redundant comments

const STATUS = {
    WRONG_HEADERS: 0,
    BAD_REQUEST: 400,
    UNAUTHORIZED: 401,
    FORBIDDEN: 403,
    INTERNAL_SERVER_ERROR: 500,
    NOT_FOUND: 404
};

function onError(res, apiMethod, params) {
    normalizeResponse(res);

    const status = res.status;
    const handlersMap = getDefaultHandlers();

    if (handlersMap.hasOwnProperty(status)) {
        return handlersMap[status]({res, apiMethod, params})
    }
}

function getDefaultHandlers() {
    return {
        [STATUS.WRONG_HEADERS]: handleWrongHeaders,
        [STATUS.BAD_REQUEST]: handleBadRequest,
        [STATUS.UNAUTHORIZED]: handleUnauthorized,
        [STATUS.FORBIDDEN]: handleForbidden,
        [STATUS.INTERNAL_SERVER_ERROR]: handleInternalServerError,
        [STATUS.NOT_FOUND]: handleNotFound
    };
}

function handleWrongHeaders({res}) {
    res.data.errorMessage = ErrorReasons.WrongHeader.description;
    Toast.notify(res.data.errorMessage, res.data.errors).show();

    return Promise.reject(ErrorReasons.WrongHeader);
}

function handleBadRequest({res}) {
    // Error handling should be processed by request caller.
    // Common error message for this case
    res.data.errorMessage = ErrorReasons.BadRequest.description;

    return Promise.reject(res);
}

function handleUnauthorized({res}) {
    res.data.errorMessage = ErrorReasons.Unauthorized.description;
    Toast.notify(res.data.errorMessage, res.data.errors).show();

    return Promise.reject(ErrorReasons.Unauthorized);
}

function handleForbidden() {
    return Toast.prompt().show();
}

function handleInternalServerError({res, params, apiMethod}) {
    return new Promise((resolve, reject) => {
        Toast.prompt(res.data.errors)
            .show()
            .then(() => {
                return apiMethod(params)
                    .then((val) => {
                        resolve(val);
                    })
                    .catch((res) => {
                        return onError(res, apiMethod, params)
                            .then(resolve)
                            .catch(reject);
                    });
            })
            .catch(() => reject(res));
    });
}

function handleNotFound({res}) {
    return new Promise(function(resolve, reject) {
        res.data.errorMessage = ErrorReasons.NotFound.description;
        Toast.notify(res.data.errorMessage, res.data.errors).show();

        reject(res);
    });
}

Extract handlers

function onError(res, apiMethod, params, handlers = {}) {
    const handlersMap = {
        ...getDefaultHandlers(),
        ...handlers
    };

    const {status} = res;

    if (handlersMap.hasOwnProperty(status)) {
        return handlersMap[status]({res, apiMethod, params})
    }
}

Is it one responsibility?

function onError(res, apiMethod, params) {
    const handlersMap = getDefaultHandlers();

    const {status} = res;
    const handle = getHandlerByStatus(handlersMap, status);
    return handle({res, apiMethod, params});
}

function getHandlerByStatus(handlersMap, status) {
    if (handlersMap.hasOwnProperty(status)) {
        return handlersMap[status];
    }

    if (handlersMap.hasOwnProperty(STATUS.DEFAULT)) {
        return handlersMap[STATUS.DEFAULT];
    }

    return noop;
}

function onError(res, apiMethod, params, handlers) {
    const {status} = res;
    const handle = getHandlerByStatus(handlers, status);
    return handle({res, apiMethod, params});
}

That problem looks boring for you?

Your super intellect is wasted on your customer's silly needs?

We have a solution for you: Gold plating

Requirements gold-plating
Developer gold-plating

Gold plating is done by team members to show their abilities, or by the project manager to the make client happy

In programming more known as YAGNI one of XP practices.

While there is certainly plenty of over-engineered, over-abstracted, gold-plated software out there, it's far more common to have.. pure lead.

If you have a developer who is going the extra mile and gold-plating stuff, count yourself lucky. With a bit of direction and focus you could have a developer producing real gold eventually. And that is rare indeed.

Quote from an article by Jeff Atwood

Is this a joke?

As for me there are 3 kinds of programmers.

Who is happy with ...

Adding a lot of code to a code base.
Removing a lot of bad code from a code base.
Preventing bad code from being written.

not ready

By Semyon Radionov

not ready

Semyon Radionov

ua.linkedin.com/in/semyon-radionov-4930a780

//todo

Your function is too long!

Blocks are not extracted

With extracted blocks

Extract Try/Catch Blocks

Extract Try/Catch Blocks

Several levels of abstraction!

One level of abstraction!

With that name, you can be sure that it is doing too many things.

It's hard to name a function or a class.

If each action is a part of a bigger process you just need a good name for it.

Flag Arguments

What can we do?

What is wrong?

DO NOT EVER COPY AND PASTE CODE

With ES 2015

Business rules in anonymous function

Many reasons to change!

We can comment it

One reasons to change!

Table-driven methods.

Lookup table.

Dispatch table.

Lookup table

Lookup table

Dispatch table

Dispatch table

Dispatch table ex2

Dispatch table ex2

not ready

More from Semyon Radionov