C++

Forum

 
reading Roman numerals - logic error

grumblesnake (16)
Hello, I'm currently going through Programming Principles and Practice Using C++.
At the moment of writing, I'm trying to write a program, that converts Roman numeral to arabic numbers. Unfortunately there seems to be a logic error, despite the fact, that I thought I triangulated its location.
The code works by reading "Roman_int"s thought an overloaded >> operator and systematically finding "thousands", then "hundreds" etc. The error seems to happen in the functions get_100s and get_10s.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
 
  int get_10s(istream& is)
{

	string s;
	char ch;
	is.get(ch);
	if (!isalpha(ch)){ is.unget(); return 0; }
	s = ch;
	switch (ch) {
	case 'X':
		while (ch == 'X'){
            is.get(ch);
            if (ch != 'L' && ch != 'C') is.unget();
            s += ch;
		}
		break;
	case 'L':
		while (is.get(ch) && ch == 'X') s += ch;
		is.unget();

	}
	cout << "string10 = " << s << endl; //to make sure that it actually recived the characters. 

	for (Roman_int ri : roman_tens) if (s == ri.roman){cout << "arabic10 = " << ri.arabic << endl; return ri.arabic;}// + get_1s(is);} 
// a mess I made to verify whether it found a matching string
	error_throw(s, " is not a roman numeral (get_10s)", no_int);
}
int get_100s(istream& is)
{

	string s;
	char ch;
	is.get(ch);
	if (!isalpha(ch)){ is.unget(); return 0; }
	s = ch;
	switch (ch) {
	case 'C':
		while (ch == 'C'){
            is.get(ch);
            if(ch != 'D' && ch != 'M') is.unget();
            s += ch;
		}
		break;
	case 'D':
		while (is.get(ch) && ch == 'C') s += ch;
		is.unget();
	}
	cerr << "string100 = " << s << endl;
	for (Roman_int ri : roman_hundreds) {
          //if (s == ri.roman) cout << ri.roman << " " << ri.arabic << endl;
        if (s == ri.roman) return ri.arabic + get_10s(is);
	}
	error_throw(s, " is not a roman numeral (get_100s)", no_int);

}


When I run the program with either 'C' or 'X' it throws my error message, which to me, seems to indicate that they don't find matching strings in their Roman_int vectors, but there doesn't seem to be anything wrong with them.

I hope some new eyes will finally help me make sense of the problem.

Complete code in the link.
Roman_int.cpp
https://pastebin.com/EF2Xmw6E
Roman_int.h
https://pastebin.com/x5YCwNZk
main.cpp
https://pastebin.com/DTCENwV8
RomanVectors.h
https://pastebin.com/YVmBQxG6

PS: This is mostly about discovering my own solutions, so I'm sure it is all awfully inefficient and doesn't really follow best practice, but but unless it is directly related to the problem, I'm not really interested in that stuff.


zaphraud (157)

Enter Roman numeral
MCMIX
string100 = CM
string10 = I
ERROR: I is not a roman numeral (get_10s)


yet

$ ./main
Enter Roman numeral
III
Roman III equals 3


If nothing else, there is a problem with the error detection. I'd start with changing that first: Parse the entire string and make sure its only correct input characters first. That way you won't have to worry about telling someone that I is not a roman numeral, you'll just be adding and subtracting them up as they come: if there's a formatting problem with the numeral, you can tell the user that the entire string is not a valid roman numeral.
Last edited on
Enoizat (1343)
When I run the program with either 'C' or 'X' it throws my error message

To solve those problems:
in:
int get_100s(istream& is)
after:
if(ch != 'D' && ch != 'M') is.unget();
change the following line from:
s += ch;
to:
if(ch != '\n') { s += ch; }

and in:
int get_10s(istream& is)
after:
if (ch != 'L' && ch != 'C') is.unget();
change the following line from:
s += ch;
to:
if(ch != '\n') { s += ch; }

Are you sure what you posted is the original code?

tpb (1495)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
 
#include <iostream>
#include <string>
#include <regex>
#include <cctype>

bool romanGood(std::string line) {
    std::regex re(
        "(M{1,3})?"
        "(D?C{1,3}|C?D|CM)?"
        "(L?X{1,3}|X?L|XC)?"
        "(V?I{1,3}|I?V|IX)?",
        std::regex_constants::extended | std::regex_constants::icase);
    return std::regex_match(line, re);
}

int romanToInt(std::string line) {
    std::string syms{"MDCLXVI"};
    int vals[] = {1000, 500, 100 ,50, 10, 5, 1};
    int last_value = 0;
    int sum = 0;
    for (auto ch: line) {
        size_t i = syms.find_first_of(toupper(ch));
        if (vals[i] > last_value)
            sum += vals[i] - 2 * last_value;
        else
            sum += vals[i];
        last_value = vals[i];
    }
    return sum;
}

