HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Factoring Integers the fun way

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thewayfunfactoringintegers

Problem

Code dump:

function factor(n) {
    function count(a) {
        if (a.length == 0) {
            return [];
        } else {
            var x = a.pop();
            var l = count(a);
            if (l[x] == undefined) {
                l[x] = 0;
            }
            l[x]++;
            return l;
        }
    }
    function decomp(comp, fac) {
        if (comp == 1) {
            return [];
        }
        if (fac == 1) {
            return [ comp ];
        }
        if (comp % fac == 0) {
            return decomp(fac, fac - 1).concat(decomp(comp / fac, fac));
        } else {
            return decomp(comp, fac - 1);
        }
    }
    return count(decomp(n, n)).map(function(a, b) {
        return [ b, a ];
    }).filter(function(a, b) {
        return a[1];
    }).map(function(a, b) {
        return "" + a[0] + "^" + a[1];
    }).reduce(function(a, b) {
        return a + " * " + b;
    }).replace("^1", "");
}


It's mainly because I've been into functional programming lately that I tried a loopless, stateless, functional algorithm; I know that this example is absolutely facetious and that no one would do this in real life. That said, are there ways I can make this:

  • More efficient (e.g. less data),



  • More concise (which keeping the cool functional aspect), or



  • More functional (in a good way) in paradigm?

Solution

-
Keystrokes are (almost) free. It's not like decomp looks any better than decompose. (Moving forward, I'm going to assume you made the rename.)

-
Your count() function is cute. For extra fun, you could replace it with Array.reduce() (which you may know as foldl):

decompose(n, n).reduce(function(accumulator, value) {
    if (typeof accumulator[value] === "undefined") {
        accumulator[value] = 0;
    }
    accumulator[value]++;
    return accumulator;
}, []).map ...


(The logic can be reduced to prev[cur]++ || (prev[cur] = 1) or prev[cur] = prev[cur] + 1 || 1 if desired.)

-
It's worth mentioning that comparisons against undefined are typically done with ===.

-
You've got a redundant else in decompose().

-
You can use switch(true) syntactic sugar to make decompose() prettier:

function decompose(composite, factor) {
    switch(true) {
        case composite == 1:
            return [];
        case factor == 1:
            return [composite];
        case composite % factor == 0:
            return decompose(factor, factor - 1).concat(decompose(composite / factor, factor));
        default:
            return decompose(composite, factor - 1);
    }
}


or alternatively, chained conditional operators (commonly called ternary operators):

function decompose(composite, factor) {
    return composite == 1 ? [] :
           factor == 1 ? [composite] :
           composite % factor != 0 ? decompose(composite, factor - 1) :
           decompose(factor, factor - 1).concat(decompose(composite / factor, factor));
    }
}


-
decompose(n, n) is ugly. Maybe you rewrite the contract of decompose to int (maybe int) -> array?

-
You're replacing only the first instance of ^1. You'll want to do .replace(/\^1/g, "") instead.

-
factor(1) doesn't return 1 as expected. You can fix this by switching your first two ifs.

-
factor(0) recurs infinitely.

-
You're using a lot of functions at the end there.

Here's my rendition. It's a little bit less elegant with the return... oh well. I'm using a utility function when to avoid potentially ugly ternary nesting. The double reduce bugs me but I can't think of a better way right now.

function factor(n) {
    function when(condition, string) {
        return condition ? string : "";
    }

    function decompose(composite, factor) {
        return factor === undefined ? decompose(composite, composite) :
               factor  1, "^ " + value));
        }, "").substr(3);
}

Code Snippets

decompose(n, n).reduce(function(accumulator, value) {
    if (typeof accumulator[value] === "undefined") {
        accumulator[value] = 0;
    }
    accumulator[value]++;
    return accumulator;
}, []).map ...
function decompose(composite, factor) {
    switch(true) {
        case composite == 1:
            return [];
        case factor == 1:
            return [composite];
        case composite % factor == 0:
            return decompose(factor, factor - 1).concat(decompose(composite / factor, factor));
        default:
            return decompose(composite, factor - 1);
    }
}
function decompose(composite, factor) {
    return composite == 1 ? [] :
           factor == 1 ? [composite] :
           composite % factor != 0 ? decompose(composite, factor - 1) :
           decompose(factor, factor - 1).concat(decompose(composite / factor, factor));
    }
}
function factor(n) {
    function when(condition, string) {
        return condition ? string : "";
    }

    function decompose(composite, factor) {
        return factor === undefined ? decompose(composite, composite) :
               factor <= 1 ? [composite] :
               composite <= 1 ? [] :
               composite % factor != 0 ? decompose(composite, factor - 1) :
               decompose(factor, factor - 1).concat(decompose(composite / factor, factor));
    }

    return decompose(n).reduce(function(accumulator, value) {
            accumulator[value] = accumulator[value] + 1 || 1;
            return accumulator;
        }, []).reduce(function(accumulator, value, index) {
            return accumulator + when(value, " * " + index + when(value > 1, "^ " + value));
        }, "").substr(3);
}

Context

StackExchange Code Review Q#59432, answer score: 4

Revisions (0)

No revisions yet.