6
\$\begingroup\$

I did the following Excercise from Stroustrups PPP-Book (CH10 EX6)

Define a Roman_int class for holding Roman numerals (as ints) with a << and >>. Provice Roman_int with an as_int() member that returns the int value, so that if r is a Roman_int, we can write cout<< "Roman" <<r <<" equals " << r.as_int() <<'\n';

I revised my own solution after finishing the book so I used all language facilities available, not only the ones to use until that chapter in the book.

I wonder what can be still improved with readability, good practice etc.

Heres the code:

Roman_int.h

#pragma once

#include <string>
#include <vector>
#include <iostream>
#include <map>

namespace roman_int
{
    using Roman = std::string;
    using Roman_value = std::string;
    using Integer = int;
    using Integer_value = int;

    class Roman_int {
    public:
        Roman_int() = default;

        explicit Roman_int(const Integer& value)
            :integer{ value }, roman{ integer_to_roman(value) }
        {
        }

        explicit Roman_int(const Roman& value)
            :integer{ roman_to_integer(value) }, roman{ value }
        {
        }

        Roman as_roman() const { return roman; }
        Integer as_integer()const { return integer; }
    private:

        struct Roman_integer_values {
            Roman_integer_values(const Roman_value& roman_digit, const Integer_value& integer_digit)
                :roman{ roman_digit }, integer{ integer_digit }
            {
            };

            Roman_value roman;
            Integer_value integer;
        };

        using Lockup_table = std::vector<Roman_integer_values>;

        static const Lockup_table lookup_table;

        Roman roman{};
        Integer integer{};

        Integer Roman_int::roman_to_integer(const Roman& roman);
        Roman integer_to_roman(const Integer& integer);

        bool is_valid_roman(const Roman& roman);
    };

    std::ostream& operator<<(std::ostream& os, const Roman_int& roman);
    std::istream& operator>>(std::istream& is, Roman_int& roman);

    Roman_int operator*(const Roman_int& a, const Roman_int& b);
    Roman_int operator/(const Roman_int& a, const Roman_int& b);
    Roman_int operator+(const Roman_int& a, const Roman_int& b);
    Roman_int operator-(const Roman_int& a, const Roman_int& b);

    bool operator==(const Roman_int& a, const Roman_int& b);
    bool operator!=(const Roman_int& a, const Roman_int& b);

    Roman_int operator%(const Roman_int& a, const Roman_int& b);
}

Roman_int.cpp

#include <algorithm>

#include "Roman_int.h"

namespace roman_int
{

    const Roman_int::Lockup_table Roman_int::lookup_table =
    {
        { "M",1000 },
        { "CM",900 },
        { "D",500 },
        { "CD",400 },
        { "C",100 },
        { "XC",90 },
        { "L",50 },
        { "X",10 },
        { "IX",9 },
        { "V",5 },
        { "IV",4 },
        { "I",1 },
    };

    Roman Roman_int::integer_to_roman(const Integer& integer)
    {
        if (integer <1) {
            throw std::runtime_error(
                "Roman Roman_int::integer_to_roman(const Integer& integer)\n"
                "Invalid Integer value it must be >= 1 \n"
            );
        }

        Roman roman;
        Integer tmp_integer = integer;

        for (auto it = lookup_table.cbegin(); it != lookup_table.cend(); ++it) {
            while (tmp_integer - it->integer >= 0) {
                tmp_integer -= it->integer;
                roman += it->roman;
            }
        }
        return roman;
    }

    Integer Roman_int::roman_to_integer(const Roman& roman)
    {
        if (!is_valid_roman(roman)) {
            throw std::runtime_error(
                "Integer Roman_int::roman_to_integer(const Roman& roman)\n"
                "Invalid input for roman\n"
            );
        }

        Integer integer = 0;

        for (const auto& roman_value : roman) {

            for (const auto& element : lookup_table){

                if (element.roman.size() != 1) {
                    continue;
                }

                if (roman_value == element.roman[0]) {
                    integer += element.integer;
                    break;
                }
            }
        }
        return integer;
    }

    bool Roman_int::is_valid_roman(const Roman& roman)
    {
        for (const auto roman_value : roman) {
            bool valid = false;
            for (const auto& element : lookup_table){

                if (element.roman.size() != 1) {
                    continue;
                }

                if (roman_value == element.roman[0]) {
                    valid = true;
                    break;
                }
            }
            if (!valid) {
                return false;
            }
        }
        return true;
    }

