Thread: Convert string_iterator to string

I have a piece of code that is converting Roman Numerals to int.

Code:

int RomanToInt(const string & Roms){
  if (Roms.size() == 0)
    return 0;

  unordered_map<string,int> Rom_Map = {{"I",1},
                                       {"V",5},
                                       {"X",10},
                                       {"L",50},
                                       {"C",100},
                                       {"D",500},
                                       {"M",1000},
                                       {"IV",4},
                                       {"IX",9},
                                       {"XL",40},
                                       {"XC",90},
                                       {"CD",400},
                                       {"CM",900}};
  auto first = Roms.begin();
  auto second = next(first);
  auto third = next(second);

  auto res = 0;

  while (first != Roms.end()){
    auto cChar = string{*first};

    string dChar = "";
    if (second != Roms.end())
      dChar = string{*first} + string{*second};

    if (Rom_Map.find(dChar) != Rom_Map.end()){
      res += Rom_Map.at(dChar);
      first = third;
    }
    else if (Rom_Map.find(cChar) != Rom_Map.end()){
      res += Rom_Map.at(cChar);
      first = second;
    }

    second = next(first);
    third = next(second);
  }

  return res;
}

}

Since I'm declaring the first type as string in my unordered_map, I have to convert the string_iterator to std::string.This looks kind of inefficient. Is there a more efficient way to do this?

Code:

// different way to do this? 
auto cChar = string{*first};
dChar = string{*first} + string{*second};

Originally Posted by nimitzhunter

Since I'm declaring the first type as string in my unordered_map, I have to convert the string_iterator to std::string.This looks kind of inefficient. Is there a more efficient way to do this?

I do not think you can avoid it: you are iterating over characters, but searching the map for strings.

By the way, although it works for me (and presumably works for you), I believe that next(first) and next(second) actually results in undefined behaviour if Roms.begin() and next(first) resulted in a one-past-the-end iterator, respectively. I suggest re-arranging the code to avoid this, e.g.,

Code:

int RomanToInt(const string & Roms)
{
    if (Roms.empty())
    {
        return 0;
    }

    static const auto Rom_Map = unordered_map<string, int>{{"I", 1},
                                                           {"V", 5},
                                                           {"X", 10},
                                                           {"L", 50},
                                                           {"C", 100},
                                                           {"D", 500},
                                                           {"M", 1000},
                                                           {"IV", 4},
                                                           {"IX", 9},
                                                           {"XL", 40},
                                                           {"XC", 90},
                                                           {"CD", 400},
                                                           {"CM", 900}};

    auto result = 0;

    auto first = Roms.begin();
    while (first != Roms.end())
    {
        auto valid = false;

        auto second = next(first);
        if (second != Roms.end())
        {
            auto found = Rom_Map.find(string{*first} + *second);
            if (found != Rom_Map.end())
            {
                valid = true;
                result += found->second;
                first = next(second);
            }
        }

        if (!valid)
        {
            auto found = Rom_Map.find(string{*first});
            if (found != Rom_Map.end())
            {
                valid = true;
                result += found->second;
                first = second;
            }
        }

        if (!valid)
        {
            // Throw an exception?
            break;
        }
    }

    return result;
}