4
\$\begingroup\$

Noobie here. I applied for a job as a Jr. engineer and they gave me this task. After they read through my solution they rejected my application because the code was hard to follow. I'd like to get an idea of what they meant by that and how I can improve the approach I took. The full assignment is below.

Write a program that processes the included example file that contains people's names, outputs some statistics and generates updated output.

Input:

An arbitrary file with the same format as the attached example file. Your program will be tested on much (e.g. 1000x) larger files.

Example lines from file:

Graham, Mckenna -- ut
    Voluptatem ipsam et at.
Marvin, Garfield -- non
    Facere et necessitatibus animi.
McLaughlin, Mariah -- consequatur
    Eveniet temporibus ducimus amet eaque.
Lang, Agustina -- pariatur
    Unde voluptas sit fugit.
Bradtke, Nikko -- et
    Maiores ab officia sed.
Adams, Luis -- error
    Repellendus alias officia amet et perspiciatis.
Lehner, Matilde -- nesciunt
    Incidunt et ut necessitatibus porro.
Ortiz, Anita -- fuga
    Tempore eos et hic.
Koch, Berry -- vel
    Laborum perferendis inventore eveniet.
Cartwright, Nicolas -- et
    Optio aliquid earum exercitationem vitae fugit.
Fisher, Elmo -- non
    Ipsum provident nobis explicabo voluptas ipsa aperiam.

Output:

  1. The unique count of full, last, and first names (i.e. duplicates are counted only once)

  2. The ten most common last names (the names and number of occurrences)

  3. The ten most common first names (the names and number of occurrences)

  4. A list of 25 completely unique names (using names within the file)

  5. A list of 25 modified names (must be new names made up of the 25 names from list number 4. Must be made by switching first and last names)

var fs = require('fs');
var async = require('async');
var readline = require('readline');
var prompt = require('prompt');
var fullNameHash = {};
var firstNameHash = {};
var lastNameHash = {};

var listSizeN = [];
var rejectList = [];

var howMany = 25;
var fileName = 'yesware-data.txt';
var getUserInput = function(cb) {
    prompt.start();

    prompt.get(['fileName', 'listLength'], function (err, result) {
    console.log('  fileName: ' + result.fileName);
    fileName = result.fileName;

    console.log('  listLength: ' + result.listLength);
    howMany = result.listLength;

    console.log(fileName);
    cb(result);
  });
};

var readFile = function (processLine, callback) {
     var lineReader = readline.createInterface({
      input: fs.createReadStream(fileName)
    });

    lineReader.on('line', function (line) {
        processLine(line, lineReader);

    });

    lineReader.on('close', function() {
        if (callback) callback();
    });
};

var buildListSizeN = function(line, listSizeN) {
        var fullName = parseFullName(line);

        if(fullName){
            fullName = fullName.trim();

            splitNames(fullName, function(fullName) {
                var firstOrLastNotInArray = (!isInArray(fullName[0], listSizeN) && !isInArray(fullName[1], listSizeN));
                var firstOrLastInRejectList = (isInArray(fullName[0], rejectList) || isInArray(fullName[1], rejectList));

                if (firstOrLastNotInArray && !firstOrLastInRejectList) {
                        var fullNameString = fullName[1] + " " + fullName[0]; // FirstName + lastName
                        listSizeN.push(fullNameString);
                } else {
                    var fullNameString = fullName[1] + " " + fullName[0]; // FirstName + lastName
                    rejectList.push(fullNameString);
                }
            });
        }
        return listSizeN;
};

var isInArray = function(target, list) {
    if (typeof list === 'object'){
        list = JSON.stringify(list);
    }

    if (list.indexOf(target) > -1) {
        return true;
    }
        return false;
};

var getNamesWithCounts = function(list, HashWithCounts) {
    var namesWithCounts = {};
    list.forEach(function(name) {
        var count = HashWithCounts[name];
        namesWithCounts[name] = count;
    });
    return namesWithCounts;
};


var getFirstTen = function(list) {
    return list.slice(0,10);
};

