Skip to main content
Markdown fixes and minor copy-edit
Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308
  • 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()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<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(https://en.cppreference.com/w/cpp/error/invalid_argumentstd::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 exersizeexercise... 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-driven development?).

  • 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(https://en.cppreference.com/w/cpp/error/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 exersize... 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?).

  • 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?).

Source Link
Aconcagua
  • 341
  • 1
  • 7

  • 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(https://en.cppreference.com/w/cpp/error/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 exersize... 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?).