What's left
For the slides
For me
1. Clean code Concepts: the historical view
2. Examples of violation
3. Table-driven methods
4. Refactoring example
The easiest way!
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);
}
}
function checkAndDeleteUser(user) {
if (validForDelete(user)) {
deleteUser(user);
} else {
invalidUser(user);
}
}
function delete(post) {
try {
api.doSomethingWithPost(post);
registry.doSomethingWithPost(post.id);
...
if(checkSomething(post)) {
someService.doSomeOptionalStuff(post)
}
}
catch (e) {
logger.logError(e);
}
}
function delete(post) {
try {
deletePostAndAllReferences(post);
}
catch (e) {
logError(e);
}
}
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);
}
});
}
function doHousework()
{
cleanTheFloor();
pourFlowers();
feedDogs();
}
function cleanTheFloorCallTaxiGetUserData() {
//
}
function cleanTheFloorPourFlowersCleanTables() {
//
}
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);
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});
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
function cookSalad() {
prepare('onion');
prepare('broccoli');
prepare('cabbage');
}
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);
const associatedNodes =
nodes.filter(function (node) {
return node.id !== activeNodeId;
});
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
);
}
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
);
}
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()
);
}
1. How to check if it is edible
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!`
}
/* ... */
}
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());
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;
}
}
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);
}
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 */
}
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;
}
};
}
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
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.
Is this a joke?
As for me there are 3 kinds of programmers.
Who is happy with ...