3
\$\begingroup\$

I solved an exercise from the book "Eloquent Javascript". I want to share with you guys the solution. The exercise is from chapter 5. Summing up, I have to find the average age from the ancestors array per century. Here is my solution:

var ancestry = JSON.parse(require('../resources/ancestry'))

//Top-down approach, functions are defined after their use
//Necessary side-effect in function to print the output
function printAverageAgePerCentury(ancestry){
    var averageAgePerCentury = getAverageAgePerCentury(ancestry);
    Object.keys(averageAgePerCentury).forEach(function(century){
        console.log(century.concat(": ").concat(averageAgePerCentury[century]))
    })
}

function getAverageAgePerCentury(ancestry){
    var agesPerCentury = getAgesPerCentury(ancestry); 
    return agesPerCentury.reduce(function(averageAgePerCentury, agePerCentury){
        if(averageAgePerCentury[agePerCentury.century]){
            averageAgePerCentury[agePerCentury.century] = [average(averageAgePerCentury[agePerCentury.century].concat(agePerCentury.age))];
            return averageAgePerCentury;
        } else {
            averageAgePerCentury[agePerCentury.century] = [agePerCentury.age];
            return averageAgePerCentury;
        }
    }, {})
}

function getAgesPerCentury(ancestry){
    return ancestry.map(function(person){
        return {
            century : whichCentury(person),
            age : person.died - person.born
        }
    })
}

function average(array){
    function plus(a,b){ return a + b}
    return array.reduce(plus) / array.length
}

function whichCentury(person){
    return Math.ceil(person.died / 100)
}

//--------------------------------//

printAverageAgePerCentury(ancestry);
  1. What do you think about the top-down approach? Do you prefer the opposite? Where functions are defined before their use.
  2. Is possible to make this solution more readable, more elegant?
  3. What could you do to make it better?
\$\endgroup\$
2
\$\begingroup\$

Incorrect calculation of average

average([1, 2, 3, 4]) is \$\dfrac{1+2+3+4}{4}=2.5\$.

However, due to the way you call reduce() in getAverageAgePerCentury():

averageAgePerCentury[agePerCentury.century] = [average(averageAgePerCentury[agePerCentury.century].concat(agePerCentury.age))];

… you would actually calculate average([average([average([average([1]), 2]), 3]), 4]), which would produce a result of 3.125 — weighted in favour of the later entries.

Doing too much

A root cause of the bug, I think, is that getAverageAgePerCentury() is doing a lot of work. It creates a map of centuries to the age of people who died in that century, and calculates the average of those values.

I would define two utility functions, inspired by Lodash's _.mapValues() and _.groupBy().

/**
 * Creates a new object in which all of the original obj's values are
 * transformed by callback(value).
 */
function mapValues(obj, callback) {
    return Object.keys(obj).reduce(function(result, key) {
        result[key] = callback(obj[key]);
        return result;
    }, {});
}

/**
 * Given an array of objects, creates a new object in which all objects where
 * attrExtractor(obj) having the same value are grouped together in an array.
 */
function groupBy(objs, attrExtractor) {
    return objs.reduce(function(result, obj) {
        var attr = attrExtractor(obj);
        if (attr in result) {
            result[attr].push(obj);
        } else {
            result[attr] = [obj];
        }
        return result;
    }, {});
}

Then, the rest of the solution looks simpler.

function average(array) {
    return array.reduce(function(a, b) { return a + b }) / array.length;
}

function averageAgePerCentury(ancestry) {
    function deathCentury(person) { return Math.ceil(person.died / 100) }
    function deathAge(person) { return person.died - person.born }

    return  mapValues(
                mapValues(
                    groupBy(ancestry, deathCentury),
                    function(persons) { return persons.map(deathAge) }
                ),
                average
            );
}

Note that I've renamed getAverageAgePerCentury(). A "getter" function should retrieve something that already exists. Here, you're doing a calculation. You call Math.sin(x), not Math.getSin(x), so the same naming principle applies here.

\$\endgroup\$
0
\$\begingroup\$

Eloquent

Eloquent is simple, efficient in both processing and memory use, easy to understand, and adaptable.

Simplicity

Don't store unneeded data it adds complexity.

The problem is looking for averages grouped by century. That means that there is no need to store any persons age, only the sum of ages for each century and the count of deaths are needed for the solution

Efficacy

Doing the minimum work possible.

To get a result requires, one pass to group, sum, and count the data. Then one pass over the centuries to display the results. With 39 persons and 6 centuries. the number of iterations should only be 45.

Your solution passes over the data 118 times over 2 times more than needed. The solution by 200_success improves that a little with 90 iterative steps, twice as many as needed.

Understandable.

Code should be easy to follow.

In the old days of coding there was a style of code commonly referred to as spaghetti code. To understand it you had to follow a complicated path of processing. Each time you jump from one section of source code to the next, it breaks the logic flow stored in your head. Following disjointed source code is hard.

