Converting roman numerals to integer

Question

I'm doing some practice with pointers and seemingly basic touchup of these concepts, the task is simple, given 3 pre-entered test cases( strings of roman numerals) convert each to a number and tally up/print the total, here's the code:

#include <string.h>
#include <stdlib.h>
#include <stdio.h>

int romanToInt(char* s)
{
  //iterator/terminator
  int i = 0;
  //serves as the total
  int output =0;
  //serves as to read the full string of roman numerals
  int length = strlen(s);

  while (i < length)
  {
    if (s[i] == 'I')
    {
      output += 1;
    }
  
    if (s[i] == 'V')
    {
      output += 5;
    }
    
    if (s[i] == 'X')
    {
      output += 10;
    }

    if (s[i] == 'L')
    {
      output += 50;
    }
    
    if (s[i] == 'C')
    {
      output += 100;
    }

    if (s[i] == 'D')
    {
      output += 500;
    }

    if (s[i] == 'M')
    {
      output += 1000;
    }
  
    // Save to a var first then see that if i is < i+1 - logic for subtraction
    // Then if i+1 > i we have logic for adding numerals
    if (s[i] < s[i + 1] && (i <= length))
    {
      output = output - s[i];
    }
    
    if (s[i + 1] > s[i] && (i <= length))
    {
      output += s[i];
    }
    i++;
  }
     
  return output;
}          

I believe my code is working(no errors) but Im sure its very clunky and inefficient given a poor compiler is saying the time limit exceeded when trying to run, do I have a memory issue that for some reason I can't see or is my logic simply inefficient, please let me know and be patient I am new to all of this

I wanted to use switch statements to cover the conversion of roman numerals or even a hashmap but those concepts are clearly out of my reach for the time being, any info is greatly appreciated!

I know this isnt the place for leetcode questions but Im running out of avenues and people to ask, I'll keep trying but any info nontheless is greatly appreciated!

Answer 1

Efficiency

Your code is terribly inefficient and that can be enhanced even without jumping to a switch statement.

Right now every one of your if-statements is individually checked every time...

This makes no sense, as soon as you find the match it won't match anything else. It can only be one numeral.

So change

if (...) 
{...}

if (...)
{...}

to

if (...)
{...}
else if (...)
{...}
else if(...)
{...} ....

This will reduce the number of checks made so that you don't keep checking once you found a match, not great but it's an extremely intelligent improvment.

Correctness

Currently your code is also wrong in three ways.

First there is a duplication. After you handle adding to output the current translated value of s[i] you do the following:

// Save to a var first then see that if i is < i+1 - logic for subtraction
// Then if i+1 > i we have logic for adding numerals
if (s[i] < s[i + 1] && (i <= length))
{
  output = output - s[i];
}
    
if (s[i + 1] > s[i] && (i <= length))
{
  output += s[i];
}

The problem is that firstly, if s[i] < s[i+1] then logically s[i+1] > `s[i]. So you are going to say subtract and then add s[i] netting to zero change.

Secondly problem is that you are comparing the wrong things. it doesn't matter if s[i] is less than s[i+1] it matters if the translated value of the roman numeral at s[i] is less than s[i+i].

Secondly you need to subtract it TWICE because you already added it... Example IX
first you add 1 for the I, and output is 1
then you see 1<10
so you subtract the 1, output is 0 you need to subtract 1 again... so output is -1

Then loop repeats and you see X you add 10 for the X, output is 9

You are done.

Bringing it together

I would reorganize your program to make a helper function int getNumeralValue(char c) that simply gets the Numeric value from a Roman Numeral kept in a char. Then your romanToInt function simply gets the value for s[i], and get's the value for s[i]+1 (you could probably use 0 for the 0/null case. If second value > first value, subtract first value, add second value, make first = second, and read a new second value. Else output += first, first = second, read a new second value. If first ever equals zero you don't read a new second value and you stop.

This would be made faster with a switch statement

inline
int getNumeralValue(char c)
{
  switch (c)
  {
    case 'I': return 1;
    case 'V': return 5;
    case 'X': return 10;
    case 'L': return 50;
    case 'C': return 100;
    case 'D': return 500;
    case 'M': return 1000;
  }
  return 0;
}

Other thoughts

Don't use leetcode to learn how to code; If you don't know what a switch statement is and you are trying to just pass leetcode assignments you aren't learning anything. You should learn and understand a 'switch' before you look at pointers. Go through the CS50 course it's a much better use of your time than jumping to leetcode at this point.

You aren't really even looking at pointers in this problem. You are looking at a string/c-array given that you use strlen and arent responsible for freeing the memory. You could use pointers here, but you don't you use array indexing notation which is fine, but means you arent really learning anything about pointers.

leetcode has its time and place, but that time and place is very much not 'learning a language'. They have absolute no data, authentication or any credibility of any kind to say "LeetCode is the best platform to help you enhance your skills" They have ripped off the public to obtain content to monetize it into being a site to carry out technial interviews. They offer 'lessons' that are repackaged free content that has been made available in a myriad of places.

Lastly this site is NOT for leetcode help.. There are a myriad of places on the web for that... so I promise you if you post another leetcode question it won't be answered here. This site is for the CS50 program. You basically asked a question and pretended to be genuinely doing your own thing and then at the end... oh yeah this is actually a leetcode question. Not appropriate. In the future lead off with that. Be respectful of the people who are helping you

If this answers your question, please click on the check mark to accept it. Please consider upvoting the answer as well if it was useful. This helps with site maintenance.