5
\$\begingroup\$

I am Learning Javascript, after doing Java development for 2 years. I am doing a module on udemy which had a coding challenge which I completed and it works but wanted to get it reviewed to find out best practices and how the code can be improved and optimised.

Here is the Challenge:

CODING CHALLENGE

Suppose that you're working in a small town administration, and you're in charge of two town elements:

  1. Parks
  2. Streets

It's a very small town, so right now there are only 3 parks and 4 streets. All parks and streets have a name and a build year.

At an end-of-year meeting, your boss wants a final report with the following:

  1. Tree density of each park in the town (forumla: number of trees/park area) √
  2. Average age of all of the town's parks (forumla: sum of all ages/number of parks) √
  3. The name of the park that has more than 1000 trees √
  4. Total and average length of the town's streets √
  5. Size classification of all streets: tiny/small/normal/big/huge. If the size is unknown, the default is normal √

All the report data should be printed to the console. √

HINT: Use some of the ES6 features: classes, subclasses, template strings, default parameters, maps, arrow functions, destructuring, etc.

My code is:

class NameAndBuildYear {
    constructor(name, buildYear) {
        this.name = name;
        this.buildYear = buildYear;
    }
}

class Park extends NameAndBuildYear {
    constructor(name, buildYear, numOfTrees, parkArea) {
        super(name, buildYear);
        this.numOfTrees = numOfTrees;
        this.parkArea = parkArea;

    }
    calculateTreeDensity() {
        return this.numOfTrees / this.parkArea
    }

    calculateAgeOfTree() {
        let age = new Date().getFullYear() - this.buildYear;
        return age;

    }

}

class Street extends NameAndBuildYear {
    constructor(name, buildYear, streetLength, size = 'normal') {
        super(name, buildYear);
        this.streetLength = streetLength;
        this.size = size;

    }

}

let mordernPark = new Park('Mordern Park', 1909, 100, 0.2);
let tootingPark = new Park('Tooting Park', 1950, 1000, 0.4);
let balhamPark = new Park('Balham Park', 1800, 600, 2.9);

let parkArray = [mordernPark, tootingPark, balhamPark];

function calculateAverageAgeOfPark() {
     var sumAge = 0;
    parkArray.forEach((element) => {
        sumAge += element.calculateAgeOfTree();
    });
    console.log(`The Average age of the the towns park is ${Math.round(sumAge / parkArray.length)} years`);
}


function calculateTreeDensityOfEachPark() {
    for (const cur of parkArray) {
        console.log(`The Tree Desnity of ${cur.name} is the density is ${Math.round(cur.calculateTreeDensity())} per square km`);
    }
}

calculateTreeDensityOfEachPark();
calculateAverageAgeOfPark();

let numberOfTreesMap = new Map();
numberOfTreesMap.set(mordernPark.name, mordernPark.numOfTrees);
numberOfTreesMap.set(tootingPark.name, tootingPark.numOfTrees);
numberOfTreesMap.set(balhamPark.name, balhamPark.numOfTrees);

for (let [key, value] of numberOfTreesMap.entries()) {
    if (value >= 1000) {
        console.log(`${key}: has a  ${value} trees`);
    }
}

const faylandAvenue = new Street('Fayland Avenue', 1982, 200);
const tootingBroadWay = new Street('Tooting Broadway', 1908, 1000, 'huge');
const penworthamRoad = new Street('Penwortham Road', 1950, 300, 'big');
const mertonRoad = new Street('Merton Road', 1932, 100, 'small');

let streetMap = new Map();
streetMap.set(faylandAvenue.name, faylandAvenue.streetLength);
streetMap.set(tootingBroadWay.name, tootingBroadWay.streetLength);
streetMap.set(penworthamRoad.name, penworthamRoad.streetLength);
streetMap.set(mertonRoad.name, mertonRoad.streetLength);

function calculateTotalAndAverageStreetLength() {
    let totalStreetLength = 0;
    for (let [key, value] of numberOfTreesMap.entries()) {
        totalStreetLength += value;
    }
    console.log(`The total length of the town's streets is ${totalStreetLength} meters and the average length is ${totalStreetLength / streetMap.size} meters`);
}

calculateTotalAndAverageStreetLength();


let streetArray = [faylandAvenue, tootingBroadWay, penworthamRoad, mertonRoad];

