2
\$\begingroup\$
var romanArray = [];
var toRoman = {

    analyze: function(number){
        romanArray = [];
        if (number >= 1000) {
            return this.thousands(number);
        }else if (number >= 100){
            return this.hundreds(number);
        }else if (number >= 10) {
            return this.tens(number);
        }else{
            return this.last_number(number);
        }
    },

    thousands: function(number){
        var remainder = number % 1000;
        var thousands = Math.floor(number / 1000);
        for(var e = 0; e < thousands; e++) {
            romanArray.push('M');
        }
        return this.hundreds(remainder);
    },

    hundreds: function(number){
        var remainder = number % 100;
        var hundreds = Math.floor(number / 100);
        if (hundreds === 4){
            romanArray.push('CD');
        }else if(hundreds === 9){
            romanArray.push('CM');
        }else if(hundreds >= 5 && hundreds < 9){
            romanArray.push('D');
            for(var i = 0; i < (hundreds % 5); i++) {
                romanArray.push('C');
            }
        }else if (hundreds > 0 && hundreds < 4){
            for(var e = 0; e < hundreds; e++) {
                romanArray.push('C');
            }
        }else{

        }
        return this.tens(remainder);
    },

    tens: function(number){
        var remainder = number % 10;
        var tens = Math.floor(number / 10);
        if (tens === 4){
            romanArray.push('XL');
        }else if(tens === 9){
            romanArray.push('XC');
        }else if(tens >= 5 && tens < 9){
            romanArray.push('L');
            for(var i = 0; i < (tens % 5); i++) {
                romanArray.push('X');
            }
        }else if (tens > 0 && tens < 4){
            for(var e = 0; e < tens; e++) {
                romanArray.push('X');
            }
        }else{

        }
        return this.last_number(remainder);
    },

    last_number: function (number){
        if (number === 4){
            romanArray.push('IV');
        }else if(number === 9){
            romanArray.push('IX');
        }else if(number >= 5 && number < 9){
            romanArray.push('V');
            var remainder = number % 5;
            for(var i = 0; i < remainder; i++) {
                romanArray.push('I');
            }
        }else if (number > 0 && number < 4){
            for(var e = 0; e < number; e++) {
                romanArray.push('I');
            }
        }else{

        }
        return romanArray.join('');
    }
};

console.log(toRoman.analyze(1000));
console.log(toRoman.analyze(2999));
console.log(toRoman.analyze(2555));

I'm wondering if this JavaScript can be refactored using some sort of base/template function.
For Example:

base_function: function (number, four, nine, five, one){
    if (number === 4){
        romanArray.push(four);
    }else if(number === 9){
        romanArray.push(nine);
    }else if(number >= 5 && number < 9){
        romanArray.push(five);
        var remainder = number % 5;
        for(var i = 0; i < remainder; i++) {
            romanArray.push(one);
        }
    }else if (number > 0 && number < 4){
        for(var e = 0; e < number; e++) {
            romanArray.push(one);
        }
    }else{

    }
    return romanArray;
},

How can I improve this code?

\$\endgroup\$
4
  • \$\begingroup\$ Your example confuses me. What would the variables four, nine, five, and one be? The names aren't very descriptive... \$\endgroup\$
    – Daniel
    Commented Dec 4, 2013 at 22:03
  • \$\begingroup\$ You're really close to answering your own question with base_function(). \$\endgroup\$ Commented Dec 4, 2013 at 22:17
  • \$\begingroup\$ I've updated the question per @DanielCook suggestion. The parameters four, nine, five, and one represent the strings at each position of the number. For example at last_number position ('IV', 'IX', 'V', 'I'), but at tens position the variables would be ('XL', 'XC', 'L', 'X'). \$\endgroup\$
    – ltrainpr
    Commented Dec 4, 2013 at 22:33
  • \$\begingroup\$ @200_success I feel I'm close to solving this, but I don't know how to make the base function chain with the next function. Any and all help is greatly appreciated. \$\endgroup\$
    – ltrainpr
    Commented Dec 4, 2013 at 22:43

2 Answers 2

3
\$\begingroup\$

Normally, asking for code to be written is against the rules of this website. However, since you already have a working solution, an idea of where you want to go, and an independent solution from @Stuart, I suppose there's no harm in just finishing it for you:

hundreds: function(number) {
    base_function(Math.floor(number / 100), 'CD', 'CM', 'D', 'C');
    return this.tens(number % 100);
},

// etc.
\$\endgroup\$
2
  • \$\begingroup\$ I was returning the wrong thing. I kept attempting to return this.base_function instead of this.tens. Good to know I was close thank you. \$\endgroup\$
    – ltrainpr
    Commented Dec 5, 2013 at 1:42
  • \$\begingroup\$ I think you need to include this before base_function for the solution to work. \$\endgroup\$
    – ltrainpr
    Commented Dec 5, 2013 at 1:46
8
\$\begingroup\$

You may be aware that there are simpler answers to this problem, such as:

function toRoman(n) {
    var r = '',
        decimals = [1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1],
        roman = ['M', 'CM', 'D', 'CD', 'C', 'XC', 'L', 'XL', 'X', 'IX', 'V', 'IV', 'I'];
    for (var i = 0; i < decimals.length; i++) {
        while (n >= decimals[i]) {
            r += roman[i];
            n -= decimals[i];
        }
    }
    return r;
}

Bearing this in mind it might be useful to explain what exactly you are trying to achieve with your refactoring? E.g. is it to practice using more complicated language patterns?

\$\endgroup\$
5
  • \$\begingroup\$ This is an elegant solution @Stuart. My goal was to refactor my code by eliminating repetition and using object oriented JavaScript(OOJS) for clarity. \$\endgroup\$
    – ltrainpr
    Commented Dec 4, 2013 at 23:06
  • \$\begingroup\$ @ltrainpr my opinion, but "I want to do this using js prototype inheritence" is the source of quite a few crappy, unnecessarily complex frameworks I've seen people build over and over again. OO in JS is an awkward shim. It's useful for optimization, or if you're building something with a plugin architecture, but contributes little else. It's usually much better to rely on the functional aspects of JS than on the object-oriented ones /opinion \$\endgroup\$ Commented Dec 4, 2013 at 23:17
  • 2
    \$\begingroup\$ My comment would also be that this problem does not call for OO design... There are no 'objects' here (except a number) and it does not in practice seem to improve clarity, and I can't see any way to improve the clarity and eliminate repetition while retaining the OO design. \$\endgroup\$
    – Stuart
    Commented Dec 4, 2013 at 23:18
  • \$\begingroup\$ @GeorgeMauer You may be right but I don't think this problem would call for OO design in any language \$\endgroup\$
    – Stuart
    Commented Dec 4, 2013 at 23:20
  • \$\begingroup\$ Except of course in Java where if you don't have an AbstractFactory that is driven by a xml configuration you're not doing it right :) All kidding aside, that's a good point. \$\endgroup\$ Commented Dec 4, 2013 at 23:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.