Prośba o ocenę programu

Proszę o ocenę programu konwertującego liczby arabskie na rzymskie i na odwrót.

#include <iostream>
#include <string>
#include <sstream>
#include <cmath>
#include <limits>

using namespace std;

class Roman_int {
public:
    Roman_int(int n) {
        this->n = n;
    }
    Roman_int(const string& str) {
        n = roman_to_int(str);
    }
    int as_int() const;
    friend istream& operator>>(istream& ist, Roman_int& r);
    friend ostream& operator<<(ostream& ost, const Roman_int& r);
private:
    int roman_to_int(const string& roman);
    string as_roman() const;
    int n;
};

int Roman_int::roman_to_int(const string& roman) {
    istringstream ist {roman};
    int result = 0.0;
    for (char ch; ist >> ch; ) {
        switch (ch) {
            case 'M':
                result += 1000;
                break;
            case 'D':
                result += 500;
                break;
            case 'C': {
                char next;
                ist >> next;
                if (next == 'D' || next == 'M') {
                    result -= 100;
                } else {
                    result += 100;
                }
                ist.putback(next);
                break;
            }
            case 'L':
                result += 50;
                break;
            case 'X': {
                char next;
                ist >> next;
                if (next == 'L') {
                    result -= 10;
                } else {
                    result += 10;
                }
                ist.putback(next);
                break;
            }
            case 'V':
                result += 5;
                break;
            case 'I': {
                char next;
                ist >> next;
                if (next == 'V') {
                    result -= 1;
                } else {
                    result += 1;
                }
                ist.putback(next);
                break;
            }
        }
    }
    return result;
}

inline int nth_digit(int n, int k) {
    return static_cast<int>(n/pow(10, k-1))%10;
}

string repeat(const string& s, int n) {
    string res;
    for (int i = 0; i < n; ++i) {
        res += s;
    }
    return res;
}

string Roman_int::as_roman() const {
    ostringstream ost;
    int tho = nth_digit(n, 4);
    if (tho >= 1 && tho <= 3) {
        ost << repeat("M", tho);
    }

    int hun = nth_digit(n, 3);
    if (hun >= 1 && hun <= 3) {
        ost << repeat("C", hun);
    } else if (hun == 4) {
        ost << "CD";
    } else if (hun >= 5 && hun <= 8) {
        ost << "D" << repeat("C", hun - 5);
    } else if (hun == 9) {
        ost << "CM";
    }

    int dec = nth_digit(n, 2);
    if (dec >= 1 && dec <= 3) {
        ost << repeat("X", dec);
    } else if (dec == 4) {
        ost << "XL";
    } else if (dec >= 5 && dec <= 8) {
        ost << "L" << repeat("X", dec - 5);
    } else if (dec == 9) {
        ost << "XC";
    }

    int one = nth_digit(n, 1);
    if (one >= 1 && one <= 3) {
        ost << repeat("I", one);
    } else if (one == 4) {
        ost << "IV";
    } else if (one >= 5 && one <= 8) {
        ost << "V" << repeat("I", one - 5);
    } else if (one == 9) {
        ost << "IX";
    }

    return ost.str();
}

int Roman_int::as_int() const {
    return n;
}

bool is_number(const string& str) {
    for (char ch : str) {
        if (!isdigit(ch)) return false;
    }
    return true;
}

istream& operator>>(istream& ist, Roman_int& r) {
    string str;
    ist >> str;
    if (!ist) {
        return ist;
    }
    if (is_number(str)) {
        r.n = atoi(str.c_str());
    } else {
        r = Roman_int(str);
    }
    return ist;
}

ostream& operator<<(ostream& ost, const Roman_int& r) {
    return ost << r.as_roman();
}

template <class T>
T read_input(const string& msg, const string& err) {
    cout << msg << "\n> ";
    T in;
    while (!(cin >> in)) {
        cout << err << '\n';
        cin.clear();
        cin.ignore(numeric_limits<streamsize>::max(), '\n');
        cout << msg << "\n> ";
    }
    return in;
}

template <>
string read_input<string>(const string& msg, const string& err) {
    cout << msg << "\n> ";
    string in;
    cin.ignore(numeric_limits<streamsize>::max(), '\n');
    while (!getline(cin, in)) {
        cout << err << '\n';
        cin.clear();
        cin.ignore(numeric_limits<streamsize>::max(), '\n');
        cout << msg << "\n> ";
    }
    return in;
}

