As a beginner, how do I know if I'm writing "good" code?

Hi all!

I’m completely new to programming but have spent the last few days powering through the exercises. I just finished the Roman Numeral Converter and felt very good about it since I devised the entire solution by myself. Curious to see how others would’ve gotten to the same result, I looked for alternative solutions and came across this thread:

When I saw the “Basic” and “Intermediate” solutions, I was struck with the question: How do I know whether my code is good/bad, basic/advanced after clearing the basic hurdle of having it satisfy the requirements?

For example, here’s my original code for the exercise. How can I tell if its efficient or not? Should I even worry about it at this point? Thanks!


function convertToRoman(num) {
  //Setup
  var patterns = ["", "A", "AA", "AAA", "AB", "B", "BA", "BAA", "BAAA", "AC", "C"];
  var sets = [["I","V","X"], ["X","L","C"], ["C","D","M"], ["M","M","M"]];
  var numArray = [];
  var finalArray = [];
  var testString = "";

  //Execution
  if (num > 3999) {
    return "Value too large!";
  }
  num = num.toString();
  numArray = num.split("");
  numArray = numArray.reverse();
  for (var i = 0; i < numArray.length; i++){
    numArray[i] = patterns[numArray[i]];
  }
  

  for (var j = 0; j < numArray.length; j++){
    testString = numArray[j];
    testString = testString.replace(/C/g, sets[j][2]);
    testString = testString.replace(/B/g, sets[j][1]);
    testString = testString.replace(/A/g, sets[j][0]);
    finalArray.push(testString);
  }

  finalArray = finalArray.reverse();
  
  return finalArray.join("");
}
1 Like

Clean, clear code is the best code.

As for this example, chaining methods would improve things slightly:

numArray = num.split("").reverse();

and

return finalArray.reverse().join("");

@miwst

Thank you for the prompt feedback!

I understand that clear, clean code is the ideal to strive for, but I was wondering more about the overall approach.

With this particular exercise, I considered both the strict conversion approach featured in the Basic Solution and the switch approach outlined in the Intermediate Solution, but ultimately decided against them because to me, my approach felt the most intuitive; but that’s a subjective judgment with personal bias.

What I guess I’m trying to ask is, should I worry about these differences in approaches on how to solve the problem? If so, how can I tell which approach is ‘better’ or ‘worse’ assuming the code themselves are equally concise and clean?

2 Likes

I took the hardest method in solving this algorithm. Not that there isn’t an easier way, but is fun trying to program one thing in multiple different solution. :smiley:

Basically, I broke down each number and get each number’s roman numeral.
But I was stubborn… I must do it recursively by removing numbers one at a time. :smiley:

I don’t think there is right way or a wrong way. As you get more practice, I am sure the amount of code require to achieve the result will be shorten. I am starting to redo all the challenges using what I’ve learned so far.

You get the currently saved result in each challenge, and you can just comment those out, then rewrite a newer version. It is awesome to see the progress. :smiley:

function convertToRoman(num) {
  var digits = num.toString( ).length;       //Total Digits
  var input  = num.toString( ).split( "" );  //Array Representation
  
  var output = [];

  for( var i = 0; i < input.length; i++ ) {
    var number = parseInt( input[i] ) * Math.pow( 10, digits - 1 );
    getRoman( output, number );
    digits--;
  }

  return output.join( "" );
}

//Get a single roman numerial
//Must be in their proper denomination
function getRoman( finalArray, num ) {   
  var digit = num.toString( ).length - 1;
  
  var base1  = Math.pow( 10, digit );
  var base5  = 5 * base1;
  var base10 = 10 * base1;
  
  var rom1  = getBase( base1 );
  var rom5  = getBase( base5 );
  var rom10 = getBase( base10 );

  var caseNum = parseInt( num.toString( ).split( "" ).shift( ) );
 
  switch( caseNum ) {
    case 1:
      finalArray.push( rom1 );
      break;
      
    case 2:
    case 3:
      getRoman( finalArray, num - base1 );
      getRoman( finalArray, base1 );
      break;
      
    case 4:
      getRoman( finalArray, base1 );
      getRoman( finalArray, base5 );
      break;
      
    case 5:
      finalArray.push( rom5 );
      break;
      
    case 6:
    case 7:
    case 8:
      var next = num - base5;
      getRoman( finalArray, base5 );
      getRoman( finalArray, next );
      break;
      
    case 9:
      getRoman( finalArray, base1 );
      getRoman( finalArray, base10 );
      break;
      
    case 10:
      finalArray.push( rom10 );
      break;
  }
}

function getBase( num ) {
  var retValue = "";
  
  switch( num ) {
    case 1:
      retValue = "I";
      break;
      
    case 10:
      retValue = "X";
      break;
      
    case 100:
      retValue = "C";
      break;
      
    case 1000:
      retValue = "M";
      break;
    
    case 10000:
      retValue = "B";
      break;
      
    case 100000:
      retValue = "K";
      break;
      
    case 5:
      retValue = "V";
      break;
      
    case 50:
      retValue = "L";
      break;
      
    case 500:
      retValue = "D";
      break;
      
    case 5000:
      retValue = "U";
      break;
      
    case 50000:
      retValue = "Z";
      break;
  }
  
  return retValue;
}

convertToRoman( 97539 );

You should only worry about taking different approaches: if your algorithm is facing a performance issue or if you want to learn other ways to solve problem. It is pretty pointless otherwise.

If you still want to know how performant your algorithm is, you can measure the task completion time with large input samples, or figure out the number of operations required for your algorithm to complete with average and worst input.

To give my two cents, your code is doing these things

  1. Number to string
  2. String to array
  3. Reverse array
  4. Iteration over array
  5. Another iteration over array which involves three search and replace operations per iteration
  6. Another reverse
  7. Finally join

That does look quite involved to me considering those operations themselves implicitly runs several more iterations. However, computer can still run them incredibly fast; so, it should not bother you.

Here’s my code if you are interested.

var romanNumberValues = [
    1, 4, 5, 9,
    10, 40, 50, 90,
    100, 400, 500, 900,
    1000
];
var romanNumbers = [
    'I', 'IV', 'V', 'IX',
    'X', 'XL', 'L', 'XC',
    'C', 'CD', 'D', 'CM',
    'M'
];

// Input: integer [1, 3999]
function convertToRoman(num) {
    var res = '';
    for (var i=romanNumbers.length-1; num > 0; ) {
        var val = romanNumberValues[i];
        if (num - val >= 0) {
            num -= val;
            res += romanNumbers[i];
        } else {
            --i;
        }
    }
    return res;
}

p.s: I edited this post a lot because I’m learning how to write and trying to learn English at the same time. So let me know if there are things that doesn’t make any sense. Thanks.

2 Likes

As a beginner, you know your code is good if it produces the expected result, AND you understand how you did everything so you can use what you have learned to solve similar problems in the future.
I imagine code optimization will come with practice. I’m still starting out so I am just happy when code executes and produces the intended result.

2 Likes