streetArray.forEach((element) => {
    console.log(`The size of ${element.name} is ${element.size}`);
});
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ – Toby Speight Apr 24 at 14:49
  • \$\begingroup\$ @TobySpeight No the edit is fine \$\endgroup\$ – user6248190 Apr 24 at 15:03
  • \$\begingroup\$ I edited the challenge so that it is easier readable. Since it is human-readable text, in should not be formatted like code. \$\endgroup\$ – Roland Illig Apr 24 at 20:12
5
\$\begingroup\$

Java to JavaScript

From Java to JavaScript is one of that hardest ways to move into JS. Appart from a similar C style syntax they are very different.

Some things to know

  • Javascript does not have classes, interfaces, or classical inheritance. The class syntax is just an alternative way of defining objects.

    The class syntax has some serious problems (no way to define private properties (new syntax is on the way) and encourages poor encapsulation styles) and should be avoided until it has stabilized.

  • this is not the same as Javas this be careful when and how you use it.

  • Objects do not have private properties. Encapsulation is via closure. However properties can be protected via the using the Object properties and functions.

  • Javascript Objects inherit via a prototype chain. All objects have Object at the top of the prototype chain.

  • Apart from the primitives boolean, undefined, null, number, string and symbol, all else are objects (and the primitives can be objects to).

  • Javascript can be run in two modes. strict mode is the preferred mode.

MDN main javascript page is a good reference, use it if you need any more information regarding this answer. I will add extra links only if not at MDN

The code

The code is a mess.

  • The following variables should all be declared using the keyword const instead of let: mordernPark, tootingPark, balhamPark, parkArray, numberOfTreesMap, streetMap, and streetArray I may have missed some.

  • Define variables and constants in one place. Do create them as you need them.

  • You define functions for some parts of the report yet in others you just have inline code. Ideally you create a function for each part, and group them all in one object. (see example. The object report has all the functions)

  • Good names should try to be under 20 characters long and contain no more than two distinct words. from Naming guidelines for professional programmers

    An example is the name calculateTotalAndAverageStreetLength is way to verbose. If you add this to an object called report you can name it report.streetLengths

  • Keep names meaningful. Don't add type to the name. eg parkArray.forEach((element) => { should be parks.forEach(park => {

  • Rather than send all report text hard coded to the console, have the functions push them to an array and return that array. You can then send that array where you need.

  • Use getters and setters when getting or setting calculated properties.

  • You have a super class NameAndBuildYear to hold the name and build year, yet you do not define a function to get the age in that object, rather you put it in the parks object. It should be with the NameAndBuildYear object.

  • Don't over complicate the code. You have named variables for each street, and array for all the streets and a map of streets by street name. Yet the street object holds the name already, why duplicate the names so many times?

Rewrite

Keeping a similar structure (and still using the class syntax (cringe...)) the rewrite uses the above points to make your code more manageable.

I have renamed the main class to Assets as it seams more fitting.

"use strict";
class Asset {
    constructor(name, built) {
        this.name = name;
        this.built = built;
    }
    get age() { return new Date().getFullYear() - this.built }
}
class Park extends Asset {
    constructor(name, built, trees, area) {
        super(name, built);
        this.trees = trees;
        this.area = area;
    }
    get density() { return this.trees / this.area }
}
class Street extends Asset {
    constructor(name, built, length, size = 'normal') {
        super(name, built);
        this.length = length;
        this.size = size;
    }
}
const parks = [
    new Park('Mordern Park', 1909, 100, 0.2),
    new Park('Tooting Park', 1950, 1000, 0.4),
    new Park('Balham Park', 1800, 600, 2.9),
];
const streets = [
    new Street('Fayland Avenue', 1982, 200),
    new Street('Tooting Broadway', 1908, 1000, 'huge'),    
    new Street('Penwortham Road', 1950, 300, 'big'),
    new Street('Merton Road', 1932, 100, 'small'),
];