int main() {
    const string msg = "Co chcesz zrobić?\n"
                 "Dostępne opcje:\n"
                 "1) Zamiana liczby rzymskiej na normalną\n"
                 "2) Zamiana liczby zwykłej na rzymską\n"
                 "q) Koniec";
    const string err = "Proszę podać liczbę.";
    bool quit = false;
    while (!quit) {
        auto choice = read_input<char>(msg, err);
        switch (choice) {
            case '1': {
                auto roman = read_input<string>("Podaj liczbę rzymską.", "Niepoprawna wartość!");
                cout << "Rzymskie " << roman << " wynosi " << Roman_int(roman).as_int() << '\n';
                break;
            }
            case '2': {
                auto n = read_input<int>("Podaj liczbę zwykłą.", "Niepoprawna liczba!");
                cout << n << " po rzymsku to " << Roman_int(n) << '\n';
                break;
            }
            case 'q':
                quit = true;
                break;
            default:
                cout << "Niepoprawny wybór!\n";
                break;
        }
    }
    cout << "Do widzenia!\n";
    return 0;
}

Za bym zaczął od

Podaj liczbę rzymską.
> IX
Rzymskie IX wynosi 11

    int result = 0.0;

to niby działa, ale skąd pomysł na użycie tutaj literału zmiennoprzecinowego?

3. Oba konstruktory powinny używać list inicjalizacyjnych, tzn

    Roman_int(int n) : n(n) {}
    Roman_int(const string& str) : n(roman_to_int(str)) {}

Gdyby w roman_to_int nie używać istringstreama, to można by się pokusić o opatrzenie ich specyfikatorem noexcept.

4. using namespace std nie przystoi - jeśli jest jakaś często używana nazwa, możesz zrobić bezpośrednio using std::string (czy coś innego). Aczkolwiek to też bym odradzał.

5. Nazewnictwo jest pomieszane. Standardowa konwencja mówi, że nazwą Twojej klasy powinno być RomanInt (albo ew. roman_int, ale raczej to pierwsze), nigdy natomiast to co napisałeś(aś?).

6. Skoro już i tak używasz szablonów to można być może pokusić się o użycie klasy szablonowej, w której można użyć także innych typów niż int, szczególnie że ten tak średnio się nadaje, bo pozwala na liczby ujemne (a rzymskie liczby jak wiadomo nie pozwalają), ale to nas przynosi do kolejnego problemu:

7. (n/pow(10, k-1))%10; użycie tutaj dowolnego typu większego niż int spowoduje, że Twoje liczby zmiennoprzecinkowe szybko stracą na precyzji. Imo, można się tutaj pokusić o naiwną implementację potęgowania, ewentualnie szybie potęgowanie. Tak czy inaczej liczb zmiennoprzecinkowych być nie powinno.

8. Piszesz inline int nth_digit(int n, int k), a to jest słabe z dwóch powodów, pierwszy to że zupełnie nie przejmujesz się tym, że k może być ujemne, również zupełnie nie przejmujesz się tym, że k może być poza dopuszczalnym zakresem. inline niczemu tutaj nie służy, bo w przeciwieństwie do tego co myślisz, inline oznacza nie mniej, nie więcej że funkcję tak opisaną można zdefiniować wiele razy. Kompilator i tak sobie sam zdecyduje, czy będzie ją inlinować czy nie.

9. Wracając do specyfikatorów, być może to nie ma dużego zastosowania w projekcie jednoplikowym, ale jak już chcesz coś takiego dawać, to odpowiednim specyfikatorem jest static. static jest takim niezręcznym słowem kluczowym, które w zależności od kontekstu ma całkiem różne znaczenia. Jeśli byś zrobił static int nth_digit(int n, int k), będzie znaczyć to, że nie będzie się dało odnosić do tej funkcji, z innych plików .c (czyli dobrze). Możesz również dać prywatną metodę statyczną do swojej klasy.

10. Skoro repeat i tak zawsze jest używany do pisania potem do streama, to może warto zrobić z niego po prostu funkcję, co pisze do streama? Ew klasę, co ma odpowiedni operator na strumieniach.

11. as_int zasłużyła sobie na specyfikator const, ale roman_to_int już nie. Dlaczego?

12. read_input<char> nie radzi sobie z wysłaniem EOF.

13. Implementacja roman_to_int jest całkiem pokomplikowana, i stąd ten błąd co na początku. Jestem przekonany, że da się prościej. Przykładowo, oto moja wersja w Pythonie (co nie sprawdza poprawności, ale to jest jej główny defekt - jeśli liczba była poprawna, to wynik jest również):

    def roman_to_int(roman):
        roman_values = dict(zip("IVXLCDM", [1, 5, 10, 50, 100, 500, 1000]))
        ret = 0
        prev_value = max(roman_values.values())
        for c in [roman_values[x] for x in roman]:
            ret += c
            if prev_value < c:
                ret -= 2*prev_value
            prev_value = c
        return ret

A na marginesie, Twoja wersja również poprawności nie sprawdza.