var parseFullName = function(line) {
        var ifDashes = line.indexOf('--');

        if (ifDashes > 0 && line){
            var fullName = line.substring(0, line.indexOf('--'));
            var firstName = fullName.split(' ').slice(0, -1).join(' ');
            var lastName = fullName.split(' ').slice(-1).join(' ');
            return firstName + lastName;
        }
    };

var splitNames = function(fullName, cb) {
    if (fullName) {
        var fullNameArray = fullName.split(', ');
        cb(fullNameArray);
    }
};

// Input will be ex. ['Morales', 'Sua']
var hashFirstLastandFullNames = function(fullNameArray) {
 var firstName = fullNameArray[1];
 var lastname = fullNameArray[0];

 if (lastname in fullNameHash) {
     fullNameHash[lastname].push(firstName);
 } else {
    fullNameHash[lastname] = [firstName];
 }

 if (firstName in firstNameHash) {
        firstNameHash[firstName] = firstNameHash[firstName] +=1;
 } else {
        firstNameHash[firstName] = 1;
 }

 if (lastname in lastNameHash) {
     lastNameHash[lastname] = lastNameHash[lastname] +=1;
 } else {
    lastNameHash[lastname] = 1;
 }
};

var sizeOfObject = function(object) {
    return Object.keys(object).length;
};

var sortObjectByValue = function(object) {
    var keys = [];

        Object.keys(object)
    .map(function (key) { return [key, object[key]]; })
    .sort(function (first, second) {
            return second[1] - first[1];
    })
    .forEach(function (each) {
       keys.push(each[0]);
    });
    return keys;
    };


var logResults = function(firstNameHashSize,lastNameHashSize,fullNameHashSize,uniqueList, modifiedList){
        console.log("The unique count of full names: ", firstNameHashSize);
        console.log("The unique count of last names: ", lastNameHashSize);
        console.log("The unique count of first names: ", fullNameHashSize);
        console.log("**************************************************************");

        var mostCommonFirstNames = sortObjectByValue(firstNameHash);
        var firstTenFNames = getFirstTen(mostCommonFirstNames);
        console.log("10 Most Common First Names: ", getNamesWithCounts(firstTenFNames,firstNameHash));

        var mostCommonLastNames = sortObjectByValue(lastNameHash);
        var firstTenLNames = getFirstTen(mostCommonLastNames);
        console.log("10 Most Common Last Names: ", getNamesWithCounts(firstTenLNames,lastNameHash));

        console.log("\n A list of " + howMany + " completely unique names: " + JSON.stringify(uniqueList, null, 2));
        console.log("\nA list of " + howMany + " modified names: " +JSON.stringify(modifiedList, null, 2));
};

var createUniqueListFromFile = function(cb) {
        var uniqueList = [];
        readFile(
            function (line, lineReader) {
                uniqueList = buildListSizeN(line, listSizeN);

                if (uniqueList.length < howMany) {
                    // do nothing
                } else {                    
                    lineReader.close();
                }
            }, 
            function(){
                cb(uniqueList);
            });
    };

var swapFullNames = function(nameOne, nameTwo) {
    nameOne = nameOne.split(' ');
    nameTwo = nameTwo.split(' ');

    var newNameOne = nameOne[0] + " " + nameTwo[1];
    var newNameTwo = nameTwo[0] + " " + nameOne[1];

    return [newNameOne, newNameTwo];
};

var swapNamesInList = function(list){
    var listLength = list.length;

    for (var i = 0; i < listLength; i++) {
        if (listLength[i] && list[i+1]) {
            var newNames = swapFullNames(list[i], list[i+1]);

            list[i] = newNames[0];
            list[i+1] = newNames[1];

            i++;
        } else {
            // do when it's the last item 
            var newNames = swapFullNames(list[i], list[0]);
            list[i] = newNames[0];
            list[0] = newNames[1];
        }
    }
    return list;
};

