Cfrtjryk

I have a project where I work with digital addresses for a Programmable Logic Controller (PLC). The requirement is that the user gives a start address e.g. 1000.0 and then for example the next 20 things are numbered in increasing order.

In this example this would mean:

1000.0

1000.1

1000.2

1000.3

1000.4

1000.5

1000.6

1000.7

1001.0

1001.1

1001.2

1001.3

1001.4

1001.5

1001.6

1001.7

1002.0

1002.1

1002.2

1002.3

Also I want to prevent that invalid addresses can be created at the start like 1000.8

To accomplish this behaviour I created a class Digital_plc_address. I would like to know what can be improved. Are there any bad styles in it?

Is it good practice to make a class to report invalid_input errors when the class cannot be created?

I did some quick checks of the class in main. I know I could have done better with unit tests but I thought for this small class it's okay to check it like this.

digital_plc_adress.h

#ifndef DIGITAL_PLC_ADDRESS_GUARD151120181614

#define DIGITAL_PLC_ADDRESS_GUARD151120181614



#include <string>



namespace digital_plc_address {



    class Digital_plc_address {

        /*

        Class representing a digital plc address.

        Each address is represented by a byte address and the digits of the 

        bytes from 0-7. e.g  100.6 , 5.2

        Creating invalid addresses like 100.8 is not allowed by the class.

        */

    public:

        Digital_plc_address(int byte, char digit = 0);

        explicit Digital_plc_address(const std::string& Digital_plc_address);



        class Invalid_format {};



        std::string as_string() const;



        Digital_plc_address& operator++();

    private:

        int m_byte;

        char m_digit;

    };

}



#endif

digital_plc_address.cpp

#include "digital_plc_address.h"



#include <regex>

#include <sstream>

#include <cctype>

#include <limits>



namespace digital_plc_address {



    Digital_plc_address::Digital_plc_address(int byte, char digit)

        :m_byte{ byte }, m_digit{ digit }

    {

        if (m_byte < 0 || m_byte > std::numeric_limits<decltype(m_byte)>::max()

            || m_digit < 0 || m_digit > 7) {

            throw Invalid_format{};

        }

    }



    Digital_plc_address::Digital_plc_address(const std::string& Digital_plc_address)

    {

        std::regex valid_format(R"(d+.[0-7]{1})");



        if (!std::regex_match(Digital_plc_address, valid_format)) {

            throw Invalid_format{};

        }



        std::istringstream ist{ Digital_plc_address };



        std::string byte_str;

        std::getline(ist, byte_str, '.');



        m_byte = std::stoi(byte_str);

        m_digit = Digital_plc_address.back() - '0';

    }



    std::string Digital_plc_address::as_string() const

    {

        return std::to_string(m_byte) + '.' + std::to_string(m_digit);

    }



    Digital_plc_address& Digital_plc_address::operator++()

    {

        if (m_digit == 7) {

            m_digit = 0;

            ++m_byte;

        }

        else {

            ++m_digit;

        }

        return *this;

    }

}

main.cpp

#include "digital_plc_address.h"



#include <iostream>





void print_io(int a)

{

    try {

        digital_plc_address::Digital_plc_address t{ a };

        std::cout << t.as_string() << 'n';

    }

    catch (digital_plc_address::Digital_plc_address::Invalid_format) {

        std::cout << "Invalid input: " << a << 'n';

    }

}



void print_io(int a, char b)

{

    try {

        digital_plc_address::Digital_plc_address t{ a,b };

        std::cout << t.as_string() << 'n';

    }

    catch (digital_plc_address::Digital_plc_address::Invalid_format) {

        std::cout << "Invalid input: " << a << 't' << b <<'n';

    }

}



void print_io(const std::string& s)

{

    try {

        digital_plc_address::Digital_plc_address t{ s };

        std::cout << t.as_string() << 'n';

    }

    catch (digital_plc_address::Digital_plc_address::Invalid_format) {

        std::cout << "Invalid input: " << s << 'n';

    }

}



int main() 

try{



    digital_plc_address::Digital_plc_address inc{ 10 };



    for (int i = 0; i < 20; ++i) {

        ++inc;

        std::cout << inc.as_string() << 'n';

    }



    print_io(100);

    print_io(100, 1);

    print_io(50, 7);



    print_io("100");

    print_io("100.1");

    print_io("50.7");



    print_io("-50.7");



    print_io("50.-7");



    print_io("50.8");



    std::getchar();

}

catch (digital_plc_address::Digital_plc_address::Invalid_format) {

    std::cout << "error";

    std::getchar();

}

• Obviously, negative (sub-) addresses are invalid - so I would prefer unsigned types for, and you can spare the checks for negative range as well.




• std::numeric_limits<decltype(m_byte)>::max() might exceed the range of addresses of your PLC (or not cover all of, but rather unlikely). What, if PLC uses 16-bit int (would be legal!), but your machine has 32-bit int? I'd better use an appropriate constant for. Better control on number of bits provide the types of <cstdint> as well, you should prefer these ones.




• Your operator++, if applied sufficiently often, could exceed the range of valid addresses as well. In worst case, signed integer overflow occurs (if you don't switch to unsigned), which is undefined behaviour. Possibly a range check for byte as well, and then decide how you want to handle it (wrap around? throw exception?).




• Depends on how your PLC actually is addressed - you could combine both values in a single address (uint32_t address = byte << 3 | digit), but if you need to pass both values separately to your PLC's API, it might not be worth the effort (unless it is about saving memory usage).




• Digital_plc_address::Digital_plc_address(const std::string& Digital_plc_address) – not a good idea to give parameter same name as class.




• You do not check range for the byte in the string constructor. User could have provided too many digits!




• There's already std::invalid_argument, I'd prefer a standard exception over a custom one (InvalidFormat); if you want to retain your custom one, I'd at least let it derive from std::exception or possibly std::logic_error as std::invaid_argument does.

Checking range for byte and digit in constructor is fine. I'm a bit in conflict with the constructor accepting a string. Generally, I prefer checking user input (a string) before and only on validity, creating the actual object. On the other hand, you provide a as_string function as well, so this constructor is kind of symmetric counterpart (especially if we consider the so formed string as data exchange format for some e. g. network protocol). So in final conclusion, I'm fine with.

About unit test / manual written tests: Unit tests come with the great advantage that they can be run and checked automatically on a build system. If you need to check your results manually anyway, benefits of using a unit test framework are limited, it would be more about exercise... In professional environment, unit-tests are required, though, so starting with manual tests and then writing unit tests would result in work being doubled (and what about test-driven development?).