0
\$\begingroup\$

Here is a class I have:

class Chart {
    constructor (options) {
        this.cx = options.center[0]
        this.cy = options.center[1]
        this.radius = options.radius
    }

    getTopLeftSegmentPath (perc = 1) {
        const point = (this.radius / 100) * perc
        const start = [this.cx - point, this.cy]
        const end = [this.cx, this.cy - point]

        return getPath(point, start, end)
    }

    getTopRightSegmentPath (perc = 1) {
        const point = (this.radius / 100) * perc
        const start = [this.cx, this.cy - point]
        const end = [this.cx + point, this.cy]

        return getPath(point, start, end)
    }

    getBottomLeftSegmentPath (perc = 1) {
        const point = (this.radius / 100) * perc
        const start = [this.cx, this.cy + point]
        const end = [this.cx - point, this.cy]

        return getPath(point, start, end)
    }

    getBottomRightSegmentPath (perc = 1) {
        const point = (this.radius / 100) * perc
        const start = [this.cx + point, this.cy]
        const end = [this.cx, this.cy + point]

        return getPath(point, start, end)
    }

    getPath (point, start, end) {
        const tc = xy => xy.join(',')

        return `M${tc(start)} A${point}, ${point} 0 0 1 ${tc(end)} L${this.cx}, ${this.cy} Z`
    }
}

Usage:

const chart = new Chart({ center: [200, 200], radius: 100 })

chart.getTopLeftSegmentPath(50)
chart.getTopRightSegmentPath(13)
chart.getBottomLeftSegmentPath(20)
chart.getBottomRightSegmentPath(100)

I am not sure what is the best way to optimize or simplify it.

Maybe there is a chance to have a single getPath method instead of all these getTopRightSegmentPath, getTopLeftSegmentPath, etc.

\$\endgroup\$
1
\$\begingroup\$

Repetition, similarities, and redundancies

To simplify code you start by looking for repetition and similarities. In most cases you can remove the repeating/similar code by adding variables to take care of the minor differences.

Count how often you use a variable name, eg end and start are defined 5 times, and you have had to type them in 10 times each, and point has even more.

Variable names also should not have repetition. If you find yourself creating a pile of variables that all share some prefix or postfix, it's a good sign that you can optimize and simplify.

Don't do something twice when once is enough. A common practice is

const blah = foo + bar;
return blah;

The blah is redundant return foo + bar; does the same with less complexity.

see note (*1)

Functional similarities

You have four almost identical functions that contain the following

    const point = (this.radius / 100) * perc
    const start = [this.cx, this.cy + point]
    const end = [this.cx - point, this.cy]
    return getPath(point, start, end)

The only difference is the addition or subtraction of point to the coordinates. For the four functions there are only 4 different combinations which you can define as an array of values, 0 for no change, -1 for subtraction and 1 for addition.

For the above snippet that would be [0, 1, -1, 0] so if you had a generic function that accepted an array

getCorner(perc, cornerDesc) {
    const point = (this.radius / 100) * perc;
    const start = [this.cx + cornerDesc[0] * point, this.cy + cornerDesc[0] * point];
    const end = [this.cx + cornerDesc[2] * point, this.cy + cornerDesc[3] * point];
    return getPath(point, start, end);
}

But this is a bit obscure and not very intuitive. We still want to to use named function calls to get a corner, "topleft..." rather than a non descriptive array to define a corner. (see below)

Variable repetition

If we look further there is still repetition of the arguments passed to getPath. We can define point, start, and end in the getPath function and pass it the perc argument and array of offsets.

Looking at the functions named getTopRightSegmentPath, getTopLeftSegmentPath ... etc, each contains SegmentPath which is a good clue in what to remove and create once.