var main = (function() {
    async.series([
            function(callback) {
                console.log("START");

                getUserInput(function(){
                    callback();
                });
            }, 
            function(callback) {
                var sizeOfFirstNames;
                var sizeOfLastNames;
                var sizeOfFullNames;

                readFile(
                    function (line) {
                    var fullName = parseFullName(line);
                    splitNames(fullName, function(fullName) {
                        hashFirstLastandFullNames(fullName);
                    });
                }, function(){
                    sizeOfFirstNames = sizeOfObject(firstNameHash);
                    sizeOfLastNames = sizeOfObject(lastNameHash);
                    sizeOfFullNames = sizeOfObject(fullNameHash);

                    callback(null, sizeOfFirstNames, sizeOfLastNames, sizeOfFullNames);

                });
            },
            function(callback){
                createUniqueListFromFile(function(uniqueList){
                    var tempUniqueList = uniqueList.slice(0);
                    var modifiedNames = swapNamesInList(tempUniqueList);
                    callback(null, uniqueList, modifiedNames);
                });
            }],
            function(err, result) {
                result.shift();
                var listSizes = result[0];
                var firstNameHashSize = listSizes[0];
                var lastNameHashSize = listSizes[1];
                var fullNameHashSize = listSizes[2];

                var uniqueListAndModifiedList = result[1];
                var uniqueList = uniqueListAndModifiedList[0];
                var modifiedList = uniqueListAndModifiedList[1];

                logResults(firstNameHashSize,lastNameHashSize,fullNameHashSize,uniqueList, modifiedList);

            });
\$\endgroup\$
  • \$\begingroup\$ What's the format of the input? One name per line? Or something else? You seem to have some code to split things on --, but read line by line, so it's a little confusing. Can you add a few lines from the example file? \$\endgroup\$ – Flambino Oct 14 '16 at 23:45
  • \$\begingroup\$ @Flambino added \$\endgroup\$ – Sua Morales Oct 15 '16 at 1:09
2
\$\begingroup\$

The essential issue here is that there's just too much code. The complexity of the problem doesn't merit over 200 lines of code.

You're transforming some regularly formatted text into a list of names. Then answering some questions about those lists.

Critiquing your code line by line just won't be that helpful in this case, because, although there are some things to improve, what you really want to do is just take a simpler approach at a high level. Here's one (of many) ways you might do that:

// a few object utilities
///////////////////////////
const length = o => Object.keys(o).length;
const byCount = (a, b) => b[1] - a[1];
const top10 = o => Object.entries(o).sort(byCount).map(x => x[0]).slice(0,10);

function addItem(countObj, key) {
  const newKey = (countObj[key] === undefined);
  if (newKey) countObj[key] = 0;
  countObj[key]++;
}    
// get the name counts
///////////////////////////
const lineReader = require('readline').createInterface({
  input: require('fs').createReadStream('names.txt')
});

const [firstNames, lastNames, fullNames] = [{}, {}, {}];

lineReader.on('line', function (line) {
  const matches = line.match(/(.*), (.*) -- .*/);
  if (!matches) return;
  const [_, last, first] = matches;

  addItem(firstNames, first);
  addItem(lastNames, last);
  addItem(fullNames, first + ' ' + last);
});   
// print results
///////////////////////////   
lineReader.on('close', function (line) {
  console.log('Unique first: ', length(firstNames));
  console.log('Unique last: ' , length(lastNames));
  console.log('Unique full: ' , length(fullNames));
  console.log('Top 10 first: ', top10(firstNames));
  console.log('Top 10 last: ' , top10(lastNames));
});

Note that the data structure I'm using here is a plain object, whose keys are the names and whose values are the number of times that name appears.

I'll leave the final questions as an exercise, because it wasn't entirely clear to me what those questions were asking.

\$\endgroup\$
  • \$\begingroup\$ okay thanks. So it sounds like I had my head in the right place (specifically related to the modularity of the code - each function doing 1 thing) but I could have done the same thing with much less code. \$\endgroup\$ – Sua Morales Oct 15 '16 at 19:21
  • \$\begingroup\$ yes, creating simple functions that do one thing, and have a clear name, are one of the most important tools for breaking a problem down. and it's good that you're doing that. but as you can see that alone won't save you if you don't have the right high-level solution to the problem. \$\endgroup\$ – Jonah Oct 15 '16 at 19:26

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.