#define COL0 "\x1b[0;0m"
#define COL1 "\x1b[34m"
#define COL2 "\x1b[35m"

int main() {
    using std::cout;
    const char *prompt = COL1 ">>> " COL0;
    std::string line;
    cout << prompt;
    while (std::getline(std::cin, line)) {
        if (romanGood(line))
            cout << '\t' << COL2 << romanToInt(line) << COL0 "\n";
        else
            cout << "bad input\n";
        cout << prompt;
    }
    cout << '\n';
    return 0;
}

grumblesnake (16)
If nothing else, there is a problem with the error detection. I'd start with changing that first: Parse the entire string and make sure its only correct input characters first.

Error handling has been a big problem for me, but mainly because the Bjarne Stroustrup's header stopped working for me, and I had to come up with my own method to circumvent using the file. So I'm not surprised if I've handled it less than optimally. This is what my error handling function looks like: 

1
2
3
4
5
6
7
8
9
10
 
void error_throw(string s1, string s2, double d)
{
	ostringstream convert;// stream used for the conversion ( output string stream )#include <sstream>
	convert << "ERROR: ";
	if (s1 != no_string) convert << s1; 
	if (s2 != no_string) convert << s2;
	if (d != no_int) convert << d;

	throw convert.str(); //return content of stream
}


I'm not completely sure what you mean, do you want me to insert every argument immediately into the ostringstream? I did notice that the error message sometimes seems to have a line skip, but didn't know what the cause was.

Are you sure what you posted is the original code?

I'm not sure actually, I think I may have made some changes shortly before I made the post. In any case, I went through the code again and came up with a lot of changes. 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
 
int get_1s(istream& is)
{
	char ch;
	string s;
	is.get(ch);
	if (!isalpha(ch)){ is.unget(); return 0; }
	s = ch;
	while (is.get(ch) && isalpha(ch)) s += ch;
    is.unget();
	for (Roman_int ri : roman_ones) if (s == ri.roman) return ri.arabic;
	error_throw(s, " is not a roman numeral", no_int);
}
//------------------------------------------------------------------------------
int get_10s(istream& is)
{
	string s;
	char ch;
	is.get(ch);
	if (!isalpha(ch)){ is.unget(); return 0; }
	s = ch;
	switch (ch) {
	case roman_10:
		while (is.get(ch) && (ch == roman_10 || ch == roman_50 || ch == roman_100)) s += ch;
		is.unget();
		break;
	case roman_50:
		while (is.get(ch) && ch == roman_10) s += ch;
		is.unget();
        break;
    default: { is.unget(); return get_1s(is); }
    }
	for (Roman_int ri : roman_tens) if (s == ri.roman) return ri.arabic + get_1s(is);
	error_throw(s, " is not a roman numeral", no_int);
}
//------------------------------------------------------------------------------
int get_100s(istream& is)
{

	string s;
	char ch;
	is.get(ch);
	if (!isalpha(ch)){ is.unget(); return 0; }
	s = ch;
	switch (ch) {
	case roman_100:
	    while (is.get(ch) && (ch == roman_100 || ch == roman_500 || ch == roman_1000)) s += ch;
		is.unget();
		break;
	case roman_500:
		while (is.get(ch) && ch == roman_100) s += ch;
		is.unget();
		break;
	default: { is.unget(); return get_10s(is); }
	}
	for (Roman_int ri : roman_hundreds) if (s == ri.roman) return ri.arabic + get_10s(is);
	error_throw(s, " is not a roman numeral", no_int);
}
//------------------------------------------------------------------------------
int get_1000s(istream& is)
{
	string s;
	char ch;
	is.get(ch);
	s = ch;
    while (is.get(ch) && ch == roman_1000) s += ch;
    is.unget();

	for (Roman_int ri : roman_thousands) if (s == ri.roman) return ri.arabic + get_100s(is);
	error_throw(s, " is not a roman numeral", no_int);
}


The reason that roman 20 and probably 200 failed was because it wasn't roman 50, 100, 500, or thousand and the if statement would return it to the stream.
Then the next function would find it and throw and error.

I guess it is basically the same solution as yours, except that I choose to simply check every legal character, as I concluded that a real number wouldn't affect the following steps.
I'm was able to run through a list of every roman numeral from 1 to 3999, which is the reason that main reads from a file: https://pastebin.com/FUmaPWcp
Output file: https://pastebin.com/jbGM2Kbr

The current code:
main.cpp v. 2
https://pastebin.com/avZLk6Cz

Roman_int.cpp v. 2
https://pastebin.com/FsQLEk3E

The headers should be identical.
Last edited on
Topic archived. No new replies allowed.