A good rule of thumb. If adding the same name to a bunch of variables, that similarity should be an object, eg getTopRightSegmentPath becomes segmentPath.topRight (the get is redundant in lieu of more info about Path's functionality)

Though Javascript class syntax is not conducive to complicated Objects (*), so it is best to redefine the whole Path object as a factory function (see rewrite)

So we can create something like

segmentPath = {
    topLeft(perc = 1) { return getPath(perc, [-1,0,0,-1]) },
    topRight ... // and so on
} 

Redundency

Looking at the getPath function we can remove some redundancy now that we have moved the defining of end and start into that function. The function xy can be removed as we can directly define start and end as strings skipping the need for the function xy and creating arrays that we immediately join.

this is redundant.

My favorite redundancy removal is this. When I see code that is full of this I see bad practice. The token this should only ever be used for properties that are to be accessed outside the object. The (so popular) class token encourages the exposure of everything, if you see the token class you can guarantee that by word count this is the most used (eg you use this 25 times) and mostly incorrectly.

Personalty, if you put effort in you can actually write good Javascript without ever needing this


The rewrite.

So with all that said let's put it together, (I can only guess at the overall requirements of the Path object so the rewrite may not suit additional behavioural and state management needs)

The variable cx, cy, and radius dont need to be exposed so we can close over them, the function getPath look like it is only used internally so again I will close over it.

function Chart(options) {
    var cx = options.center[0];
    var cy = options.center[1];
    var radius = options.radius;
    function getPath(offsets, perc = 1) {
        const rad = (radius / 100) * perc;
        const start = `${cx + offsets[0] * rad},${cy + offsets[1] * rad}`;
        const end = `${cx + offsets[2] * rad},${cy + offsets[3] * rad}`;
        return `M${start} A${rad}, ${rad} 0 0 1 ${end} L${cx}, ${cy} Z`;
    }
    return {
        segmentPath : {
            topLeft(perc) { return getPath([-1, 0, 0, 1], perc) },
            topRight(perc) { return getPath([0, -1, 1, 0], perc) },
            bottomLeft(perc) { return getPath([0, 1, -1, 0], perc) },
            bottomRight(perc) { return getPath([1, 0, 0, 1], perc) },
        }
    };    
}

Which is used as follows

const chart = Chart({center : [x,y], radius : 10});
const svgPath = chart.pathSegment.topLeft(2);

Alternative

I did consider the following...

    return {
        topLeft(perc = 1) { return getPath(prec, -1, 0, 0, 1) },
        topRight(perc = 1) { return getPath(prec, 0, -1, 1, 0) },
        bottomLeft(perc = 1) { return getPath(prec, 0, 1, -1, 0) },
        bottomRight(perc = 1) { return getPath(prec, 1, 0, 0, 1) },
    };    

...with the function getPath defined as

    function getPath(perc, ...offsets) {

But the repetition of perc = 1 annoyed me

Access properties via getters and setters

If you need to change the properties it is better to use getters and setters, though only as example, you can include some vetting on the setters

    return {
        segmentPath : {
            topLeft(perc) { return getPath([-1, 0, 0, 1], perc) },
            topRight(perc) { return getPath([0, -1, 1, 0], perc) },
            bottomLeft(perc) { return getPath([0, 1, -1, 0], perc) },
            bottomRight(perc) { return getPath([1, 0, 0, 1], perc) },
        }
        get center() { return [cx, cy] },
        get radius() { return radius },
        set center(cent) { [cx, cy] = cent },
        set radius(r) { radius = r },
    }

Usage

const chart = Chart({center : [x,y], radius : 10});
chart.radius *= 2;
console.log(chart.radius); // >> 20
chart.center = [x * 2, y * 2];
const svgPath = chart.pathSegment.topLeft(2);

Look mum no this


Some notes

(*) Called Object/s because in Javascript there are no "classes" only Objects. The class token is just a way of defining an object.

(*1) Note that in some cases to such as conditional and loop statements (where you use conditional operators) it is better to define befor the statement variables that require operations.

if (foo & bar === something) {

is better written as

const blah = foo & bar;
if (blah === something) {

This is because it can be a source of bug when precedence can give non intuitive results.

if(10 & 0 === 0) { // false
if((10 & 0) === 0){ // true
return 10 & 0 === 0 // returns 0 not a boolean
return (10 & 0) === 0 // returns a boolean true

Unless you have very familiar with all off javascript's operator precedence dont add expressions to statements.

Also if the following line becomes too long, dont remove

const blah = foo + bar + apple + sauce;
const poo = blah + " something else is " + value * 2 + " when " + num + count; 

is better than

const poo = (foo + bar + apple + sauce) + " something else is " + value * 2 + " when " + num + count;

^^ scroll bar bad :(

\$\endgroup\$

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.