A bit of feedback on OOP, design pattern, best practice etc

First off apologies for the epic post.

As per a previous post I have been writing a number of optimized utility functions that I can use for a bit of data wrangling.

In addition as part of the exercise I have also written a timing module, which currently logs results out to the console.

Loosely following other testing modules, my module is invoked like this

// Setup
const randomArray = Array.from({length: 1000000}, () => Math.floor(Math.random() * 100));
const sum = (a, b) => a + b;

//Tests
const reduceArrays = (
    timeTests('reduce performance tests with an (Array) of one million random values')
        .add(
            'Vanilla JS reduce',
            () => {
                randomArray.reduce(sum);
            }
        )
        .add(
            '_.loDash reduce',
            () => {
                _.reduce(randomArray, sum);
            }
        )
        .add(
            'Custom reduce',
            () => {
                reduce(randomArray, sum);
            }
        )
);

reduceArrays.run(100); // 1000 iterations

It currently outputs a log like this.

Starting reduce performance tests with an (Array) of one million random values...
1. Vanilla JS reduce: 88.7 ops/sec (100 runs)
2. _.loDash reduce: 500.6 ops/sec (100 runs)
3. Custom reduce: 456.8 ops/sec (100 runs)
Results compared:
1st. _.loDash reduce: 464% faster
2nd. Custom reduce: 414.7% faster
3rd. Vanilla JS reduce: slowest

Note: 1., 2. and 3. are logged one after the other as each test completes and Results compared is outputted on completion of the tests,

I am amending my module to make it a little less tightly coupled. The console logging code was intertwined with my timing methods and instead I want to be able to pass in a custom logging class/instance.

The idea being that I could have a logger that outputs to the DOM instead of the console if I so wish.

This is the main issue I could do with input on:

With regards OOP, I really am making this up as I go along, so would appreciate some feedback. Pleass go easy on me, I know I don’t deserve it :slight_smile:

/**
 * timeIt - A function that measures time taken to execute a function over a number of iterations.
 * @param {Function} fn - The function to measure.
 * @returns {Function} A wrapped function that returns the start time, stop time, and number of iterations.
 */
function timeIt(fn) {
    // Updates the wrapper function with the passed functions name and toString method.
    const wrapper = wraps(fn);

    return wrapper (
        /**
         * wrapper
         * @param {number} iterations - The number of times to run the function.
         * @param {*} args - The arguments to pass to the function.
         * @returns {object} The delta and iterations.
         */
        function(iterations, ...args) {
            const start = performance.now();
            for (let i = 0; i < iterations; i++) fn(...args);
            const stop = performance.now();
            return { delta: stop - start, iterations };
        }
    );
}

/**
 * @interface
 * @property {Function} log - Logs a single time entry.
 * @property {Function} logResult - Logs the results of all time entries.
 */
class ConsoleLogger {
    log(time) {
        console.log(time);
    }
    logResult(times) {
        console.table(times);
    }
}

/**
 * @class
 * @param {ConsoleLogger} logger - An instance of a logger class.
 */
class Times {
    constructor(logger) {
        this.times = [];
        this.log = logger.log.bind(logger);
        this.logResult = logger.logResult.bind(logger);
    }

    add(time) {
        this.times.push(time);
        this.log(time);
    }
}


class TimeTest {
    constructor(id, description, fn) {
        this.id = id;
        this.description = description;
        this.fn = timeIt(fn);
    }

    run(iterations) {
        return {
            id: this.id,
            description: this.description,
            ...this.fn(iterations)
        };
    }
}

class TimeTests {
    #id = 1;

    constructor(title, logger) {
        this.title = title;
        this.tests = new Set();
        this.times = new Times(logger);
    }