    std::ostream& operator<<(std::ostream& os, const Roman_int& roman)
    {
        return os << roman.as_roman();
    }

    std::istream& operator>>(std::istream& is, Roman_int& roman)
    {
        Roman input;
        is >> input;
        if (!is) {
            is.setstate(std::ios::failbit);
            return is;
        }
        else {
            for (size_t i = 0; i < input.size(); i++) {

                if (!isdigit(input[i])) {
                    is.setstate(std::ios::failbit);
                    return is;
                }
            }
            roman = Roman_int(input);
            return is;
        }
    }

    Roman_int operator*(const Roman_int& a, const Roman_int& b)
    {
        Roman_int r{ a.as_integer() * b.as_integer() };
        return r;
    }

    Roman_int operator/(const Roman_int& a, const Roman_int& b)
    {
        Roman_int r{ a.as_integer() / b.as_integer() };
        return r;
    }

    Roman_int operator+(const Roman_int& a, const Roman_int& b)
    {
        Roman_int r{ a.as_integer() + b.as_integer() };
        return r;
    }

    Roman_int operator-(const Roman_int& a, const Roman_int& b)
    {
        int result = a.as_integer() - b.as_integer();
        if (result < 0) {
            result = 1;
        }
        Roman_int r{ r };
        return r;
    }

    bool operator==(const Roman_int& a, const Roman_int& b)
    {
        if (a.as_integer() == b.as_integer() && a.as_roman() == b.as_roman())
            return true;
        return false;
    }
    bool operator!=(const Roman_int& a, const Roman_int& b)
    {
        return !(a == b);
    }

    Roman_int operator%(const Roman_int& a, const Roman_int& b)
    {
        Roman_int r{ a.as_integer() % b.as_integer() };
        return r;
    }   
}

main.cpp

#include <iostream>
#include "Roman_int.h"


int main()
try {

    for (int i = 1; i < 100; ++i) {
        roman_int::Roman_int test{ i };

        std::cout << test.as_integer() << '\t' << test << '\n';
    }
    std::cin.get();
}
catch (std::runtime_error& e) {
    std::cerr << e.what() << "\n";
    std::cin.get();
}
catch (...) {
    std::cerr << "unknown error\n";
    std::cin.get();
}
\$\endgroup\$
  • \$\begingroup\$ Did you really mean to call it Lockup_table instead of Lookup_table? \$\endgroup\$ – Toby Speight Aug 2 '18 at 19:21
  • \$\begingroup\$ no it is a typo \$\endgroup\$ – Sandro4912 Aug 2 '18 at 20:11
8
\$\begingroup\$

Adopt and enforce a "no warnings" policy. You should compile with warnings and you should treat them like errors. Compiling with the following flags

-pedantic -Wall -Wextra -Weffc++ -Werror

is the following output

In file included from Roman_int.cpp:3:
./Roman_int.h:50:28: error: extra qualification on member 'roman_to_integer'
    Integer Roman_int::roman_to_integer(const Roman& roman);
            ~~~~~~~~~~~^
./Roman_int.h:20:14: error: field 'integer' will be initialized after field 'roman' [-Werror,-Wreorder]
        :integer{ value }, roman{ integer_to_roman(value) }
         ^
./Roman_int.h:25:14: error: field 'integer' will be initialized after field 'roman' [-Werror,-Wreorder]
        :integer{ roman_to_integer(value) }, roman{ value }
         ^
Roman_int.cpp:145:22: error: variable 'r' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]
    Roman_int r{ r };
              ~  ^
3 errors generated.

The first one is just a flat out error that should have been caught. The other two are warnings turned into errors. Out-of-order initialization of data members and a variable initialized to its uninitialized self.


#pragma once

Be aware that you are giving up portability here as this is, while common, a non-standard compiler extension. For nearly all applications, as long as you use implementations that support it, neither physically nor logically copy files around, and the filesystem doesn't trigger a false-positive, then #pragma once is fine. Otherwise, stick with standard include guards and give some effort to differentiate the guard name.


#include <string>
#include <vector>
#include <iostream>
#include <map>

Only include what is required to make a file self-sufficient. You don't want to burden users of your code with the bloat of unused code (like <map> and <iostream>) but you don't want users latently including files because you forgot to include something.