const report = {
    create() {
        return [
            ...report.parkAges(),
            ...report.treeDensities(),
            ...report.manyTrees(),
            ...report.streetLengths(),
            ...report.streetSizes(),
        ];
    },
    parkAges() {
        var meanAge = parks.reduce((sum,park) => sum + park.age, 0) / parks.length;
        return [`Parks have an average age of ${Math.round(meanAge)} years`];
    },
    treeDensities() { 
        return parks.map(park => `${park.name} has ${park.density} trees per square km`);
    },
    manyTrees() { 
        return parks
            .filter(park => park.trees > 1000)
            .map(park => `${park.name}: has a  ${park.trees} trees`);
    },
    streetLengths() {
        const total = streets.reduce((sum, street) => sum + street.length, 0);;
        return [
          `The total length of the town's streets is ${total} meters`,
          `The average length is ${Math.round(total / streets.length)} meters`
        ];
    },
    streetSizes() { 
        return streets.map(street => `${street.name} is ${street.size}`);
    }
}
// to output to console
// console.log(report.create());


// For example only
logArray(report.create());

function log(textContent) {
    reportDisplay.appendChild(Object.assign(document.createElement("div"),{textContent}));
}
function logArray(array) { array.forEach(log) }
<code id="reportDisplay"></code>

Example

The following example is what I would call JS pure and takes a lot of care to protect via encapsulation the data and functions of the object assets.

It is a little far from ideal, it is mostly to show some alternative style OO JavaScript for those that come from more strict class typed languages like Java.

"use strict";
const assets = (() => {
    const types = Object.freeze({
        park: "park",
        road: "road",
        street: "road",
        parks: "park",
        streets: "road",        
    });
    const currentYear = new Date().getFullYear();
    const assetTypes = {
        [types.road](length, size = "normal") {
            return {
                get length() { return length },
                get size() { return size },
            };
        },
        [types.park](trees, area) {
            return {
                get manyTrees() { return trees > 1000 },
                get density() { return Number((trees / area).toFixed(1)) }
            };
        },
        create(type, name, year, ...data) {
            return Object.freeze({
                get age() { return currentYear - year },
                get name() { return name },
                ...assetTypes[type](...data), 
            });
        },
    };    
    const assets = {[types.road]: new Map(), [types.park]: new Map()};
    const API = Object.freeze({
        types,
        each(type, cb) { for(const asset of assets[type].values()) { cb(asset) } },
        addAsset(type, name, ...data) { 
            assets[type].set(
                name, 
                assetTypes.create(type, name, ...data)
            );
        },
        get totalStreetLength() {
            var total = 0;
            API.each(types.road, road => total += road.length);
            return total;
        },
        get meanStreetLength() { return API.totalStreetLength / assets[types.road].size },
        get meanParkAge() {
            var total = 0;
            API.each(types.park, park => total += park.age);
            return Math.round(total / assets[types.park].size);
        },
        report() {
            const report = [];
            API.each(types.parks, park => report.push(`${park.name} has ${park.density} trees per km squared.`));
            report.push(`The average age of all parks is ${API.meanParkAge}years.`);
            API.each(types.parks, park => park.manyTrees &&  (report.push(`${park.name} has more than 1000 trees.`)));
            report.push(`The total street length is ${API.totalStreetLength}m.`);
            report.push(`The average street length is ${API.meanStreetLength}m.`);
            API.each(types.streets, street => report.push(`${street.name} has a size clasification of ${street.size}`));
            return report;
        }
    });
    return API;
})();

const smallTown = assets;
const types = assets.types;
smallTown.addAsset(types.park, 'Mordern Park', 1909, 100, 0.2);
smallTown.addAsset(types.park, 'Tooting Park', 1950, 1010, 0.4);
smallTown.addAsset(types.park, 'Balham Park', 1800, 100, 2.9);
smallTown.addAsset(types.street, 'Fayland Avenue', 1982, 200);
smallTown.addAsset(types.street, 'Tooting Broadway', 1908, 1000, 'huge');
smallTown.addAsset(types.street, 'Penwortham Road', 1950, 300, 'big');
smallTown.addAsset(types.street, 'Merton Road', 1932, 100, 'small');


logArray(smallTown.report())
function log(textContent) {
    reportDisplay.appendChild(Object.assign(document.createElement("div"),{textContent}));
}
function logArray(array) { array.forEach(log) }
<code id="reportDisplay"></code>

\$\endgroup\$
  • \$\begingroup\$ fantastic answer, honestly learnt so much from it. I am more familiar with your second example but I think the first example looks a lot cleaner \$\endgroup\$ – user6248190 Apr 25 at 8:58

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.