    add(description, fn) {
        this.tests.add(new TimeTest(this.#id++, description, fn));
        return this;
    }

    remove(id) {
        const test = find(this.tests, (test) => test.id === id);
        if (test)
            this.tests.delete(test);
        return this;
    }

    run(iterations = 1000) {
        for (const test of this.tests)
            this.times.add(test.run(iterations));
        this.logTimes();
    }

    clear() {
        this.#id = 1;
        this.tests.clear();
    }

    toString() {
        return Serialize.toString(this.tests);
    }
}


/**
 * Creates a new instance of TimeTests with the given title and logger.
 * @param {string} title - The title for the time tests.
 * @param {ConsoleLogger} [logger] - The logger to use for logging the test results.
 * Defaults to an instance of ConsoleLogger.
 * @returns {TimeTests} A new instance of TimeTests.
 */
export default function timeTests(title, logger) {
    if (!logger) {
        logger = new ConsoleLogger();
    }
    return new TimeTests(title, logger);

I hope the code is clear enough to reason with, thanks.

1 Like

Just to add separately.

wraps is a function that within the limitations of JS, loosely follows Python’s functools wraps

e.g.

/**
 * wraps - Updates a wrapper function with properties from the wrapped function.
 * @param {Function} fn - The function to wrap.
 * @returns {Function} A wrapped function with its own properties.
 */
function wraps(fn) {
    return function(wrapper) {
        return Object.defineProperties(
            wrapper, {
                name: Object.getOwnPropertyDescriptor(fn, 'name'),
                toString: {
                    value: fn.toString.bind(fn)
                }
            }
        );
    };
}

It is used in conjunction with a Serialize module I have written, which is used in the toString method of TimeTests to output a prettified output.

e.g. if I console log an instance

console.log(`${reduceArrays}`)

I get a prettified output of the tests

Set(3) {
    TimeTest {
        id: 1,
        description: 'Vanilla JS reduce',
        fn: () => {
            randomArray.reduce(sum);
        }
    },
    TimeTest {
        id: 2,
        description: '_.loDash reduce',
        fn: () => {
            _.reduce(randomArray, sum);
        }
    },
    TimeTest {
        id: 3,
        description: 'Custom reduce',
        fn: () => {
            reduce(randomArray, sum);
        }
    }
}

Again this was somewhat inspired by Python and it’s dunder methods __str__ and __repr__. Unfortunately in JS you have to implicitly call the instances toString method. e.g. console.log(reduceArray.toString()) or console.log(`${reduceArrays}`)

Not dug into the code yet, but just for my clarity…

are both the "faster"s from… the lowest place? or from each other? It’s not immediately clear if, for example, 3rd is (pulling number out of my …) 16 seconds, 2nd is 4 (ish, work with me) seconds, and 1st is… < 1 second? 3.5 seconds? (Maybe give the baseline result in the output?)

2 Likes

Good question, it is based on the slowest. It is something I wasn’t sure on.

Hence being able to customise logging separately would be a good thing.

What’s the purpose of id? How is it different from the index of the set? (For that matter, why is it a set, if they’re all objects, which will inherently be unique due to the nature of Object referencing?)

I wrote this bit about a week ago, so I am having to have a bit of think.

Set seems like a reasonable choice. It prevents duplication, it provides add, delete and clear methods. ids start from 1 rather than 0.

edit: scrapped the ordinal bit, as I think I am wrong on that.

Except it doesn’t.

class DummyObject { constructor(id) { this.id = id; } }
let setTest = new Set();
setTest.add(new DummyObject(1))
setTest.add(new DummyObject(1))
set.size; << 2
1 Like

It did cross my mind, and in your example different instances are not the same. I would have had to test that. Brain is a bit frazzled.

Either way, I don’t see the choice of Set being an issue.

Edit: Sorry if I am coming across as defensive, I am interested in your alternative approach :slight_smile:

It’s not an issue. Though i… might want to run through the idea of whether it’s faster to use an array over a Set (though… having said that, my brain is telling me an array is still not a primitive, so they’re both objects anyway. eh. shrug)

Other than that… my original question was why is timeIt external to all of the classes, given that it’s only used as the iterative on TimeTest… I assume your intent is to allow overriding the reference of timeIt?

I’m so torn m_hutley. Originally this was written just as functions, and then became class based. I am out of my comfort zone.

About 30 minutes ago I was on the verge of adding timeit as a method instead. My frazzled brains couldn’t think about how to implement that with the wrapper and I wasn’t sure if it was really necessary anyway, so left it.

That said it was on my radar as a code smell, so I get you asking the question.

Performance here really isn’t an issue. The only bit that needs to be tight as it were is the timeit function itself.

Well, piece of advice #1 is going to be stop, put it down, step back, and breathe. Let your brain rest and come back at it again tomorrow or the day after just so your brain has the chance to do what all programmers brains do when they come back to code they wrote a while ago and go “why the **** did I do that?” :laughing:

Putting it in as a method would work, but you’d need methods to allow the user to define their own function for timing (which, I assume, is why you’ve made it a parameter to TimeTest’s constructor rather than a hard baked reference).

Alternatively, leaving timeIt floating in the namespace means that a user can redefine timeIt as a standard (function) variable and the system should use their code.

1 Like

Sound advice @m_hutley, seriously need a break and get your “why the **** did I do that?” comment. :+1:

3 Likes

In your class you substitute Set for Map, and then check uniqueness on the add.

let tests = new Map();

// I am using function here but you'll put as methods in your class

function add(obj) { 
  key = obj.id
  if (!tests.has(obj.id)) {
    tests.set(key, obj);
  }
}

function remove(obj) {
  tests.delete(obj.id);
}


function clear() {
  tests.clear();
}
1 Like

Hi @Zensei, objects with an id aren’t passed to add. The method receives two arguments, a string title of the test and a test function. :slight_smile:

Ok I think walking away was good.

I’ve swapped out the ConsoleLogger class for a baseHandler object. It has 3 handlers, one to be called before the test runs, one to be called on each completed test, and one to be called on completion of all tests.

/**
 * @typedef {object} Result
 * @property {number} id - The id of the test.
 * @property {string} description - The description of the test.
 * @property {number} time - The time in milliseconds.
 */
const baseHandler = {
    /**
     * Function to run before time tests.
     * @param {string} title - Title for time tests.
     * @param {number} iterations - The number of iterations.
     */
    before (title, iterations) {
        console.log('Title:', title);
        console.log('Iterations:', iterations);
    },
    /**
     * Function to run after each time tests.
     * @param {Result} result - The result of a test.
     * @param {number} iterations - The number of iterations.
     */
    each (result, iterations) {
        console.log(result);
    },
    /**
     * Function to be called after all time tests.
     * @param {string} title - Title for time tests.
     * @param {Result[]} results - Array of results for time tests.
     * @param {number} iterations - The number of iterations.
     */
    end (title, results, iterations) {
        console.table(results);
    }
};

The function to create the tests will take an optional handler object and use it to create a mixin with the base handler.

/**
 * Creates a new instance of TimeTests with the given title and logger.
 * @param {string} title - The title for the time tests.
 * @param {object} handler - The handler object.
 * @returns {TimeTests} A new instance of TimeTests.
 */
export default function timeTests(title, handler) {
    return new TimeTests(
        title, (isObject(handler))
            ? Object.assign({}, baseHandler, handler) : baseHandler
    );
}

My thinking is a custom handler could be exported from it’s own module. For instance a module to populate tables in the browser.

Elsewhere I have been doing some other refactoring.

TimeTests

Dropped the remove, clear methods. If that becomes a requirement I will put it back in, but I can’t see an immediate need.

I have added a skip method instead, which I think is more useful.

The run method now handles running the event handlers. The Times class has been dropped, and replaced with a results array.

class TimeTests {
    #id = 1;

    constructor(title, handler) {
        this.title = title;
        this.tests = new Set();
        this.handler = handler;
    }

    add(description, fn) {
        this.tests.add(new TimeTest(this.#id++, description, fn));
        return this;
    }

    skip() {
        return this;
    }

    run(iterations = 1000) {
        const results = [];
        const handler = this.handler;

        handler.before(this.title, iterations);

        for (const test of this.tests) {
            const result = test.run(iterations);
            results.push(result);
            handler.each(result, iterations);
        }

        handler.end(this.title, results, iterations);
    }

    toString() {
        return Serialize.toString(this.tests);
    }
}

TimeTest

I have added timeIt as a method, and it now only returns the time. No need to return iterations as we can pass that in from the run method of TimeTests.

class TimeTest {
    constructor(id, description, fn) {
        this.id = id;
        this.description = description;
        this.fn = this.timeIt(fn);
    }

    /**
     * timeIt - Executes looped times tests on a function.
     * @param {Function} fn - The function to time.
     * @returns {Function} A wrapped function that returns the overall time for a test.
     */
    timeIt(fn) {
        // Updates the wrapper function with the passed functions name and toString method.
        const wrapper = wraps(fn);

        return wrapper (
        /**
         * wrapper
         * @param {number} iterations - The number of times to run the function.
         * @param {*} args - The arguments to pass to the function.
         * @returns {object} The time and iterations.
         */
            function(iterations, ...args) {
                const start = performance.now();
                for (let i = 0; i < iterations; i++) fn(...args);
                const stop = performance.now();
                return stop - start;
            }
        );
    }

    run(iterations) {
        return {
            id: this.id,
            description: this.description,
            time: this.fn(iterations)
        };
    }
}

So some quick tests

No handler supplied

const reduceMapTest = (
    timeTests('reduce performance tests with a Map with one million random values')
        .add(
            '_.loDash reduce',
            () => {
                _.reduce(_.toArray(randomMap), sumKeyValues, 0);
            }
        )
        .add(
            'Custom reduce',
            () => {
                reduceMap(randomMap, sumValues, 0);
            }
        )
        .add(
            'Vanilla JS reduce',
            () => {
                Array.from(randomMap).reduce(sumKeyValues, 0);
            }
        )
);

reduceMapTest.run(100); // 100 iterations

A handler containing an each method and the first add changed to skip

const handler = {
    each: (result, iterations) => {
        const {description, time} = result;
        const averageTime = Number(time / iterations).toFixed(3);
        console.log(`${description} - avgTime: ${averageTime}ms`);
    }
};

const reduceMapTest = (
    timeTests('reduce performance tests with a Map with one million random values', handler)
        .skip(
            '_.loDash reduce',
            () => {
                _.reduce(_.toArray(randomMap), sumKeyValues, 0);
            }
        )
        .add(
            'Custom reduce',
            () => {
                reduceMap(randomMap, sumValues, 0);
            }
        )
        .add(
            'Vanilla JS reduce',
            () => {
                Array.from(randomMap).reduce(sumKeyValues, 0);
            }
        )
);

reduceMapTest.run(100); // 100 iterations

I’m sure there is room for improvement, but this is all a bit simpler I think.

Again, thanks @m_hutley.

The above worked reasonably well until I tried to implement a browser version.

I hadn’t considered the need for my code to be asynchronous. Instead of seeing each test render one by one as per the console, I was presented with a blank page and then sometime later the complete tests all rendered in one go.

This is the amended asynchronous version.

// Description: A utility for measuring the performance of functions.
import baseHandler from './handlers/base-handler.js';
import { wraps } from '/src/functions.js';

class TimeTest {
    constructor(id, description, fn) {
        this.id = id;
        this.description = description;
        this.fn = this.timeIt(fn);
    }

    /**
     * timeIt - Executes looped times tests on a function.
     * @param {Function} fn - The function to time.
     * @returns {Function} A wrapped function that returns the overall time for a test.
     */
    timeIt(fn) {
        // Updates the wrapper function with the passed functions name and toString method.
        const wrapper = wraps(fn);

        return wrapper (
            /**
             * wrapper
             * @param {number} iterations - The number of times to run the function.
             * @param {*} args - The arguments to pass to the function.
             * @returns {object} The time and iterations.
             */
            async function(iterations, ...args) {
                let totalTime = 0;

                for (let i = 0; i < iterations; i++) {
                    const start = performance.now();
                    await fn(...args);
                    totalTime += performance.now() - start;
                    await new Promise((resolve) => requestAnimationFrame(resolve));
                }
                return totalTime;
            }
        );
    }

    async run(iterations) {
        return {
            id: this.id,
            description: this.description,
            value: await this.fn(iterations)
        };
    }
}


class TimeTests {
    #id = 1;

    constructor(title, handler) {
        this.title = title;
        this.tests = new Set();
        this.handler = handler;
    }

    add(description, fn) {
        this.tests.add(new TimeTest(this.#id++, description, fn));
        return this;
    }

    skip() {
        return this;
    }

    async run (iterations = 1000) {
        const results = [];
        const handler = this.handler;

        await handler.before(this.title, this.tests, iterations);

        for (const test of this.tests) {
            const result = await test.run(iterations);
            results.push(handler.each(result, iterations));
        }

        handler.end(results, iterations);
    }

    toString() {
        return Serialize.toString(this.tests);
    }
}


/**
 * Creates a new instance of TimeTests with the given title and logger.
 * @param {string} title - The title for the time tests.
 * @param {object} handler - The handler object.
 * @returns {TimeTests} A new instance of TimeTests.
 */
export default function timeTests(title, handler) {
    return new TimeTests(
        title, ({}.toString.call(handler) === '[object Object]')
            ? Object.assign(baseHandler, handler) : baseHandler
    );
}

I’ve added an option in the browser handler to import a markdown description which is rendered to the page in HTML ahead of running the tests.

This is a screen grab to illustrate.

Some live versions here, one with a description, the rest without.
reduce-object-test
reduce-map-test
flatmap-map-test

If at all interested the files can be found here

I want to add that my little test program is not particularly accurate. It seems that other suites out there tend to be built on benchmark.js, which is altogether more advanced.

That said as they say, ‘you learn by doing’ and this exercise has given me a better insight into Javascript’s workings.

1 Like

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.