<iostream> is a special case. Many of the implementations of C++ transparently injects static constructors into every translation unit that includes it's library. You should avoid including <iostream> in headers unless you absolutely need it. Consider splitting IO related operations from your class or include the minimal amount possible. In your case, <iosfwd> provides the forward declarations for std::ostream and std::istream. In the source file, you'll need <iostream> for the definitions to the declarations.


namespace roman_int

Use namespaces for logical structuring as they exist to group together types and functions that are similar. Maybe you have a namespace for numerics (number systems). Maybe you have a bunch of roman-related things. Maybe you have a project-wide workspace of stuff written by you.


    using Roman = std::string;
    using Roman_value = std::string;
    using Integer = int;
    using Integer_value = int;

    class Roman_int {

These aliases are publicly exposed and may be better placed inside Roman_int or just deleted. They aren't very safe to use and really just obfuscates the underlying types by being outside of its used scope. Read up on type safety using strong types (Boccara, Müller) to avoid issues commonly found with aliasing existing types.

Entities, like functions, classes, namespaces, and modules, should be open for extension, but closed for modification. If someone wants to add data members or functionality, there are mechanisms in C++ to extend functionality, like Templates.

    template <typename Roman = std::string, typename Integer = int>
    class Roman_int {

Now a user isn't stuck using std::string when maybe they want some other class that will compress the symbols. int may be the incorrect size for the users need.


        Roman_int() = default;
        ...
        Roman roman{};
        Integer integer{};

Constructors are used to guarantee and simplify initialization of classes. Constructors should create a fully initialized object and if it cannot, then throw an exception. Default constructors are simple. We either know we have default values or we omit the default constructor from the interface. For Roman Numerals, there is no representation for 0, but you set integer to 0. Perhaps a sane default state may be to set roman to "I" and set integer to 1 to match the logic of integer_to_roman()?


        explicit Roman_int(const Integer& value)
            :integer{ value }, roman{ integer_to_roman(value) }
        {}
        ...
        Roman roman{};
        Integer integer{};

If a user passes a non-positive integer which cannot be represented as a roman numeral, should this object fully initialize? The empty string is possible. You could throw or use another type of error reporting option. You could have roman be of type std::optional and have it's non-representable states be std::nullopt .

As the warnings above noted, your initialization is out of order. C++ actually has a defined initialization order. From the standard:

In a non-delegating constructor, initialization proceeds in the following order:

  • ...

  • Then, non-static data members are initialized in the order they were declared in the class definition (again regardless of the order of the mem-initializers).

This didn't bite you here, but if you ever had data members dependent on each other where the misorder mattered, you would have seen a bug.

You passed the parameter by reference to-const. Unless you plan on supporting integral types beyond 3-4 words (the natural size of a register), like infinite precision numerics, then that is fine. Otherwise, cheap to pass values should be passed by value.


        if (integer <1) {
            throw std::runtime_error(
                "Roman Roman_int::integer_to_roman(const Integer& integer)\n"
                "Invalid Integer value it must be >= 1 \n"
            );
        }

Keep in mind that an exception should reflect what went wrong. The hierarchy of exceptions most users will use and derive from are:

Exception Hierarchy

Exceptions can carry type information and you can leverage the type system to handle each exception type differently. You should have thrown a logic error instead of a runtime error as the error was caused by a flaw in the callers thinking. Even better, throw an invalid argument exception or derive a new exception type specifically for your domain (nonpositive_argument). Runtime errors can be thought of as errors beyond the users control (hardware failure, dropped connection, etc).


        for (auto it = lookup_table.cbegin(); it != lookup_table.cend(); ++it) {
            while (tmp_integer - it->integer >= 0) {
                tmp_integer -= it->integer;
                roman += it->roman;
            }
        }

Prefer to use range-based for or an algorithm over writing your own loops.

        for (auto&& numeral : lookup_table) {
            while (tmp_integer >= numeral.integer) {
                tmp_integer -= numeral.integer
                roman += numeral.roman;
            }
        }

    Roman_int operator/(const Roman_int& a, const Roman_int& b)
    {
        Roman_int r{ a.as_integer() / b.as_integer() };
        return r;
    }

Since you currently have cases where the Roman_int can be 0, you should check if the divisor is 0 in both the division and modulus operator. Assuming you disallow that, you don't need to check.


    Roman_int operator-(const Roman_int& a, const Roman_int& b)
    {
        int result = a.as_integer() - b.as_integer();
        if (result < 0) {
            result = 1;
        }
        Roman_int r{ r };
        return r;
    }

Don't clamp the result to 1. If the result is a nonpositive value, report it to the caller as its likely a logic error.

Enabled warnings would have caught the self-assignment problem.

You can construct the result as an unnamed temporary of your return type (return {result};).


    bool operator==(const Roman_int& a, const Roman_int& b)
    {
        if (a.as_integer() == b.as_integer() && a.as_roman() == 

b.as_roman()) return true; return false; }

Instead of returning true or false depending on the result of the if statement, just return the result of the if statement.

    bool operator==(const Roman_int& a, const Roman_int& b)
    {
        return a.as_integer() == b.as_integer() && a.as_roman() == b.as_roman();
    }

Assuming both the integer and roman representations are synchronized throughout the lifetime of the object, do you need to check both?


While it's nice that you were able to print out the entire table from 1 to 99, it would be better if you had unit tests for each of your methods and compared observed results to expected results. Consider picking up a testing framework like Catch2, Boost.Test, or Googletest.


Overall, it's well written. Some issues to fix. I would suggest that you check out the CPPCoreGuidelines (if you use Visual Studio, use the CoreGuideline analysis tooling), read up on SOLID, and integrate both unit testing and clang-format into your tooling.


Update

        Roman_integer_values(const Roman_value& roman_digit, const Integer_value& integer_digit)
            :roman{ roman_digit }, integer{ integer_digit }
        {
        };
         ^

You have extra semicolons after function definitions. Compilers will consider them as "empty statements" and ignore them. Remove them.

There are multiple places where your class member functions have local variables that shadow class data members.

    Roman roman{};
    Integer integer{};

    Integer Roman_int::roman_to_integer(const Roman& roman);
                                                     ^^^^^
    Roman integer_to_roman(const Integer& integer);
                                          ^^^^^^^
    bool is_valid_roman(const Roman& roman);
                                     ^^^^^

Same applies with variables declared inside your class member functions.

    Roman as_roman() const { return roman; }
    Integer as_integer()const { return integer; }

Consider returning the string by const reference. The user can then decide if they want a reference to the string or a copy. That also allows you to qualify your string getter as non-throwing (noexcept). If you opt to generalize your class with templated types instead of static aliased types, be consistent with your declaration of reference/value qualifiers. Your constructor takes int by reference to const. Your getting returns int by value. If Integer is templated and the user passes ExpensiveToCopyInteger (infinite precision integers), then that copy becomes expensive.

    const Roman& as_roman() const noexcept { return roman; }
    Integer as_integer() const noexcept { return integer; }
\$\endgroup\$
  • \$\begingroup\$ were to set these flags -pedantic -Wall -Wextra -Weffc++ -Werror in visual studio? i see only w1 to w4 there \$\endgroup\$ – Sandro4912 Aug 3 '18 at 16:18
  • \$\begingroup\$ Project Properties > Config Properties > C/C++. Under general, you can set the warning level (/W3 or /W4 is fine). If you see /Wall, just know VS libs will produce tons of warnings so you'll need to suppress them. -Weffc++ and -Wextra do not exist for VS. Werror is the next option under warning level (/WX). The closest MSVC has to pedantic is "Disable Language extensions" and "Conformance mode" both on yes. Both of those can be found on the Language property window. Misordered initialization was recently added to VS, go to the Command Line pane and add /w35038. \$\endgroup\$ – Snowhawk Aug 3 '18 at 17:16
7
\$\begingroup\$

A few errors in the code

There are a few errors in your code: r being initialized with itself in operator-, a wrong initialization order in your constructors, and an superfluous class specifier in Integer Roman_int::roman_to_integer(const Roman& roman);. You should correct them directly in you post, at least the last one which prevents your code from compiling.

A domain-specific error

Romain numerals are a bit trickier than you seem to think. Your is_valid_roman function lets invalid roman literals pass, such as "MMICXIX". You can't only check whether characters are valid, their order also counts.

Here's a valid algorithm, based on the components you offer in your code with some simplification:

#include <iostream>
#include <algorithm>
#include <string>

const std::pair<std::string, int> look_up[] = 
{
        { "M",1000 },
        { "CM",900 },
        { "D",500 },
        { "CD",400 },
        { "C",100 },
        { "XC",90 },
        { "L",50 },
        { "X",10 },
        { "IX",9 },
        { "V",5 },
        { "IV",4 },
        { "I",1 },
};

bool is_valid_roman(const std::string& input) {
    // roman litterals must:
    // 1. be constitued of subsequences registered in the look-up table and
    // 2. those subsequences must be in decreasing order and
    // 3. only one-letter subsequence can be repeated
    auto num_it = std::begin(look_up); 
    auto num_last = std::end(look_up);
    for (auto it = input.begin(); it != input.end(); ) {
        if (num_it == num_last) return false; // rules 1 and 2
        while (!std::equal(num_it->first.begin(), num_it->first.end(), it))
            if (++num_it == num_last) return false; // rules 1 and 2
        std::advance(it, num_it->first.size());
        if (num_it->first.size() == 2) ++num_it; // rules 3
    }
    return true;
}

The idea is to progressively consume the look-up table in order to enforce the decreasing order of the roman number's subsequences.

Don't throw away useful results

It is worth noticing that the algorithm to check if the roman number is valid is more or less the same than the algorithm to compute its value. It'd be best not to do work twice, then. The best way to do that is to rely on std::optional to relay the result: it's empty if the number is ill-formed, and contains a value otherwise:

std::optional<int> roman_integer_value(const std::string& input) {
    // roman litterals must:
    // 1. be constitued of subsequences registered in the look-up table and
    // 2. those subsequences must be in decreasing order and
    // 3. only one-letter subsequence can be repeated
    auto value = 0;
    auto num_it = std::begin(look_up); 
    auto num_last = std::end(look_up);
    for (auto it = input.begin(); it != input.end(); ) {
        if (num_it == num_last) return {};
        while (!std::equal(num_it->first.begin(), num_it->first.end(), it))
            if (++num_it == num_last) return {};
        std::advance(it, num_it->first.size());
        value += num_it->second;
        if (num_it->first.size() == 2) ++num_it;
    }
    return value;
}

is_valid_roman can now be based on it with a simple static_cast<bool> (operator bool is part of std::optional's interface).

Your exception handling isn't coherent

In your constructor, you check if the roman number is valid, and that is good. But then, operator- silently substitutes 1 to values equal to or less than 0, which can lead to bugs really hard to find. And in operator== you're checking whether roman and integral values are equivalent -although this is the invariant the constructor establishes- but consider that it isn't worth to throw if they aren't.

Also, be more specific: std::runtime_error is a bit general, whereas std::range_error is designed for such occasions.

factorize operations on integer values

You miss on an occasion to factorize your operators, for instance:

template <typename Binary_function>
Roman_int binary_operation_on_integer_value(const Roman_int& a, const Roman_int& b, Binary_function&& fn) {
    return Roman_int{ std::forward<Binary_function>(fn)(a.as_integer(), b.as_integer()) };
} 

Roman_int operator*(const Roman_int& a, const Roman_int& b)
{
    return binary_operation_on_integer_value(a, b, std::multiplies{});
}

This doesn't seem much, but the extra thinking would have prevented:

Roman_int operator-(const Roman_int& a, const Roman_int& b)
{
    int result = a.as_integer() - b.as_integer();
    if (result < 0) {
        result = 1;
    }
    Roman_int r{ result };
    return r;
}

which should rely on the constructor to throw an exception whenever a <= b. operator== would also have been more efficient this way.

No comment

Actually I'm commenting on the absence of comment. Your is_valid_roman should be commented: which rules are you trying to enforce?

The nested struct Roman_integer_values is useless

I understand the desire to illustrate features, but this struct doesn't bring anything to the table an std::pair wouldn't also bring, besides maybe the naming. Also, it is defined inside Roman_int, and is never used inside that class: that means it has a useless, cumbersome extra long qualified name (roman_int::Roman_int::Roman_integer_values)

Use array over vector when

no dynamic allocation is needed. Your look up table shouldn't grow, so use an std::array instead.

Miscellaneous

while (tmp_integer - it->integer >= 0) in integer_to_roman, isn't very readable. Use a simple comparison instead: while (tmp_integer >= it->integer)

"while (tmp_integer - it->integer >= 0)" not optimal run_time error not specific enough look_up_table should be an array

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.