Now we have named functions. The idea is that the name of the function gives all the information needed to follow the logic without having to step into the function.

This is seldom how it works, and tracing functions because the function name lacks the information to understand the process is a modern form of spaghetti code.

Adaptability

Time is money, and good code means that it is reusable with the minimum of effort. How long would it take to get only male or female averages. Do you need to think about it, or is the solution already in the code and you know instantly what to add, or change.

Can the code handle bad data, can the code handle vary large data sets. What needs to change to make it a distributed solution.

An Eloquent solution.

Minimal iteration passes, efficient with no irrelevant data stored, and only 45 iterations.

Understandable, not a sea of nested function calls, Logic can be followed fairly much from top to bottom without needing to step into disjointed inadequately named function calls.

Could be a little simplier but I went for adaptability over simplicity. The generateGroupKey allows a variety of groupings.

The output function means it is easily be formatted to any type of visualization, storage, or additional processing, Ancestry data can be randomly split and processing distributed with output providing a collation service.

No side effects and pure as Snow White.

const groupByCentury = person => (person.died / 100 | 0) + 1;

groupAndAverageAges(ancestry, groupByCentury, console.log);

function groupAndAverageAges(ancestry, generateGroupKey, output) {
    const groups = new Map();

    for (const person of ancestry) {
        const group = generateGroupKey(person);
        var stats = groups.get(group);
        if (stats === undefined) {
            stats = {group : group, sum : 0, count : 0};
            groups.set(group, stats);
        }
        stats.sum += person.died - person.born;
        stats.count += 1;
    }

    for (const group of groups.values()) {
        output({
            grouping : group.group, 
            meanAge : Math.round(group.sum / group.count)
        });
    }    
}

@Aaron Goldsmith thanks for the edit suggestion but I have a style rule that is no object property short hand on the same line as standard properties. So I leave the line as is rather than break it up.

An alternative.

To accommodate points made in the comments we can leave the responsibility of what to do with the information generated by the function to the calling context.

This version returns an array with an item for each century.

const groupByCentury = person => (person.died / 100 | 0) + 1;

const meanAgeGroupedByCentury = groupAndAverageAges(ancestry, groupByCentury);

function groupAndAverageAges(ancestry, generateGroupKey) {
    const groups = new Map();
    for (const person of ancestry) {
        const groupKey = generateGroupKey(person);
        let stats = groups.get(groupKey);
        if (! stats) {
            stats = {group : groupKey, sum : 0, count : 0};
            groups.set(groupKey, stats);
        }
        stats.sum += person.died - person.born;
        stats.count += 1;

    }
    return [...groups.values()].map(group => ({
            group : group.group, 
            averageAge : Math.round(group.sum / group.count)
        })
    );
}
\$\endgroup\$
  • \$\begingroup\$ I'm not a fan of the output callback. To me, it violates the Single Responsibility Principle. It also makes it harder to use the results for further analysis. \$\endgroup\$ – 200_success Feb 3 '18 at 11:42
  • \$\begingroup\$ I think that you greatly enhanced the discussion. Your solution is more resource use efficient. I would use yours if I have constraints like a real time application or a in IOT device with reduced memory. \$\endgroup\$ – tiagolisalves Feb 3 '18 at 19:52
0
\$\begingroup\$

I refactored the functions to be shorter and ES6, and changed some formatting to make it easier to read. You might also want to consider making some of your variable and function names shorter. Also, not sure the ordering you have for the functions is necessary.

const ancestry = JSON.parse(require('../resources/ancestry'));

const average = (array) => array.reduce((a,b) => a + b) / array.length;
const whichCentury = (person) => Math.ceil(person.died / 100);

const printAverageAgePerCentury = (ancestry) => {
  const averageAgePerCentury = getAverageAgePerCentury(ancestry);
  Object.keys(averageAgePerCentury).forEach((century) => {
    console.log(century.concat(": ").concat(averageAgePerCentury[century]));
  });
};

const getAgesPerCentury = (ancestry) => {
  return ancestry.map((person) => {
    return {
      century : whichCentury(person),
      age : person.died - person.born
    };
  });
};

const getAverageAgePerCentury = (ancestry) => {
  return getAgesPerCentury(ancestry).reduce((averageAgePerCentury, agePerCentury) => {
    if (averageAgePerCentury[agePerCentury.century]) {
      averageAgePerCentury[agePerCentury.century] = [average(averageAgePerCentury[agePerCentury.century].concat(agePerCentury.age))];
      return averageAgePerCentury;
    } else {
      averageAgePerCentury[agePerCentury.century] = [agePerCentury.age];
      return averageAgePerCentury;
    }
  }, {});
}
\$\endgroup\$
  • \$\begingroup\$ The ordering is about to read the program like a history. You read the function A that calls another function B and search for function B definition right below the function A, like when you're reading a book and don't know the definition of a word. So, you separate the abstraction level of a function in a top-down way, to ease the readability. What do you think? \$\endgroup\$ – tiagolisalves Feb 3 '18 at 19:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.