Modeling an IPv4 AddressValidate IP address in JavaArray-like container for uints shorter than 8 bits (Rev 1)IPv4 struct utilizing explicit layoutIPv4 struct: Round 2Update user country based on IPv4 decimal addressCalculate IP AddressPattern for writing a generic string transformation functionRandom IP Address GeneratorModeling a parking lotValidate IP4 address

Python: next in for loop

Adding span tags within wp_list_pages list items

What's the point of deactivating Num Lock on login screens?

can i play a electric guitar through a bass amp?

How to format long polynomial?

Problem of parity - Can we draw a closed path made up of 20 line segments...

Can divisibility rules for digits be generalized to sum of digits

A newer friend of my brother's gave him a load of baseball cards that are supposedly extremely valuable. Is this a scam?

How did the USSR manage to innovate in an environment characterized by government censorship and high bureaucracy?

How old can references or sources in a thesis be?

Why Is Death Allowed In the Matrix?

How to find program name(s) of an installed package?

How to write a macro that is braces sensitive?

Why can't I see bouncing of a switch on an oscilloscope?

Approximately how much travel time was saved by the opening of the Suez Canal in 1869?

What are the differences between the usage of 'it' and 'they'?

What defenses are there against being summoned by the Gate spell?

How do we improve the relationship with a client software team that performs poorly and is becoming less collaborative?

Why are electrically insulating heatsinks so rare? Is it just cost?

"You are your self first supporter", a more proper way to say it

I’m planning on buying a laser printer but concerned about the life cycle of toner in the machine

Maximum likelihood parameters deviate from posterior distributions

Finding angle with pure Geometry.

How does one intimidate enemies without having the capacity for violence?



Modeling an IPv4 Address


Validate IP address in JavaArray-like container for uints shorter than 8 bits (Rev 1)IPv4 struct utilizing explicit layoutIPv4 struct: Round 2Update user country based on IPv4 decimal addressCalculate IP AddressPattern for writing a generic string transformation functionRandom IP Address GeneratorModeling a parking lotValidate IP4 address






.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;








13












$begingroup$


I've recently picked up a book that gives various "modern" C++ challenges/solutions. One of the first ones I did was on modeling an IPv4 address in C++. Below is the full implementation; it's also on Github. Any suggestions? My goal is to make this as modern as possible and I'm not sure if there are some C++17 features I'm not taking advantage of.



ip_address.h



#pragma once

#include <array>
#include <string>
#include <string_view>
#include <stdint.h>

namespace ip

/**
* @brief Thrown when there is an invalid ip address passed via
* string.
*/
class invalid_format_exception : public std::exception

std::string invalid_format_;
public:
invalid_format_exception(const std::string &invalid_format);
char const* what() const override;
;

/**
* Class that models a IPv4 address.
*/
class address

public:

#pragma region Type definitions
using value_type = uint8_t;
using reference = value_type & ;
using pointer = value_type * ;
using iterator = std::array<value_type, 4>::iterator;
using const_iterator = std::array<value_type, 4>::const_iterator;
using reverse_iterator = std::array<value_type, 4>::reverse_iterator;
using const_reverse_iterator = std::array<value_type, 4>::const_reverse_iterator;
using size_type = std::array<value_type, 4>::size_type;
#pragma endregion

/**
* @brief Create an IP address representation from the
* four parts of the address definition.
* @param first the first part of the address
* @param second the second part of the address
* @param third the third part of the address.
* @param fourth the fourth part of the address.
* @details Example:
* @code
* ip::address addr(127, 0, 0, 1);
* @endcode
*/
address(const value_type& first, const value_type &second,
const value_type &third, const value_type& fourth);

/**
* @brief Create an IP address representaiton from an
* array.
* @param data the data array.
* @details Example:
* @code
* ip::address addr = 127, 0, 0, 1;
* @endcode
*/
address(const std::array<unsigned char, 4> &data);

/**
* @brief Create an IP adderss representation from a
* unsigned 32 bit integer.
* @param value the integer representation of an IP address.
*/
explicit address(const uint32_t &value);

/**
* @brief Implicit conversion to an unsigned 32 bit integer.
*/
uint32_t operator()() const;

/**
* @brief Access operator.
* @param index the index to access.
*/
reference operator[](const int &index) noexcept(false);

/**
* @brief Const version of the access operator.
*/
value_type operator[](const int &index) const noexcept(false);

/**
* @brief Prefix increment operator.
*/
void operator++();

/**
* @brief Postfix increment operator.
*/
::ip::address& operator++(int);

/**
* @brief Prefix decrement operator.
*/
void operator--();

/**
* @brief Prefix decrement operator.
*/
::ip::address& operator--(int);

iterator begin();
iterator end();
const_iterator begin() const;
const_iterator end() const;
private:
std::array<value_type, 4> data_;
;

bool operator<(const ip::address &first, const ip::address &second);
bool operator==(const ip::address &first, const ip::address &second);
std::ostream& operator<<(std::ostream& output, const ip::address &address);
address from_string(const std::string &view);
std::string to_string(const address& address);



ip_address.cpp



#include <ip_address.h>

#include <iterator>
#include <iostream>
#include <sstream>
#include <regex>
#include <vector>
#include <string>

#pragma region Utilities
template<typename Output>
void split(const std::string &s, char delim, Output result)
std::stringstream ss(s);
std::string item;
while (std::getline(ss, item, delim))
*(result++) = item;



std::vector<std::string> split(const std::string &s, char delim)
std::vector<std::string> elems;
split(s, delim, std::back_inserter(elems));
return elems;


#pragma endregion

ip::invalid_format_exception::invalid_format_exception(const std::string& invalid_format)
: invalid_format_(invalid_format)



char const* ip::invalid_format_exception::what() const

std::ostringstream oss;
oss << "Invalid IP address format: " << invalid_format_;
return oss.str().c_str();


ip::address::address(const value_type & first, const value_type & second, const value_type & third, const value_type & fourth)

data_[0] = first;
data_[1] = second;
data_[2] = third;
data_[3] = fourth;


ip::address::address(const std::array<unsigned char, 4>& data)

data_ = data;


ip::address::address(const uint32_t& value)

data_[0] = value >> 24 & 0xFF;
data_[1] = value >> 16 & 0xFF;
data_[2] = value >> 8 & 0xFF;
data_[3] = value & 0xFF;


uint32_t ip::address::operator()() const

const uint32_t value = data_[0] << 24

ip::address::reference ip::address::operator[](const int& index)

return data_.at(index);


ip::address::value_type ip::address::operator[](const int& index) const

return data_.at(index);


void ip::address::operator++()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if(location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]++;



::ip::address& ip::address::operator++(int)

auto result(*this);
++(*this);
return result;


void ip::address::operator--()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if (location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]--;



::ip::address& ip::address::operator--(int)

auto result(*this);
--(*this);
return result;


ip::address::iterator ip::address::begin()

return data_.begin();


ip::address::const_iterator ip::address::end() const

return data_.end();


bool ip::operator<(const ip::address& first, const ip::address& second)

return (uint32_t)first() < (uint32_t)second();


bool ip::operator==(const ip::address& first, const ip::address& second)

return (uint32_t)first() == (uint32_t) second();


ip::address::const_iterator ip::address::begin() const

return data_.begin();


ip::address::iterator ip::address::end()

return data_.end();


std::ostream& ip::operator<<(std::ostream& output, const ip::address& address)

std::copy(address.begin(), address.end()-1,
std::ostream_iterator<short>(output, "."));
output << +address[3];
return output;


ip::address ip::from_string(const std::string &view)

auto parts = split(view, '.');
if (parts.size() != 4)

throw invalid_format_exception(view);


return
(ip::address::value_type)std::stoi(parts[0]),
(ip::address::value_type)std::stoi(parts[1]),
(ip::address::value_type)std::stoi(parts[2]),
(ip::address::value_type)std::stoi(parts[3])
;


std::string ip::to_string(const address& address)

std::ostringstream string_stream;
string_stream << address;
return string_stream.str();










share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$







  • 1




    $begingroup$
    networking-ts has an excellent reference implementation: open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4771.pdf#page179
    $endgroup$
    – sudo rm -rf slash
    yesterday






  • 1




    $begingroup$
    Mod Note: Please do not use comments for extended conversations. Comments are intended to resolve unclarities about the post. Use Code Review Chat for extended conversations and in-depth discussions. If you want to critique the code presented in the question, please do so in an answer. Thanks!
    $endgroup$
    – Vogel612
    8 hours ago











  • $begingroup$
    Remember that an ipv4 is not only four numbers separated by 3 dots. 1 = 0.0.0.1, 1.2=1.0.0.2, 1.2.3 = 1.2.0.3, 192.168.0.1 = 3232235521 = 0xC0A80001
    $endgroup$
    – Lenne
    57 mins ago


















13












$begingroup$


I've recently picked up a book that gives various "modern" C++ challenges/solutions. One of the first ones I did was on modeling an IPv4 address in C++. Below is the full implementation; it's also on Github. Any suggestions? My goal is to make this as modern as possible and I'm not sure if there are some C++17 features I'm not taking advantage of.



ip_address.h



#pragma once

#include <array>
#include <string>
#include <string_view>
#include <stdint.h>

namespace ip

/**
* @brief Thrown when there is an invalid ip address passed via
* string.
*/
class invalid_format_exception : public std::exception

std::string invalid_format_;
public:
invalid_format_exception(const std::string &invalid_format);
char const* what() const override;
;

/**
* Class that models a IPv4 address.
*/
class address

public:

#pragma region Type definitions
using value_type = uint8_t;
using reference = value_type & ;
using pointer = value_type * ;
using iterator = std::array<value_type, 4>::iterator;
using const_iterator = std::array<value_type, 4>::const_iterator;
using reverse_iterator = std::array<value_type, 4>::reverse_iterator;
using const_reverse_iterator = std::array<value_type, 4>::const_reverse_iterator;
using size_type = std::array<value_type, 4>::size_type;
#pragma endregion

/**
* @brief Create an IP address representation from the
* four parts of the address definition.
* @param first the first part of the address
* @param second the second part of the address
* @param third the third part of the address.
* @param fourth the fourth part of the address.
* @details Example:
* @code
* ip::address addr(127, 0, 0, 1);
* @endcode
*/
address(const value_type& first, const value_type &second,
const value_type &third, const value_type& fourth);

/**
* @brief Create an IP address representaiton from an
* array.
* @param data the data array.
* @details Example:
* @code
* ip::address addr = 127, 0, 0, 1;
* @endcode
*/
address(const std::array<unsigned char, 4> &data);

/**
* @brief Create an IP adderss representation from a
* unsigned 32 bit integer.
* @param value the integer representation of an IP address.
*/
explicit address(const uint32_t &value);

/**
* @brief Implicit conversion to an unsigned 32 bit integer.
*/
uint32_t operator()() const;

/**
* @brief Access operator.
* @param index the index to access.
*/
reference operator[](const int &index) noexcept(false);

/**
* @brief Const version of the access operator.
*/
value_type operator[](const int &index) const noexcept(false);

/**
* @brief Prefix increment operator.
*/
void operator++();

/**
* @brief Postfix increment operator.
*/
::ip::address& operator++(int);

/**
* @brief Prefix decrement operator.
*/
void operator--();

/**
* @brief Prefix decrement operator.
*/
::ip::address& operator--(int);

iterator begin();
iterator end();
const_iterator begin() const;
const_iterator end() const;
private:
std::array<value_type, 4> data_;
;

bool operator<(const ip::address &first, const ip::address &second);
bool operator==(const ip::address &first, const ip::address &second);
std::ostream& operator<<(std::ostream& output, const ip::address &address);
address from_string(const std::string &view);
std::string to_string(const address& address);



ip_address.cpp



#include <ip_address.h>

#include <iterator>
#include <iostream>
#include <sstream>
#include <regex>
#include <vector>
#include <string>

#pragma region Utilities
template<typename Output>
void split(const std::string &s, char delim, Output result)
std::stringstream ss(s);
std::string item;
while (std::getline(ss, item, delim))
*(result++) = item;



std::vector<std::string> split(const std::string &s, char delim)
std::vector<std::string> elems;
split(s, delim, std::back_inserter(elems));
return elems;


#pragma endregion

ip::invalid_format_exception::invalid_format_exception(const std::string& invalid_format)
: invalid_format_(invalid_format)



char const* ip::invalid_format_exception::what() const

std::ostringstream oss;
oss << "Invalid IP address format: " << invalid_format_;
return oss.str().c_str();


ip::address::address(const value_type & first, const value_type & second, const value_type & third, const value_type & fourth)

data_[0] = first;
data_[1] = second;
data_[2] = third;
data_[3] = fourth;


ip::address::address(const std::array<unsigned char, 4>& data)

data_ = data;


ip::address::address(const uint32_t& value)

data_[0] = value >> 24 & 0xFF;
data_[1] = value >> 16 & 0xFF;
data_[2] = value >> 8 & 0xFF;
data_[3] = value & 0xFF;


uint32_t ip::address::operator()() const

const uint32_t value = data_[0] << 24

ip::address::reference ip::address::operator[](const int& index)

return data_.at(index);


ip::address::value_type ip::address::operator[](const int& index) const

return data_.at(index);


void ip::address::operator++()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if(location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]++;



::ip::address& ip::address::operator++(int)

auto result(*this);
++(*this);
return result;


void ip::address::operator--()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if (location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]--;



::ip::address& ip::address::operator--(int)

auto result(*this);
--(*this);
return result;


ip::address::iterator ip::address::begin()

return data_.begin();


ip::address::const_iterator ip::address::end() const

return data_.end();


bool ip::operator<(const ip::address& first, const ip::address& second)

return (uint32_t)first() < (uint32_t)second();


bool ip::operator==(const ip::address& first, const ip::address& second)

return (uint32_t)first() == (uint32_t) second();


ip::address::const_iterator ip::address::begin() const

return data_.begin();


ip::address::iterator ip::address::end()

return data_.end();


std::ostream& ip::operator<<(std::ostream& output, const ip::address& address)

std::copy(address.begin(), address.end()-1,
std::ostream_iterator<short>(output, "."));
output << +address[3];
return output;


ip::address ip::from_string(const std::string &view)

auto parts = split(view, '.');
if (parts.size() != 4)

throw invalid_format_exception(view);


return
(ip::address::value_type)std::stoi(parts[0]),
(ip::address::value_type)std::stoi(parts[1]),
(ip::address::value_type)std::stoi(parts[2]),
(ip::address::value_type)std::stoi(parts[3])
;


std::string ip::to_string(const address& address)

std::ostringstream string_stream;
string_stream << address;
return string_stream.str();










share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$







  • 1




    $begingroup$
    networking-ts has an excellent reference implementation: open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4771.pdf#page179
    $endgroup$
    – sudo rm -rf slash
    yesterday






  • 1




    $begingroup$
    Mod Note: Please do not use comments for extended conversations. Comments are intended to resolve unclarities about the post. Use Code Review Chat for extended conversations and in-depth discussions. If you want to critique the code presented in the question, please do so in an answer. Thanks!
    $endgroup$
    – Vogel612
    8 hours ago











  • $begingroup$
    Remember that an ipv4 is not only four numbers separated by 3 dots. 1 = 0.0.0.1, 1.2=1.0.0.2, 1.2.3 = 1.2.0.3, 192.168.0.1 = 3232235521 = 0xC0A80001
    $endgroup$
    – Lenne
    57 mins ago














13












13








13





$begingroup$


I've recently picked up a book that gives various "modern" C++ challenges/solutions. One of the first ones I did was on modeling an IPv4 address in C++. Below is the full implementation; it's also on Github. Any suggestions? My goal is to make this as modern as possible and I'm not sure if there are some C++17 features I'm not taking advantage of.



ip_address.h



#pragma once

#include <array>
#include <string>
#include <string_view>
#include <stdint.h>

namespace ip

/**
* @brief Thrown when there is an invalid ip address passed via
* string.
*/
class invalid_format_exception : public std::exception

std::string invalid_format_;
public:
invalid_format_exception(const std::string &invalid_format);
char const* what() const override;
;

/**
* Class that models a IPv4 address.
*/
class address

public:

#pragma region Type definitions
using value_type = uint8_t;
using reference = value_type & ;
using pointer = value_type * ;
using iterator = std::array<value_type, 4>::iterator;
using const_iterator = std::array<value_type, 4>::const_iterator;
using reverse_iterator = std::array<value_type, 4>::reverse_iterator;
using const_reverse_iterator = std::array<value_type, 4>::const_reverse_iterator;
using size_type = std::array<value_type, 4>::size_type;
#pragma endregion

/**
* @brief Create an IP address representation from the
* four parts of the address definition.
* @param first the first part of the address
* @param second the second part of the address
* @param third the third part of the address.
* @param fourth the fourth part of the address.
* @details Example:
* @code
* ip::address addr(127, 0, 0, 1);
* @endcode
*/
address(const value_type& first, const value_type &second,
const value_type &third, const value_type& fourth);

/**
* @brief Create an IP address representaiton from an
* array.
* @param data the data array.
* @details Example:
* @code
* ip::address addr = 127, 0, 0, 1;
* @endcode
*/
address(const std::array<unsigned char, 4> &data);

/**
* @brief Create an IP adderss representation from a
* unsigned 32 bit integer.
* @param value the integer representation of an IP address.
*/
explicit address(const uint32_t &value);

/**
* @brief Implicit conversion to an unsigned 32 bit integer.
*/
uint32_t operator()() const;

/**
* @brief Access operator.
* @param index the index to access.
*/
reference operator[](const int &index) noexcept(false);

/**
* @brief Const version of the access operator.
*/
value_type operator[](const int &index) const noexcept(false);

/**
* @brief Prefix increment operator.
*/
void operator++();

/**
* @brief Postfix increment operator.
*/
::ip::address& operator++(int);

/**
* @brief Prefix decrement operator.
*/
void operator--();

/**
* @brief Prefix decrement operator.
*/
::ip::address& operator--(int);

iterator begin();
iterator end();
const_iterator begin() const;
const_iterator end() const;
private:
std::array<value_type, 4> data_;
;

bool operator<(const ip::address &first, const ip::address &second);
bool operator==(const ip::address &first, const ip::address &second);
std::ostream& operator<<(std::ostream& output, const ip::address &address);
address from_string(const std::string &view);
std::string to_string(const address& address);



ip_address.cpp



#include <ip_address.h>

#include <iterator>
#include <iostream>
#include <sstream>
#include <regex>
#include <vector>
#include <string>

#pragma region Utilities
template<typename Output>
void split(const std::string &s, char delim, Output result)
std::stringstream ss(s);
std::string item;
while (std::getline(ss, item, delim))
*(result++) = item;



std::vector<std::string> split(const std::string &s, char delim)
std::vector<std::string> elems;
split(s, delim, std::back_inserter(elems));
return elems;


#pragma endregion

ip::invalid_format_exception::invalid_format_exception(const std::string& invalid_format)
: invalid_format_(invalid_format)



char const* ip::invalid_format_exception::what() const

std::ostringstream oss;
oss << "Invalid IP address format: " << invalid_format_;
return oss.str().c_str();


ip::address::address(const value_type & first, const value_type & second, const value_type & third, const value_type & fourth)

data_[0] = first;
data_[1] = second;
data_[2] = third;
data_[3] = fourth;


ip::address::address(const std::array<unsigned char, 4>& data)

data_ = data;


ip::address::address(const uint32_t& value)

data_[0] = value >> 24 & 0xFF;
data_[1] = value >> 16 & 0xFF;
data_[2] = value >> 8 & 0xFF;
data_[3] = value & 0xFF;


uint32_t ip::address::operator()() const

const uint32_t value = data_[0] << 24

ip::address::reference ip::address::operator[](const int& index)

return data_.at(index);


ip::address::value_type ip::address::operator[](const int& index) const

return data_.at(index);


void ip::address::operator++()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if(location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]++;



::ip::address& ip::address::operator++(int)

auto result(*this);
++(*this);
return result;


void ip::address::operator--()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if (location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]--;



::ip::address& ip::address::operator--(int)

auto result(*this);
--(*this);
return result;


ip::address::iterator ip::address::begin()

return data_.begin();


ip::address::const_iterator ip::address::end() const

return data_.end();


bool ip::operator<(const ip::address& first, const ip::address& second)

return (uint32_t)first() < (uint32_t)second();


bool ip::operator==(const ip::address& first, const ip::address& second)

return (uint32_t)first() == (uint32_t) second();


ip::address::const_iterator ip::address::begin() const

return data_.begin();


ip::address::iterator ip::address::end()

return data_.end();


std::ostream& ip::operator<<(std::ostream& output, const ip::address& address)

std::copy(address.begin(), address.end()-1,
std::ostream_iterator<short>(output, "."));
output << +address[3];
return output;


ip::address ip::from_string(const std::string &view)

auto parts = split(view, '.');
if (parts.size() != 4)

throw invalid_format_exception(view);


return
(ip::address::value_type)std::stoi(parts[0]),
(ip::address::value_type)std::stoi(parts[1]),
(ip::address::value_type)std::stoi(parts[2]),
(ip::address::value_type)std::stoi(parts[3])
;


std::string ip::to_string(const address& address)

std::ostringstream string_stream;
string_stream << address;
return string_stream.str();










share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$




I've recently picked up a book that gives various "modern" C++ challenges/solutions. One of the first ones I did was on modeling an IPv4 address in C++. Below is the full implementation; it's also on Github. Any suggestions? My goal is to make this as modern as possible and I'm not sure if there are some C++17 features I'm not taking advantage of.



ip_address.h



#pragma once

#include <array>
#include <string>
#include <string_view>
#include <stdint.h>

namespace ip

/**
* @brief Thrown when there is an invalid ip address passed via
* string.
*/
class invalid_format_exception : public std::exception

std::string invalid_format_;
public:
invalid_format_exception(const std::string &invalid_format);
char const* what() const override;
;

/**
* Class that models a IPv4 address.
*/
class address

public:

#pragma region Type definitions
using value_type = uint8_t;
using reference = value_type & ;
using pointer = value_type * ;
using iterator = std::array<value_type, 4>::iterator;
using const_iterator = std::array<value_type, 4>::const_iterator;
using reverse_iterator = std::array<value_type, 4>::reverse_iterator;
using const_reverse_iterator = std::array<value_type, 4>::const_reverse_iterator;
using size_type = std::array<value_type, 4>::size_type;
#pragma endregion

/**
* @brief Create an IP address representation from the
* four parts of the address definition.
* @param first the first part of the address
* @param second the second part of the address
* @param third the third part of the address.
* @param fourth the fourth part of the address.
* @details Example:
* @code
* ip::address addr(127, 0, 0, 1);
* @endcode
*/
address(const value_type& first, const value_type &second,
const value_type &third, const value_type& fourth);

/**
* @brief Create an IP address representaiton from an
* array.
* @param data the data array.
* @details Example:
* @code
* ip::address addr = 127, 0, 0, 1;
* @endcode
*/
address(const std::array<unsigned char, 4> &data);

/**
* @brief Create an IP adderss representation from a
* unsigned 32 bit integer.
* @param value the integer representation of an IP address.
*/
explicit address(const uint32_t &value);

/**
* @brief Implicit conversion to an unsigned 32 bit integer.
*/
uint32_t operator()() const;

/**
* @brief Access operator.
* @param index the index to access.
*/
reference operator[](const int &index) noexcept(false);

/**
* @brief Const version of the access operator.
*/
value_type operator[](const int &index) const noexcept(false);

/**
* @brief Prefix increment operator.
*/
void operator++();

/**
* @brief Postfix increment operator.
*/
::ip::address& operator++(int);

/**
* @brief Prefix decrement operator.
*/
void operator--();

/**
* @brief Prefix decrement operator.
*/
::ip::address& operator--(int);

iterator begin();
iterator end();
const_iterator begin() const;
const_iterator end() const;
private:
std::array<value_type, 4> data_;
;

bool operator<(const ip::address &first, const ip::address &second);
bool operator==(const ip::address &first, const ip::address &second);
std::ostream& operator<<(std::ostream& output, const ip::address &address);
address from_string(const std::string &view);
std::string to_string(const address& address);



ip_address.cpp



#include <ip_address.h>

#include <iterator>
#include <iostream>
#include <sstream>
#include <regex>
#include <vector>
#include <string>

#pragma region Utilities
template<typename Output>
void split(const std::string &s, char delim, Output result)
std::stringstream ss(s);
std::string item;
while (std::getline(ss, item, delim))
*(result++) = item;



std::vector<std::string> split(const std::string &s, char delim)
std::vector<std::string> elems;
split(s, delim, std::back_inserter(elems));
return elems;


#pragma endregion

ip::invalid_format_exception::invalid_format_exception(const std::string& invalid_format)
: invalid_format_(invalid_format)



char const* ip::invalid_format_exception::what() const

std::ostringstream oss;
oss << "Invalid IP address format: " << invalid_format_;
return oss.str().c_str();


ip::address::address(const value_type & first, const value_type & second, const value_type & third, const value_type & fourth)

data_[0] = first;
data_[1] = second;
data_[2] = third;
data_[3] = fourth;


ip::address::address(const std::array<unsigned char, 4>& data)

data_ = data;


ip::address::address(const uint32_t& value)

data_[0] = value >> 24 & 0xFF;
data_[1] = value >> 16 & 0xFF;
data_[2] = value >> 8 & 0xFF;
data_[3] = value & 0xFF;


uint32_t ip::address::operator()() const

const uint32_t value = data_[0] << 24

ip::address::reference ip::address::operator[](const int& index)

return data_.at(index);


ip::address::value_type ip::address::operator[](const int& index) const

return data_.at(index);


void ip::address::operator++()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if(location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]++;



::ip::address& ip::address::operator++(int)

auto result(*this);
++(*this);
return result;


void ip::address::operator--()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if (location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]--;



::ip::address& ip::address::operator--(int)

auto result(*this);
--(*this);
return result;


ip::address::iterator ip::address::begin()

return data_.begin();


ip::address::const_iterator ip::address::end() const

return data_.end();


bool ip::operator<(const ip::address& first, const ip::address& second)

return (uint32_t)first() < (uint32_t)second();


bool ip::operator==(const ip::address& first, const ip::address& second)

return (uint32_t)first() == (uint32_t) second();


ip::address::const_iterator ip::address::begin() const

return data_.begin();


ip::address::iterator ip::address::end()

return data_.end();


std::ostream& ip::operator<<(std::ostream& output, const ip::address& address)

std::copy(address.begin(), address.end()-1,
std::ostream_iterator<short>(output, "."));
output << +address[3];
return output;


ip::address ip::from_string(const std::string &view)

auto parts = split(view, '.');
if (parts.size() != 4)

throw invalid_format_exception(view);


return
(ip::address::value_type)std::stoi(parts[0]),
(ip::address::value_type)std::stoi(parts[1]),
(ip::address::value_type)std::stoi(parts[2]),
(ip::address::value_type)std::stoi(parts[3])
;


std::string ip::to_string(const address& address)

std::ostringstream string_stream;
string_stream << address;
return string_stream.str();







c++ c++11 ip-address






share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited 1 hour ago









Martin Schröder

247518




247518






New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked 2 days ago









Developer PaulDeveloper Paul

19616




19616




New contributor




Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






Developer Paul is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







  • 1




    $begingroup$
    networking-ts has an excellent reference implementation: open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4771.pdf#page179
    $endgroup$
    – sudo rm -rf slash
    yesterday






  • 1




    $begingroup$
    Mod Note: Please do not use comments for extended conversations. Comments are intended to resolve unclarities about the post. Use Code Review Chat for extended conversations and in-depth discussions. If you want to critique the code presented in the question, please do so in an answer. Thanks!
    $endgroup$
    – Vogel612
    8 hours ago











  • $begingroup$
    Remember that an ipv4 is not only four numbers separated by 3 dots. 1 = 0.0.0.1, 1.2=1.0.0.2, 1.2.3 = 1.2.0.3, 192.168.0.1 = 3232235521 = 0xC0A80001
    $endgroup$
    – Lenne
    57 mins ago













  • 1




    $begingroup$
    networking-ts has an excellent reference implementation: open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4771.pdf#page179
    $endgroup$
    – sudo rm -rf slash
    yesterday






  • 1




    $begingroup$
    Mod Note: Please do not use comments for extended conversations. Comments are intended to resolve unclarities about the post. Use Code Review Chat for extended conversations and in-depth discussions. If you want to critique the code presented in the question, please do so in an answer. Thanks!
    $endgroup$
    – Vogel612
    8 hours ago











  • $begingroup$
    Remember that an ipv4 is not only four numbers separated by 3 dots. 1 = 0.0.0.1, 1.2=1.0.0.2, 1.2.3 = 1.2.0.3, 192.168.0.1 = 3232235521 = 0xC0A80001
    $endgroup$
    – Lenne
    57 mins ago








1




1




$begingroup$
networking-ts has an excellent reference implementation: open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4771.pdf#page179
$endgroup$
– sudo rm -rf slash
yesterday




$begingroup$
networking-ts has an excellent reference implementation: open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4771.pdf#page179
$endgroup$
– sudo rm -rf slash
yesterday




1




1




$begingroup$
Mod Note: Please do not use comments for extended conversations. Comments are intended to resolve unclarities about the post. Use Code Review Chat for extended conversations and in-depth discussions. If you want to critique the code presented in the question, please do so in an answer. Thanks!
$endgroup$
– Vogel612
8 hours ago





$begingroup$
Mod Note: Please do not use comments for extended conversations. Comments are intended to resolve unclarities about the post. Use Code Review Chat for extended conversations and in-depth discussions. If you want to critique the code presented in the question, please do so in an answer. Thanks!
$endgroup$
– Vogel612
8 hours ago













$begingroup$
Remember that an ipv4 is not only four numbers separated by 3 dots. 1 = 0.0.0.1, 1.2=1.0.0.2, 1.2.3 = 1.2.0.3, 192.168.0.1 = 3232235521 = 0xC0A80001
$endgroup$
– Lenne
57 mins ago





$begingroup$
Remember that an ipv4 is not only four numbers separated by 3 dots. 1 = 0.0.0.1, 1.2=1.0.0.2, 1.2.3 = 1.2.0.3, 192.168.0.1 = 3232235521 = 0xC0A80001
$endgroup$
– Lenne
57 mins ago











3 Answers
3






active

oldest

votes


















28












$begingroup$

I think it's very strange that you provide iterators and an operator[] for an IP address. Generally speaking, IP addresses are not considered to be "iterable"; an IP address is just a single address. If you were modeling a subnet mask, like 127.0.0.0/8, then it might make sense to model it as a range of addresses; but if you're modeling just a single address, I don't think it is appropriate at all to model it as a range of octets. What benefit do you gain from that? IMHO: none. None benefit.




As 1201ProgramAlarm already said, your increment and decrement operators' signatures are a bit screwed up (essentially, backwards). Plus:



::ip::address& ip::address::operator++(int)

auto result(*this);
++(*this);
return result;



This one should also have given you a compiler warning (assuming you use any mainstream compiler, such as GCC, Clang, or MSVC). Step number one when writing C++ is always to compile with -W -Wall -Wextra and fix all the warnings prior to publishing your code. The compiler warnings are usually telling you about bugs in your code; and even when they're not technically bugs, you should still fix the warnings, so that none of your coworkers have to read the warnings ever again. Clean code is friendly code!




ip::address::iterator ip::address::begin()

return data_.begin();


ip::address::const_iterator ip::address::end() const

return data_.end();



It is super weird to me that you define these member functions in the order "nonconst begin, const end, const begin, nonconst end." That's harmless, but it's just weird. Also, I recommend defining these functions directly in-line in the body of the class. They're one-liners. You waste space (and thus, waste the reader's time) by defining them out-of-line. That is, I'd write:



 iterator begin() return data_.begin(); 
iterator end() return data_.end();
const_iterator begin() const return data_.begin();
const_iterator end() const return data_.end();
private:
std::array<value_type, 4> data_;


Also, all four of these methods should probably be declared noexcept.




Overloaded comparison operators should always be defined in-line in the body of the class, using the "hidden friend" (a.k.a. "ADL friend," a.k.a. "Barton-Nackman") trick. That is, instead of



class address ... ;

bool operator<(const ip::address &first, const ip::address &second);

bool ip::operator<(const ip::address& first, const ip::address& second)

return (uint32_t)first() < (uint32_t)second();



you should write simply



class address 
// ...

friend bool operator<(const address& a, const address& b)
return uint32_t(a()) < uint32_t(b());

;


Notice that I switched your type-casts from C style to constructor-style, a.k.a. "Python style," just for the heck of it. I find the fewer parentheses the easier it is to read. Also, I switched the verbose first and second to simply a and b: we don't need long names for these extremely locally scoped variables.



But wait, there's more! I initially assumed that first() was a typo — but it's not! You actually declared an overloaded operator():



 /**
* @brief Implicit conversion to an unsigned 32 bit integer.
*/
uint32_t operator()() const;


Why on earth is this an overloaded function-call operator instead of a conversion operator? Worse, why is this any kind of operator at all, when you already went out of your way to declare a free function ip::to_string(const address&)? Why is the conversion to uint32_t not implemented as ip::to_uint32(const address&)?



Consistency is important. Also, compatibility with the rest of the language is important. When you overload operator(), you're making ip::address "callable," which means you're enabling your clients to write things like



ip::address myAddress(127, 0, 0, 1);
std::function<int()> f = myAddress; // !!
assert(f() == 0x7F000001);


Just as with the iterator/range-of-octets business, this functionality strikes me as fundamentally not what an IP address ought to be about. IP addresses aren't ranges, and IP addresses aren't callables. They should be just addresses. To the extent that your ip::address is anything other than just an address, you have actually failed in your stated goal of "modeling an IP address"!




Your operator<< should also be defined in-line.



Anytime you provide operator==, you should also provide operator!= — the language doesn't (yet) provide it for you automatically.



Anytime you provide operator<, you should also provide operator<=, >, and >= — the language doesn't (yet) provide these for you automatically. (But in C++2a you'll have operator<=> to play with!)




void ip::address::operator++()

auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

return data < 255;
);

if(location != std::rend(data_))

const auto r_index = std::distance(data_.rbegin(), location);
auto index = 4 - r_index - 1;
data_[index]++;




It's odd that you write data_.rend() in one place and std::rend(data_) in the other. I recommend the former in both cases, simply because it's shorter.



However, doesn't this code "increment" address(0, 0, 0, 255) to address(0, 0, 1, 255) instead of to address(0, 0, 1, 0)? If so, oops! IMO the clearest and simplest way to write this "odometer algorithm" is simply



address& operator++() noexcept

if (++data_[3] == 0)
if (++data_[2] == 0)
if (++data_[1] == 0)
++data_[0];



return *this;



Short and sweet. Arguably it's overly complicated and "clever" — but look what it's replacing! What it's replacing uses multiple STL algorithms, is three lines longer, and (AFAICT) doesn't even work. So feel free to reduce the "cleverness" of my proposed code even further, if you can; regardless, I claim it's an improvement over the original.






share|improve this answer









$endgroup$








  • 1




    $begingroup$
    Good point about compiling with warning and fixing said warnings. Also the order of definition for the "begin()" and "end()" methods wasn't on purpose. They are supposed to be in the same order as the declaration. Why should "overloaded comparison operators always be defined in-line in the body of the class"? This seems like conjecture to me and I don't see the real value in bloating the class with something that can easily be left outside the class. I do agree with creating a ip::to_uint32t() function so good point there and I overall agree with your comments about consistency.
    $endgroup$
    – Developer Paul
    yesterday











  • $begingroup$
    "I find the fewer parentheses the easier it is to read" — haven't you just increased nesting level of parentheses, leaving total count of them unchanged?
    $endgroup$
    – Ruslan
    yesterday






  • 2




    $begingroup$
    Something else to consider: An address is a small trivial type. Thus, passing by constant reference instead of value is a pessimisation.
    $endgroup$
    – Deduplicator
    yesterday


















10












$begingroup$

You storing the value in std::array<value_type, 4> which is fine. But if you change your mind on the storage type you have to change this in like 10 places. To make this easier it is a good idea to abstract the storage type and then use this storage type in all places.



 using Storage = std::array<value_type, 4>;
using iterator = Storage::iterator;
using const_iterator = Storage::const_iterator;
using reverse_iterator = Storage::reverse_iterator;
using const_reverse_iterator = Storage::const_reverse_iterator;
using size_type = Storage::size_type;


Now if you change the underlying storage type you only have to change it in one place.




What does it mean to increment/decrement an ip address?



 /**
* @brief Prefix increment operator.
*/
void operator++();


What scenario does this make sense?




If there is a test for equality:



bool operator==(const ip::address &first, const ip::address &second);


Then I would expect a test for inequality.




If there is an output operator:



std::ostream& operator<<(std::ostream& output, const ip::address &address);


Then I would expect an input operator.




The standard exceptions (except std::exception itself) already implement what(). You should inherit from one of these rather than std::exception (probably std::runtime_error.



class invalid_format_exception : public std::exception

std::string invalid_format_;
public:
invalid_format_exception(const std::string &invalid_format);
char const* what() const override;
;


This becomes:



struct invalid_format_exception: std::runtime_error

using std::runtime_error::runtime_error; // Pull runtime_error constructor into this class.
;



Are you sure that the IP address is always stored in big endian form?



data_[0] = value >> 24 & 0xFF;
data_[1] = value >> 16 & 0xFF;
data_[2] = value >> 8 & 0xFF;
data_[3] = value & 0xFF;


I would double check and also add a big comment that that is what you expect.




The increment operator looks complicated.

I think it can really be simplified by using some existing functions you have identified.



void ip::address::operator++()

uint32_t value = (*this); // convert to 32 bit number
++value; // Add 1
(*this) = address(value); // convert back to address and copy/move




Functions that simply forward calls just put them in the class and forget about them. There is nothing to maintain and it need not take up multiple lines in the source file:



ip::address::iterator ip::address::begin()

return data_.begin();


ip::address::const_iterator ip::address::end() const

return data_.end();


// I would just do the following the header:


iterator begin() return data_.begin();
iterator end() return data_.end();
const_iterator begin() const return data_.begin();
const_iterator end() const return data_.end();


You are of course missing a few:



 const_iterator cbegin() const return data_.cbegin();
reverse_iterator rbegin() return data_.rbegin();

// You can add the end() versions.





share|improve this answer









$endgroup$












  • $begingroup$
    "What does it mean to increment/decrement an ip address?" It means exactly what it sounds like, no? The main purpose was to be able to enumerate a number of addresses in a given range. I didn't mention this in the OP, but it was another part of the challenge. Good point about the storage type, that will definitely be useful. As far as putting the defs of begin(), end() and the like in the header I disagree with that sentiment. I do not like mixing and matching where functions are defined. They will either be all in the header, or all in the source file; not both.
    $endgroup$
    – Developer Paul
    yesterday










  • $begingroup$
    @DeveloperPaul: It should not matter were the definitions are. All good development tools will automatically jump to definition when asked. I use vi and it still jumps to the function definition when I ask without me knowing where the file is.
    $endgroup$
    – Martin York
    yesterday










  • $begingroup$
    It means exactly what it sounds like, no? Sure but why. This is not a property of an address. Consecutive addresses have no relationship. So this should not be in the address class. You could put it in a helper class that allows you to scan addresses but it should not be part of the address class.
    $endgroup$
    – Martin York
    yesterday










  • $begingroup$
    I see your point now. Seems like I've included quite a bit in the address class that doesn't need to be there.
    $endgroup$
    – Developer Paul
    yesterday


















6












$begingroup$

You're passing fundamental types by const reference. These are better off just being passed by value. So you'd get things like



explicit address(uint32_t value);
reference operator[](int index) noexcept(false);


Your prefix increment and decrement operators should return a reference to the incremented value.



address &operator++() /* ... */ return *this; 
address &operator--() /* ... */ return *this;


This will allow expressions like addr = ++other_addr;. (Note that, since you're in the address class, you can just name the class, you don't need to specify scope with ::ip::address).



Your postfix increment and decrement operators have a bug, because they return a reference to a local variable. The return types should be a value.



address operator++(int);
address operator--(int);


For readability and clarity, expressions mixing shifts and bit masking should use parentheses:



data_[0] = (value >> 24) & 0xFF;





share|improve this answer











$endgroup$













    Your Answer





    StackExchange.ifUsing("editor", function ()
    return StackExchange.using("mathjaxEditing", function ()
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    );
    );
    , "mathjax-editing");

    StackExchange.ifUsing("editor", function ()
    StackExchange.using("externalEditor", function ()
    StackExchange.using("snippets", function ()
    StackExchange.snippets.init();
    );
    );
    , "code-snippets");

    StackExchange.ready(function()
    var channelOptions =
    tags: "".split(" "),
    id: "196"
    ;
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function()
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled)
    StackExchange.using("snippets", function()
    createEditor();
    );

    else
    createEditor();

    );

    function createEditor()
    StackExchange.prepareEditor(
    heartbeatType: 'answer',
    autoActivateHeartbeat: false,
    convertImagesToLinks: false,
    noModals: true,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    imageUploader:
    brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
    contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
    allowUrls: true
    ,
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    );



    );






    Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.









    draft saved

    draft discarded


















    StackExchange.ready(
    function ()
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216894%2fmodeling-an-ipv4-address%23new-answer', 'question_page');

    );

    Post as a guest















    Required, but never shown

























    3 Answers
    3






    active

    oldest

    votes








    3 Answers
    3






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes









    28












    $begingroup$

    I think it's very strange that you provide iterators and an operator[] for an IP address. Generally speaking, IP addresses are not considered to be "iterable"; an IP address is just a single address. If you were modeling a subnet mask, like 127.0.0.0/8, then it might make sense to model it as a range of addresses; but if you're modeling just a single address, I don't think it is appropriate at all to model it as a range of octets. What benefit do you gain from that? IMHO: none. None benefit.




    As 1201ProgramAlarm already said, your increment and decrement operators' signatures are a bit screwed up (essentially, backwards). Plus:



    ::ip::address& ip::address::operator++(int)

    auto result(*this);
    ++(*this);
    return result;



    This one should also have given you a compiler warning (assuming you use any mainstream compiler, such as GCC, Clang, or MSVC). Step number one when writing C++ is always to compile with -W -Wall -Wextra and fix all the warnings prior to publishing your code. The compiler warnings are usually telling you about bugs in your code; and even when they're not technically bugs, you should still fix the warnings, so that none of your coworkers have to read the warnings ever again. Clean code is friendly code!




    ip::address::iterator ip::address::begin()

    return data_.begin();


    ip::address::const_iterator ip::address::end() const

    return data_.end();



    It is super weird to me that you define these member functions in the order "nonconst begin, const end, const begin, nonconst end." That's harmless, but it's just weird. Also, I recommend defining these functions directly in-line in the body of the class. They're one-liners. You waste space (and thus, waste the reader's time) by defining them out-of-line. That is, I'd write:



     iterator begin() return data_.begin(); 
    iterator end() return data_.end();
    const_iterator begin() const return data_.begin();
    const_iterator end() const return data_.end();
    private:
    std::array<value_type, 4> data_;


    Also, all four of these methods should probably be declared noexcept.




    Overloaded comparison operators should always be defined in-line in the body of the class, using the "hidden friend" (a.k.a. "ADL friend," a.k.a. "Barton-Nackman") trick. That is, instead of



    class address ... ;

    bool operator<(const ip::address &first, const ip::address &second);

    bool ip::operator<(const ip::address& first, const ip::address& second)

    return (uint32_t)first() < (uint32_t)second();



    you should write simply



    class address 
    // ...

    friend bool operator<(const address& a, const address& b)
    return uint32_t(a()) < uint32_t(b());

    ;


    Notice that I switched your type-casts from C style to constructor-style, a.k.a. "Python style," just for the heck of it. I find the fewer parentheses the easier it is to read. Also, I switched the verbose first and second to simply a and b: we don't need long names for these extremely locally scoped variables.



    But wait, there's more! I initially assumed that first() was a typo — but it's not! You actually declared an overloaded operator():



     /**
    * @brief Implicit conversion to an unsigned 32 bit integer.
    */
    uint32_t operator()() const;


    Why on earth is this an overloaded function-call operator instead of a conversion operator? Worse, why is this any kind of operator at all, when you already went out of your way to declare a free function ip::to_string(const address&)? Why is the conversion to uint32_t not implemented as ip::to_uint32(const address&)?



    Consistency is important. Also, compatibility with the rest of the language is important. When you overload operator(), you're making ip::address "callable," which means you're enabling your clients to write things like



    ip::address myAddress(127, 0, 0, 1);
    std::function<int()> f = myAddress; // !!
    assert(f() == 0x7F000001);


    Just as with the iterator/range-of-octets business, this functionality strikes me as fundamentally not what an IP address ought to be about. IP addresses aren't ranges, and IP addresses aren't callables. They should be just addresses. To the extent that your ip::address is anything other than just an address, you have actually failed in your stated goal of "modeling an IP address"!




    Your operator<< should also be defined in-line.



    Anytime you provide operator==, you should also provide operator!= — the language doesn't (yet) provide it for you automatically.



    Anytime you provide operator<, you should also provide operator<=, >, and >= — the language doesn't (yet) provide these for you automatically. (But in C++2a you'll have operator<=> to play with!)




    void ip::address::operator++()

    auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

    return data < 255;
    );

    if(location != std::rend(data_))

    const auto r_index = std::distance(data_.rbegin(), location);
    auto index = 4 - r_index - 1;
    data_[index]++;




    It's odd that you write data_.rend() in one place and std::rend(data_) in the other. I recommend the former in both cases, simply because it's shorter.



    However, doesn't this code "increment" address(0, 0, 0, 255) to address(0, 0, 1, 255) instead of to address(0, 0, 1, 0)? If so, oops! IMO the clearest and simplest way to write this "odometer algorithm" is simply



    address& operator++() noexcept

    if (++data_[3] == 0)
    if (++data_[2] == 0)
    if (++data_[1] == 0)
    ++data_[0];



    return *this;



    Short and sweet. Arguably it's overly complicated and "clever" — but look what it's replacing! What it's replacing uses multiple STL algorithms, is three lines longer, and (AFAICT) doesn't even work. So feel free to reduce the "cleverness" of my proposed code even further, if you can; regardless, I claim it's an improvement over the original.






    share|improve this answer









    $endgroup$








    • 1




      $begingroup$
      Good point about compiling with warning and fixing said warnings. Also the order of definition for the "begin()" and "end()" methods wasn't on purpose. They are supposed to be in the same order as the declaration. Why should "overloaded comparison operators always be defined in-line in the body of the class"? This seems like conjecture to me and I don't see the real value in bloating the class with something that can easily be left outside the class. I do agree with creating a ip::to_uint32t() function so good point there and I overall agree with your comments about consistency.
      $endgroup$
      – Developer Paul
      yesterday











    • $begingroup$
      "I find the fewer parentheses the easier it is to read" — haven't you just increased nesting level of parentheses, leaving total count of them unchanged?
      $endgroup$
      – Ruslan
      yesterday






    • 2




      $begingroup$
      Something else to consider: An address is a small trivial type. Thus, passing by constant reference instead of value is a pessimisation.
      $endgroup$
      – Deduplicator
      yesterday















    28












    $begingroup$

    I think it's very strange that you provide iterators and an operator[] for an IP address. Generally speaking, IP addresses are not considered to be "iterable"; an IP address is just a single address. If you were modeling a subnet mask, like 127.0.0.0/8, then it might make sense to model it as a range of addresses; but if you're modeling just a single address, I don't think it is appropriate at all to model it as a range of octets. What benefit do you gain from that? IMHO: none. None benefit.




    As 1201ProgramAlarm already said, your increment and decrement operators' signatures are a bit screwed up (essentially, backwards). Plus:



    ::ip::address& ip::address::operator++(int)

    auto result(*this);
    ++(*this);
    return result;



    This one should also have given you a compiler warning (assuming you use any mainstream compiler, such as GCC, Clang, or MSVC). Step number one when writing C++ is always to compile with -W -Wall -Wextra and fix all the warnings prior to publishing your code. The compiler warnings are usually telling you about bugs in your code; and even when they're not technically bugs, you should still fix the warnings, so that none of your coworkers have to read the warnings ever again. Clean code is friendly code!




    ip::address::iterator ip::address::begin()

    return data_.begin();


    ip::address::const_iterator ip::address::end() const

    return data_.end();



    It is super weird to me that you define these member functions in the order "nonconst begin, const end, const begin, nonconst end." That's harmless, but it's just weird. Also, I recommend defining these functions directly in-line in the body of the class. They're one-liners. You waste space (and thus, waste the reader's time) by defining them out-of-line. That is, I'd write:



     iterator begin() return data_.begin(); 
    iterator end() return data_.end();
    const_iterator begin() const return data_.begin();
    const_iterator end() const return data_.end();
    private:
    std::array<value_type, 4> data_;


    Also, all four of these methods should probably be declared noexcept.




    Overloaded comparison operators should always be defined in-line in the body of the class, using the "hidden friend" (a.k.a. "ADL friend," a.k.a. "Barton-Nackman") trick. That is, instead of



    class address ... ;

    bool operator<(const ip::address &first, const ip::address &second);

    bool ip::operator<(const ip::address& first, const ip::address& second)

    return (uint32_t)first() < (uint32_t)second();



    you should write simply



    class address 
    // ...

    friend bool operator<(const address& a, const address& b)
    return uint32_t(a()) < uint32_t(b());

    ;


    Notice that I switched your type-casts from C style to constructor-style, a.k.a. "Python style," just for the heck of it. I find the fewer parentheses the easier it is to read. Also, I switched the verbose first and second to simply a and b: we don't need long names for these extremely locally scoped variables.



    But wait, there's more! I initially assumed that first() was a typo — but it's not! You actually declared an overloaded operator():



     /**
    * @brief Implicit conversion to an unsigned 32 bit integer.
    */
    uint32_t operator()() const;


    Why on earth is this an overloaded function-call operator instead of a conversion operator? Worse, why is this any kind of operator at all, when you already went out of your way to declare a free function ip::to_string(const address&)? Why is the conversion to uint32_t not implemented as ip::to_uint32(const address&)?



    Consistency is important. Also, compatibility with the rest of the language is important. When you overload operator(), you're making ip::address "callable," which means you're enabling your clients to write things like



    ip::address myAddress(127, 0, 0, 1);
    std::function<int()> f = myAddress; // !!
    assert(f() == 0x7F000001);


    Just as with the iterator/range-of-octets business, this functionality strikes me as fundamentally not what an IP address ought to be about. IP addresses aren't ranges, and IP addresses aren't callables. They should be just addresses. To the extent that your ip::address is anything other than just an address, you have actually failed in your stated goal of "modeling an IP address"!




    Your operator<< should also be defined in-line.



    Anytime you provide operator==, you should also provide operator!= — the language doesn't (yet) provide it for you automatically.



    Anytime you provide operator<, you should also provide operator<=, >, and >= — the language doesn't (yet) provide these for you automatically. (But in C++2a you'll have operator<=> to play with!)




    void ip::address::operator++()

    auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

    return data < 255;
    );

    if(location != std::rend(data_))

    const auto r_index = std::distance(data_.rbegin(), location);
    auto index = 4 - r_index - 1;
    data_[index]++;




    It's odd that you write data_.rend() in one place and std::rend(data_) in the other. I recommend the former in both cases, simply because it's shorter.



    However, doesn't this code "increment" address(0, 0, 0, 255) to address(0, 0, 1, 255) instead of to address(0, 0, 1, 0)? If so, oops! IMO the clearest and simplest way to write this "odometer algorithm" is simply



    address& operator++() noexcept

    if (++data_[3] == 0)
    if (++data_[2] == 0)
    if (++data_[1] == 0)
    ++data_[0];



    return *this;



    Short and sweet. Arguably it's overly complicated and "clever" — but look what it's replacing! What it's replacing uses multiple STL algorithms, is three lines longer, and (AFAICT) doesn't even work. So feel free to reduce the "cleverness" of my proposed code even further, if you can; regardless, I claim it's an improvement over the original.






    share|improve this answer









    $endgroup$








    • 1




      $begingroup$
      Good point about compiling with warning and fixing said warnings. Also the order of definition for the "begin()" and "end()" methods wasn't on purpose. They are supposed to be in the same order as the declaration. Why should "overloaded comparison operators always be defined in-line in the body of the class"? This seems like conjecture to me and I don't see the real value in bloating the class with something that can easily be left outside the class. I do agree with creating a ip::to_uint32t() function so good point there and I overall agree with your comments about consistency.
      $endgroup$
      – Developer Paul
      yesterday











    • $begingroup$
      "I find the fewer parentheses the easier it is to read" — haven't you just increased nesting level of parentheses, leaving total count of them unchanged?
      $endgroup$
      – Ruslan
      yesterday






    • 2




      $begingroup$
      Something else to consider: An address is a small trivial type. Thus, passing by constant reference instead of value is a pessimisation.
      $endgroup$
      – Deduplicator
      yesterday













    28












    28








    28





    $begingroup$

    I think it's very strange that you provide iterators and an operator[] for an IP address. Generally speaking, IP addresses are not considered to be "iterable"; an IP address is just a single address. If you were modeling a subnet mask, like 127.0.0.0/8, then it might make sense to model it as a range of addresses; but if you're modeling just a single address, I don't think it is appropriate at all to model it as a range of octets. What benefit do you gain from that? IMHO: none. None benefit.




    As 1201ProgramAlarm already said, your increment and decrement operators' signatures are a bit screwed up (essentially, backwards). Plus:



    ::ip::address& ip::address::operator++(int)

    auto result(*this);
    ++(*this);
    return result;



    This one should also have given you a compiler warning (assuming you use any mainstream compiler, such as GCC, Clang, or MSVC). Step number one when writing C++ is always to compile with -W -Wall -Wextra and fix all the warnings prior to publishing your code. The compiler warnings are usually telling you about bugs in your code; and even when they're not technically bugs, you should still fix the warnings, so that none of your coworkers have to read the warnings ever again. Clean code is friendly code!




    ip::address::iterator ip::address::begin()

    return data_.begin();


    ip::address::const_iterator ip::address::end() const

    return data_.end();



    It is super weird to me that you define these member functions in the order "nonconst begin, const end, const begin, nonconst end." That's harmless, but it's just weird. Also, I recommend defining these functions directly in-line in the body of the class. They're one-liners. You waste space (and thus, waste the reader's time) by defining them out-of-line. That is, I'd write:



     iterator begin() return data_.begin(); 
    iterator end() return data_.end();
    const_iterator begin() const return data_.begin();
    const_iterator end() const return data_.end();
    private:
    std::array<value_type, 4> data_;


    Also, all four of these methods should probably be declared noexcept.




    Overloaded comparison operators should always be defined in-line in the body of the class, using the "hidden friend" (a.k.a. "ADL friend," a.k.a. "Barton-Nackman") trick. That is, instead of



    class address ... ;

    bool operator<(const ip::address &first, const ip::address &second);

    bool ip::operator<(const ip::address& first, const ip::address& second)

    return (uint32_t)first() < (uint32_t)second();



    you should write simply



    class address 
    // ...

    friend bool operator<(const address& a, const address& b)
    return uint32_t(a()) < uint32_t(b());

    ;


    Notice that I switched your type-casts from C style to constructor-style, a.k.a. "Python style," just for the heck of it. I find the fewer parentheses the easier it is to read. Also, I switched the verbose first and second to simply a and b: we don't need long names for these extremely locally scoped variables.



    But wait, there's more! I initially assumed that first() was a typo — but it's not! You actually declared an overloaded operator():



     /**
    * @brief Implicit conversion to an unsigned 32 bit integer.
    */
    uint32_t operator()() const;


    Why on earth is this an overloaded function-call operator instead of a conversion operator? Worse, why is this any kind of operator at all, when you already went out of your way to declare a free function ip::to_string(const address&)? Why is the conversion to uint32_t not implemented as ip::to_uint32(const address&)?



    Consistency is important. Also, compatibility with the rest of the language is important. When you overload operator(), you're making ip::address "callable," which means you're enabling your clients to write things like



    ip::address myAddress(127, 0, 0, 1);
    std::function<int()> f = myAddress; // !!
    assert(f() == 0x7F000001);


    Just as with the iterator/range-of-octets business, this functionality strikes me as fundamentally not what an IP address ought to be about. IP addresses aren't ranges, and IP addresses aren't callables. They should be just addresses. To the extent that your ip::address is anything other than just an address, you have actually failed in your stated goal of "modeling an IP address"!




    Your operator<< should also be defined in-line.



    Anytime you provide operator==, you should also provide operator!= — the language doesn't (yet) provide it for you automatically.



    Anytime you provide operator<, you should also provide operator<=, >, and >= — the language doesn't (yet) provide these for you automatically. (But in C++2a you'll have operator<=> to play with!)




    void ip::address::operator++()

    auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

    return data < 255;
    );

    if(location != std::rend(data_))

    const auto r_index = std::distance(data_.rbegin(), location);
    auto index = 4 - r_index - 1;
    data_[index]++;




    It's odd that you write data_.rend() in one place and std::rend(data_) in the other. I recommend the former in both cases, simply because it's shorter.



    However, doesn't this code "increment" address(0, 0, 0, 255) to address(0, 0, 1, 255) instead of to address(0, 0, 1, 0)? If so, oops! IMO the clearest and simplest way to write this "odometer algorithm" is simply



    address& operator++() noexcept

    if (++data_[3] == 0)
    if (++data_[2] == 0)
    if (++data_[1] == 0)
    ++data_[0];



    return *this;



    Short and sweet. Arguably it's overly complicated and "clever" — but look what it's replacing! What it's replacing uses multiple STL algorithms, is three lines longer, and (AFAICT) doesn't even work. So feel free to reduce the "cleverness" of my proposed code even further, if you can; regardless, I claim it's an improvement over the original.






    share|improve this answer









    $endgroup$



    I think it's very strange that you provide iterators and an operator[] for an IP address. Generally speaking, IP addresses are not considered to be "iterable"; an IP address is just a single address. If you were modeling a subnet mask, like 127.0.0.0/8, then it might make sense to model it as a range of addresses; but if you're modeling just a single address, I don't think it is appropriate at all to model it as a range of octets. What benefit do you gain from that? IMHO: none. None benefit.




    As 1201ProgramAlarm already said, your increment and decrement operators' signatures are a bit screwed up (essentially, backwards). Plus:



    ::ip::address& ip::address::operator++(int)

    auto result(*this);
    ++(*this);
    return result;



    This one should also have given you a compiler warning (assuming you use any mainstream compiler, such as GCC, Clang, or MSVC). Step number one when writing C++ is always to compile with -W -Wall -Wextra and fix all the warnings prior to publishing your code. The compiler warnings are usually telling you about bugs in your code; and even when they're not technically bugs, you should still fix the warnings, so that none of your coworkers have to read the warnings ever again. Clean code is friendly code!




    ip::address::iterator ip::address::begin()

    return data_.begin();


    ip::address::const_iterator ip::address::end() const

    return data_.end();



    It is super weird to me that you define these member functions in the order "nonconst begin, const end, const begin, nonconst end." That's harmless, but it's just weird. Also, I recommend defining these functions directly in-line in the body of the class. They're one-liners. You waste space (and thus, waste the reader's time) by defining them out-of-line. That is, I'd write:



     iterator begin() return data_.begin(); 
    iterator end() return data_.end();
    const_iterator begin() const return data_.begin();
    const_iterator end() const return data_.end();
    private:
    std::array<value_type, 4> data_;


    Also, all four of these methods should probably be declared noexcept.




    Overloaded comparison operators should always be defined in-line in the body of the class, using the "hidden friend" (a.k.a. "ADL friend," a.k.a. "Barton-Nackman") trick. That is, instead of



    class address ... ;

    bool operator<(const ip::address &first, const ip::address &second);

    bool ip::operator<(const ip::address& first, const ip::address& second)

    return (uint32_t)first() < (uint32_t)second();



    you should write simply



    class address 
    // ...

    friend bool operator<(const address& a, const address& b)
    return uint32_t(a()) < uint32_t(b());

    ;


    Notice that I switched your type-casts from C style to constructor-style, a.k.a. "Python style," just for the heck of it. I find the fewer parentheses the easier it is to read. Also, I switched the verbose first and second to simply a and b: we don't need long names for these extremely locally scoped variables.



    But wait, there's more! I initially assumed that first() was a typo — but it's not! You actually declared an overloaded operator():



     /**
    * @brief Implicit conversion to an unsigned 32 bit integer.
    */
    uint32_t operator()() const;


    Why on earth is this an overloaded function-call operator instead of a conversion operator? Worse, why is this any kind of operator at all, when you already went out of your way to declare a free function ip::to_string(const address&)? Why is the conversion to uint32_t not implemented as ip::to_uint32(const address&)?



    Consistency is important. Also, compatibility with the rest of the language is important. When you overload operator(), you're making ip::address "callable," which means you're enabling your clients to write things like



    ip::address myAddress(127, 0, 0, 1);
    std::function<int()> f = myAddress; // !!
    assert(f() == 0x7F000001);


    Just as with the iterator/range-of-octets business, this functionality strikes me as fundamentally not what an IP address ought to be about. IP addresses aren't ranges, and IP addresses aren't callables. They should be just addresses. To the extent that your ip::address is anything other than just an address, you have actually failed in your stated goal of "modeling an IP address"!




    Your operator<< should also be defined in-line.



    Anytime you provide operator==, you should also provide operator!= — the language doesn't (yet) provide it for you automatically.



    Anytime you provide operator<, you should also provide operator<=, >, and >= — the language doesn't (yet) provide these for you automatically. (But in C++2a you'll have operator<=> to play with!)




    void ip::address::operator++()

    auto location = std::find_if(data_.rbegin(), data_.rend(), [](const unsigned char& data)

    return data < 255;
    );

    if(location != std::rend(data_))

    const auto r_index = std::distance(data_.rbegin(), location);
    auto index = 4 - r_index - 1;
    data_[index]++;




    It's odd that you write data_.rend() in one place and std::rend(data_) in the other. I recommend the former in both cases, simply because it's shorter.



    However, doesn't this code "increment" address(0, 0, 0, 255) to address(0, 0, 1, 255) instead of to address(0, 0, 1, 0)? If so, oops! IMO the clearest and simplest way to write this "odometer algorithm" is simply



    address& operator++() noexcept

    if (++data_[3] == 0)
    if (++data_[2] == 0)
    if (++data_[1] == 0)
    ++data_[0];



    return *this;



    Short and sweet. Arguably it's overly complicated and "clever" — but look what it's replacing! What it's replacing uses multiple STL algorithms, is three lines longer, and (AFAICT) doesn't even work. So feel free to reduce the "cleverness" of my proposed code even further, if you can; regardless, I claim it's an improvement over the original.







    share|improve this answer












    share|improve this answer



    share|improve this answer










    answered yesterday









    QuuxplusoneQuuxplusone

    13.3k12165




    13.3k12165







    • 1




      $begingroup$
      Good point about compiling with warning and fixing said warnings. Also the order of definition for the "begin()" and "end()" methods wasn't on purpose. They are supposed to be in the same order as the declaration. Why should "overloaded comparison operators always be defined in-line in the body of the class"? This seems like conjecture to me and I don't see the real value in bloating the class with something that can easily be left outside the class. I do agree with creating a ip::to_uint32t() function so good point there and I overall agree with your comments about consistency.
      $endgroup$
      – Developer Paul
      yesterday











    • $begingroup$
      "I find the fewer parentheses the easier it is to read" — haven't you just increased nesting level of parentheses, leaving total count of them unchanged?
      $endgroup$
      – Ruslan
      yesterday






    • 2




      $begingroup$
      Something else to consider: An address is a small trivial type. Thus, passing by constant reference instead of value is a pessimisation.
      $endgroup$
      – Deduplicator
      yesterday












    • 1




      $begingroup$
      Good point about compiling with warning and fixing said warnings. Also the order of definition for the "begin()" and "end()" methods wasn't on purpose. They are supposed to be in the same order as the declaration. Why should "overloaded comparison operators always be defined in-line in the body of the class"? This seems like conjecture to me and I don't see the real value in bloating the class with something that can easily be left outside the class. I do agree with creating a ip::to_uint32t() function so good point there and I overall agree with your comments about consistency.
      $endgroup$
      – Developer Paul
      yesterday











    • $begingroup$
      "I find the fewer parentheses the easier it is to read" — haven't you just increased nesting level of parentheses, leaving total count of them unchanged?
      $endgroup$
      – Ruslan
      yesterday






    • 2




      $begingroup$
      Something else to consider: An address is a small trivial type. Thus, passing by constant reference instead of value is a pessimisation.
      $endgroup$
      – Deduplicator
      yesterday







    1




    1




    $begingroup$
    Good point about compiling with warning and fixing said warnings. Also the order of definition for the "begin()" and "end()" methods wasn't on purpose. They are supposed to be in the same order as the declaration. Why should "overloaded comparison operators always be defined in-line in the body of the class"? This seems like conjecture to me and I don't see the real value in bloating the class with something that can easily be left outside the class. I do agree with creating a ip::to_uint32t() function so good point there and I overall agree with your comments about consistency.
    $endgroup$
    – Developer Paul
    yesterday





    $begingroup$
    Good point about compiling with warning and fixing said warnings. Also the order of definition for the "begin()" and "end()" methods wasn't on purpose. They are supposed to be in the same order as the declaration. Why should "overloaded comparison operators always be defined in-line in the body of the class"? This seems like conjecture to me and I don't see the real value in bloating the class with something that can easily be left outside the class. I do agree with creating a ip::to_uint32t() function so good point there and I overall agree with your comments about consistency.
    $endgroup$
    – Developer Paul
    yesterday













    $begingroup$
    "I find the fewer parentheses the easier it is to read" — haven't you just increased nesting level of parentheses, leaving total count of them unchanged?
    $endgroup$
    – Ruslan
    yesterday




    $begingroup$
    "I find the fewer parentheses the easier it is to read" — haven't you just increased nesting level of parentheses, leaving total count of them unchanged?
    $endgroup$
    – Ruslan
    yesterday




    2




    2




    $begingroup$
    Something else to consider: An address is a small trivial type. Thus, passing by constant reference instead of value is a pessimisation.
    $endgroup$
    – Deduplicator
    yesterday




    $begingroup$
    Something else to consider: An address is a small trivial type. Thus, passing by constant reference instead of value is a pessimisation.
    $endgroup$
    – Deduplicator
    yesterday













    10












    $begingroup$

    You storing the value in std::array<value_type, 4> which is fine. But if you change your mind on the storage type you have to change this in like 10 places. To make this easier it is a good idea to abstract the storage type and then use this storage type in all places.



     using Storage = std::array<value_type, 4>;
    using iterator = Storage::iterator;
    using const_iterator = Storage::const_iterator;
    using reverse_iterator = Storage::reverse_iterator;
    using const_reverse_iterator = Storage::const_reverse_iterator;
    using size_type = Storage::size_type;


    Now if you change the underlying storage type you only have to change it in one place.




    What does it mean to increment/decrement an ip address?



     /**
    * @brief Prefix increment operator.
    */
    void operator++();


    What scenario does this make sense?




    If there is a test for equality:



    bool operator==(const ip::address &first, const ip::address &second);


    Then I would expect a test for inequality.




    If there is an output operator:



    std::ostream& operator<<(std::ostream& output, const ip::address &address);


    Then I would expect an input operator.




    The standard exceptions (except std::exception itself) already implement what(). You should inherit from one of these rather than std::exception (probably std::runtime_error.



    class invalid_format_exception : public std::exception

    std::string invalid_format_;
    public:
    invalid_format_exception(const std::string &invalid_format);
    char const* what() const override;
    ;


    This becomes:



    struct invalid_format_exception: std::runtime_error

    using std::runtime_error::runtime_error; // Pull runtime_error constructor into this class.
    ;



    Are you sure that the IP address is always stored in big endian form?



    data_[0] = value >> 24 & 0xFF;
    data_[1] = value >> 16 & 0xFF;
    data_[2] = value >> 8 & 0xFF;
    data_[3] = value & 0xFF;


    I would double check and also add a big comment that that is what you expect.




    The increment operator looks complicated.

    I think it can really be simplified by using some existing functions you have identified.



    void ip::address::operator++()

    uint32_t value = (*this); // convert to 32 bit number
    ++value; // Add 1
    (*this) = address(value); // convert back to address and copy/move




    Functions that simply forward calls just put them in the class and forget about them. There is nothing to maintain and it need not take up multiple lines in the source file:



    ip::address::iterator ip::address::begin()

    return data_.begin();


    ip::address::const_iterator ip::address::end() const

    return data_.end();


    // I would just do the following the header:


    iterator begin() return data_.begin();
    iterator end() return data_.end();
    const_iterator begin() const return data_.begin();
    const_iterator end() const return data_.end();


    You are of course missing a few:



     const_iterator cbegin() const return data_.cbegin();
    reverse_iterator rbegin() return data_.rbegin();

    // You can add the end() versions.





    share|improve this answer









    $endgroup$












    • $begingroup$
      "What does it mean to increment/decrement an ip address?" It means exactly what it sounds like, no? The main purpose was to be able to enumerate a number of addresses in a given range. I didn't mention this in the OP, but it was another part of the challenge. Good point about the storage type, that will definitely be useful. As far as putting the defs of begin(), end() and the like in the header I disagree with that sentiment. I do not like mixing and matching where functions are defined. They will either be all in the header, or all in the source file; not both.
      $endgroup$
      – Developer Paul
      yesterday










    • $begingroup$
      @DeveloperPaul: It should not matter were the definitions are. All good development tools will automatically jump to definition when asked. I use vi and it still jumps to the function definition when I ask without me knowing where the file is.
      $endgroup$
      – Martin York
      yesterday










    • $begingroup$
      It means exactly what it sounds like, no? Sure but why. This is not a property of an address. Consecutive addresses have no relationship. So this should not be in the address class. You could put it in a helper class that allows you to scan addresses but it should not be part of the address class.
      $endgroup$
      – Martin York
      yesterday










    • $begingroup$
      I see your point now. Seems like I've included quite a bit in the address class that doesn't need to be there.
      $endgroup$
      – Developer Paul
      yesterday















    10












    $begingroup$

    You storing the value in std::array<value_type, 4> which is fine. But if you change your mind on the storage type you have to change this in like 10 places. To make this easier it is a good idea to abstract the storage type and then use this storage type in all places.



     using Storage = std::array<value_type, 4>;
    using iterator = Storage::iterator;
    using const_iterator = Storage::const_iterator;
    using reverse_iterator = Storage::reverse_iterator;
    using const_reverse_iterator = Storage::const_reverse_iterator;
    using size_type = Storage::size_type;


    Now if you change the underlying storage type you only have to change it in one place.




    What does it mean to increment/decrement an ip address?



     /**
    * @brief Prefix increment operator.
    */
    void operator++();


    What scenario does this make sense?




    If there is a test for equality:



    bool operator==(const ip::address &first, const ip::address &second);


    Then I would expect a test for inequality.




    If there is an output operator:



    std::ostream& operator<<(std::ostream& output, const ip::address &address);


    Then I would expect an input operator.




    The standard exceptions (except std::exception itself) already implement what(). You should inherit from one of these rather than std::exception (probably std::runtime_error.



    class invalid_format_exception : public std::exception

    std::string invalid_format_;
    public:
    invalid_format_exception(const std::string &invalid_format);
    char const* what() const override;
    ;


    This becomes:



    struct invalid_format_exception: std::runtime_error

    using std::runtime_error::runtime_error; // Pull runtime_error constructor into this class.
    ;



    Are you sure that the IP address is always stored in big endian form?



    data_[0] = value >> 24 & 0xFF;
    data_[1] = value >> 16 & 0xFF;
    data_[2] = value >> 8 & 0xFF;
    data_[3] = value & 0xFF;


    I would double check and also add a big comment that that is what you expect.




    The increment operator looks complicated.

    I think it can really be simplified by using some existing functions you have identified.



    void ip::address::operator++()

    uint32_t value = (*this); // convert to 32 bit number
    ++value; // Add 1
    (*this) = address(value); // convert back to address and copy/move




    Functions that simply forward calls just put them in the class and forget about them. There is nothing to maintain and it need not take up multiple lines in the source file:



    ip::address::iterator ip::address::begin()

    return data_.begin();


    ip::address::const_iterator ip::address::end() const

    return data_.end();


    // I would just do the following the header:


    iterator begin() return data_.begin();
    iterator end() return data_.end();
    const_iterator begin() const return data_.begin();
    const_iterator end() const return data_.end();


    You are of course missing a few:



     const_iterator cbegin() const return data_.cbegin();
    reverse_iterator rbegin() return data_.rbegin();

    // You can add the end() versions.





    share|improve this answer









    $endgroup$












    • $begingroup$
      "What does it mean to increment/decrement an ip address?" It means exactly what it sounds like, no? The main purpose was to be able to enumerate a number of addresses in a given range. I didn't mention this in the OP, but it was another part of the challenge. Good point about the storage type, that will definitely be useful. As far as putting the defs of begin(), end() and the like in the header I disagree with that sentiment. I do not like mixing and matching where functions are defined. They will either be all in the header, or all in the source file; not both.
      $endgroup$
      – Developer Paul
      yesterday










    • $begingroup$
      @DeveloperPaul: It should not matter were the definitions are. All good development tools will automatically jump to definition when asked. I use vi and it still jumps to the function definition when I ask without me knowing where the file is.
      $endgroup$
      – Martin York
      yesterday










    • $begingroup$
      It means exactly what it sounds like, no? Sure but why. This is not a property of an address. Consecutive addresses have no relationship. So this should not be in the address class. You could put it in a helper class that allows you to scan addresses but it should not be part of the address class.
      $endgroup$
      – Martin York
      yesterday










    • $begingroup$
      I see your point now. Seems like I've included quite a bit in the address class that doesn't need to be there.
      $endgroup$
      – Developer Paul
      yesterday













    10












    10








    10





    $begingroup$

    You storing the value in std::array<value_type, 4> which is fine. But if you change your mind on the storage type you have to change this in like 10 places. To make this easier it is a good idea to abstract the storage type and then use this storage type in all places.



     using Storage = std::array<value_type, 4>;
    using iterator = Storage::iterator;
    using const_iterator = Storage::const_iterator;
    using reverse_iterator = Storage::reverse_iterator;
    using const_reverse_iterator = Storage::const_reverse_iterator;
    using size_type = Storage::size_type;


    Now if you change the underlying storage type you only have to change it in one place.




    What does it mean to increment/decrement an ip address?



     /**
    * @brief Prefix increment operator.
    */
    void operator++();


    What scenario does this make sense?




    If there is a test for equality:



    bool operator==(const ip::address &first, const ip::address &second);


    Then I would expect a test for inequality.




    If there is an output operator:



    std::ostream& operator<<(std::ostream& output, const ip::address &address);


    Then I would expect an input operator.




    The standard exceptions (except std::exception itself) already implement what(). You should inherit from one of these rather than std::exception (probably std::runtime_error.



    class invalid_format_exception : public std::exception

    std::string invalid_format_;
    public:
    invalid_format_exception(const std::string &invalid_format);
    char const* what() const override;
    ;


    This becomes:



    struct invalid_format_exception: std::runtime_error

    using std::runtime_error::runtime_error; // Pull runtime_error constructor into this class.
    ;



    Are you sure that the IP address is always stored in big endian form?



    data_[0] = value >> 24 & 0xFF;
    data_[1] = value >> 16 & 0xFF;
    data_[2] = value >> 8 & 0xFF;
    data_[3] = value & 0xFF;


    I would double check and also add a big comment that that is what you expect.




    The increment operator looks complicated.

    I think it can really be simplified by using some existing functions you have identified.



    void ip::address::operator++()

    uint32_t value = (*this); // convert to 32 bit number
    ++value; // Add 1
    (*this) = address(value); // convert back to address and copy/move




    Functions that simply forward calls just put them in the class and forget about them. There is nothing to maintain and it need not take up multiple lines in the source file:



    ip::address::iterator ip::address::begin()

    return data_.begin();


    ip::address::const_iterator ip::address::end() const

    return data_.end();


    // I would just do the following the header:


    iterator begin() return data_.begin();
    iterator end() return data_.end();
    const_iterator begin() const return data_.begin();
    const_iterator end() const return data_.end();


    You are of course missing a few:



     const_iterator cbegin() const return data_.cbegin();
    reverse_iterator rbegin() return data_.rbegin();

    // You can add the end() versions.





    share|improve this answer









    $endgroup$



    You storing the value in std::array<value_type, 4> which is fine. But if you change your mind on the storage type you have to change this in like 10 places. To make this easier it is a good idea to abstract the storage type and then use this storage type in all places.



     using Storage = std::array<value_type, 4>;
    using iterator = Storage::iterator;
    using const_iterator = Storage::const_iterator;
    using reverse_iterator = Storage::reverse_iterator;
    using const_reverse_iterator = Storage::const_reverse_iterator;
    using size_type = Storage::size_type;


    Now if you change the underlying storage type you only have to change it in one place.




    What does it mean to increment/decrement an ip address?



     /**
    * @brief Prefix increment operator.
    */
    void operator++();


    What scenario does this make sense?




    If there is a test for equality:



    bool operator==(const ip::address &first, const ip::address &second);


    Then I would expect a test for inequality.




    If there is an output operator:



    std::ostream& operator<<(std::ostream& output, const ip::address &address);


    Then I would expect an input operator.




    The standard exceptions (except std::exception itself) already implement what(). You should inherit from one of these rather than std::exception (probably std::runtime_error.



    class invalid_format_exception : public std::exception

    std::string invalid_format_;
    public:
    invalid_format_exception(const std::string &invalid_format);
    char const* what() const override;
    ;


    This becomes:



    struct invalid_format_exception: std::runtime_error

    using std::runtime_error::runtime_error; // Pull runtime_error constructor into this class.
    ;



    Are you sure that the IP address is always stored in big endian form?



    data_[0] = value >> 24 & 0xFF;
    data_[1] = value >> 16 & 0xFF;
    data_[2] = value >> 8 & 0xFF;
    data_[3] = value & 0xFF;


    I would double check and also add a big comment that that is what you expect.




    The increment operator looks complicated.

    I think it can really be simplified by using some existing functions you have identified.



    void ip::address::operator++()

    uint32_t value = (*this); // convert to 32 bit number
    ++value; // Add 1
    (*this) = address(value); // convert back to address and copy/move




    Functions that simply forward calls just put them in the class and forget about them. There is nothing to maintain and it need not take up multiple lines in the source file:



    ip::address::iterator ip::address::begin()

    return data_.begin();


    ip::address::const_iterator ip::address::end() const

    return data_.end();


    // I would just do the following the header:


    iterator begin() return data_.begin();
    iterator end() return data_.end();
    const_iterator begin() const return data_.begin();
    const_iterator end() const return data_.end();


    You are of course missing a few:



     const_iterator cbegin() const return data_.cbegin();
    reverse_iterator rbegin() return data_.rbegin();

    // You can add the end() versions.






    share|improve this answer












    share|improve this answer



    share|improve this answer










    answered yesterday









    Martin YorkMartin York

    74.4k488273




    74.4k488273











    • $begingroup$
      "What does it mean to increment/decrement an ip address?" It means exactly what it sounds like, no? The main purpose was to be able to enumerate a number of addresses in a given range. I didn't mention this in the OP, but it was another part of the challenge. Good point about the storage type, that will definitely be useful. As far as putting the defs of begin(), end() and the like in the header I disagree with that sentiment. I do not like mixing and matching where functions are defined. They will either be all in the header, or all in the source file; not both.
      $endgroup$
      – Developer Paul
      yesterday










    • $begingroup$
      @DeveloperPaul: It should not matter were the definitions are. All good development tools will automatically jump to definition when asked. I use vi and it still jumps to the function definition when I ask without me knowing where the file is.
      $endgroup$
      – Martin York
      yesterday










    • $begingroup$
      It means exactly what it sounds like, no? Sure but why. This is not a property of an address. Consecutive addresses have no relationship. So this should not be in the address class. You could put it in a helper class that allows you to scan addresses but it should not be part of the address class.
      $endgroup$
      – Martin York
      yesterday










    • $begingroup$
      I see your point now. Seems like I've included quite a bit in the address class that doesn't need to be there.
      $endgroup$
      – Developer Paul
      yesterday
















    • $begingroup$
      "What does it mean to increment/decrement an ip address?" It means exactly what it sounds like, no? The main purpose was to be able to enumerate a number of addresses in a given range. I didn't mention this in the OP, but it was another part of the challenge. Good point about the storage type, that will definitely be useful. As far as putting the defs of begin(), end() and the like in the header I disagree with that sentiment. I do not like mixing and matching where functions are defined. They will either be all in the header, or all in the source file; not both.
      $endgroup$
      – Developer Paul
      yesterday










    • $begingroup$
      @DeveloperPaul: It should not matter were the definitions are. All good development tools will automatically jump to definition when asked. I use vi and it still jumps to the function definition when I ask without me knowing where the file is.
      $endgroup$
      – Martin York
      yesterday










    • $begingroup$
      It means exactly what it sounds like, no? Sure but why. This is not a property of an address. Consecutive addresses have no relationship. So this should not be in the address class. You could put it in a helper class that allows you to scan addresses but it should not be part of the address class.
      $endgroup$
      – Martin York
      yesterday










    • $begingroup$
      I see your point now. Seems like I've included quite a bit in the address class that doesn't need to be there.
      $endgroup$
      – Developer Paul
      yesterday















    $begingroup$
    "What does it mean to increment/decrement an ip address?" It means exactly what it sounds like, no? The main purpose was to be able to enumerate a number of addresses in a given range. I didn't mention this in the OP, but it was another part of the challenge. Good point about the storage type, that will definitely be useful. As far as putting the defs of begin(), end() and the like in the header I disagree with that sentiment. I do not like mixing and matching where functions are defined. They will either be all in the header, or all in the source file; not both.
    $endgroup$
    – Developer Paul
    yesterday




    $begingroup$
    "What does it mean to increment/decrement an ip address?" It means exactly what it sounds like, no? The main purpose was to be able to enumerate a number of addresses in a given range. I didn't mention this in the OP, but it was another part of the challenge. Good point about the storage type, that will definitely be useful. As far as putting the defs of begin(), end() and the like in the header I disagree with that sentiment. I do not like mixing and matching where functions are defined. They will either be all in the header, or all in the source file; not both.
    $endgroup$
    – Developer Paul
    yesterday












    $begingroup$
    @DeveloperPaul: It should not matter were the definitions are. All good development tools will automatically jump to definition when asked. I use vi and it still jumps to the function definition when I ask without me knowing where the file is.
    $endgroup$
    – Martin York
    yesterday




    $begingroup$
    @DeveloperPaul: It should not matter were the definitions are. All good development tools will automatically jump to definition when asked. I use vi and it still jumps to the function definition when I ask without me knowing where the file is.
    $endgroup$
    – Martin York
    yesterday












    $begingroup$
    It means exactly what it sounds like, no? Sure but why. This is not a property of an address. Consecutive addresses have no relationship. So this should not be in the address class. You could put it in a helper class that allows you to scan addresses but it should not be part of the address class.
    $endgroup$
    – Martin York
    yesterday




    $begingroup$
    It means exactly what it sounds like, no? Sure but why. This is not a property of an address. Consecutive addresses have no relationship. So this should not be in the address class. You could put it in a helper class that allows you to scan addresses but it should not be part of the address class.
    $endgroup$
    – Martin York
    yesterday












    $begingroup$
    I see your point now. Seems like I've included quite a bit in the address class that doesn't need to be there.
    $endgroup$
    – Developer Paul
    yesterday




    $begingroup$
    I see your point now. Seems like I've included quite a bit in the address class that doesn't need to be there.
    $endgroup$
    – Developer Paul
    yesterday











    6












    $begingroup$

    You're passing fundamental types by const reference. These are better off just being passed by value. So you'd get things like



    explicit address(uint32_t value);
    reference operator[](int index) noexcept(false);


    Your prefix increment and decrement operators should return a reference to the incremented value.



    address &operator++() /* ... */ return *this; 
    address &operator--() /* ... */ return *this;


    This will allow expressions like addr = ++other_addr;. (Note that, since you're in the address class, you can just name the class, you don't need to specify scope with ::ip::address).



    Your postfix increment and decrement operators have a bug, because they return a reference to a local variable. The return types should be a value.



    address operator++(int);
    address operator--(int);


    For readability and clarity, expressions mixing shifts and bit masking should use parentheses:



    data_[0] = (value >> 24) & 0xFF;





    share|improve this answer











    $endgroup$

















      6












      $begingroup$

      You're passing fundamental types by const reference. These are better off just being passed by value. So you'd get things like



      explicit address(uint32_t value);
      reference operator[](int index) noexcept(false);


      Your prefix increment and decrement operators should return a reference to the incremented value.



      address &operator++() /* ... */ return *this; 
      address &operator--() /* ... */ return *this;


      This will allow expressions like addr = ++other_addr;. (Note that, since you're in the address class, you can just name the class, you don't need to specify scope with ::ip::address).



      Your postfix increment and decrement operators have a bug, because they return a reference to a local variable. The return types should be a value.



      address operator++(int);
      address operator--(int);


      For readability and clarity, expressions mixing shifts and bit masking should use parentheses:



      data_[0] = (value >> 24) & 0xFF;





      share|improve this answer











      $endgroup$















        6












        6








        6





        $begingroup$

        You're passing fundamental types by const reference. These are better off just being passed by value. So you'd get things like



        explicit address(uint32_t value);
        reference operator[](int index) noexcept(false);


        Your prefix increment and decrement operators should return a reference to the incremented value.



        address &operator++() /* ... */ return *this; 
        address &operator--() /* ... */ return *this;


        This will allow expressions like addr = ++other_addr;. (Note that, since you're in the address class, you can just name the class, you don't need to specify scope with ::ip::address).



        Your postfix increment and decrement operators have a bug, because they return a reference to a local variable. The return types should be a value.



        address operator++(int);
        address operator--(int);


        For readability and clarity, expressions mixing shifts and bit masking should use parentheses:



        data_[0] = (value >> 24) & 0xFF;





        share|improve this answer











        $endgroup$



        You're passing fundamental types by const reference. These are better off just being passed by value. So you'd get things like



        explicit address(uint32_t value);
        reference operator[](int index) noexcept(false);


        Your prefix increment and decrement operators should return a reference to the incremented value.



        address &operator++() /* ... */ return *this; 
        address &operator--() /* ... */ return *this;


        This will allow expressions like addr = ++other_addr;. (Note that, since you're in the address class, you can just name the class, you don't need to specify scope with ::ip::address).



        Your postfix increment and decrement operators have a bug, because they return a reference to a local variable. The return types should be a value.



        address operator++(int);
        address operator--(int);


        For readability and clarity, expressions mixing shifts and bit masking should use parentheses:



        data_[0] = (value >> 24) & 0xFF;






        share|improve this answer














        share|improve this answer



        share|improve this answer








        edited yesterday

























        answered yesterday









        1201ProgramAlarm1201ProgramAlarm

        3,6732925




        3,6732925




















            Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.









            draft saved

            draft discarded


















            Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.












            Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.











            Developer Paul is a new contributor. Be nice, and check out our Code of Conduct.














            Thanks for contributing an answer to Code Review Stack Exchange!


            • Please be sure to answer the question. Provide details and share your research!

            But avoid


            • Asking for help, clarification, or responding to other answers.

            • Making statements based on opinion; back them up with references or personal experience.

            Use MathJax to format equations. MathJax reference.


            To learn more, see our tips on writing great answers.




            draft saved


            draft discarded














            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216894%2fmodeling-an-ipv4-address%23new-answer', 'question_page');

            );

            Post as a guest















            Required, but never shown





















































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown

































            Required, but never shown














            Required, but never shown












            Required, but never shown







            Required, but never shown







            Popular posts from this blog

            Can not update quote_id field of “quote_item” table magento 2Magento 2.1 - We can't remove the item. (Shopping Cart doesnt allow us to remove items before becomes empty)Add value for custom quote item attribute using REST apiREST API endpoint v1/carts/cartId/items always returns error messageCorrect way to save entries to databaseHow to remove all associated quote objects of a customer completelyMagento 2 - Save value from custom input field to quote_itemGet quote_item data using quote id and product id filter in Magento 2How to set additional data to quote_item table from controller in Magento 2?What is the purpose of additional_data column in quote_item table in magento2Set Custom Price to Quote item magento2 from controller

            How to solve knockout JS error in Magento 2 Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern) Announcing the arrival of Valued Associate #679: Cesar Manara Unicorn Meta Zoo #1: Why another podcast?(Magento2) knockout.js:3012 Uncaught ReferenceError: Unable to process bindingUnable to process binding Knockout.js magento 2Cannot read property `scopeLabel` of undefined on Product Detail PageCan't get Customer Data on frontend in Magento 2Magento2 Order Summary - unable to process bindingKO templates are not loading in Magento 2.1 applicationgetting knockout js error magento 2Product grid not load -— Unable to process binding Knockout.js magento 2Product form not loaded in magento2Uncaught ReferenceError: Unable to process binding “if: function()return (isShowLegend()) ” magento 2

            Nissan Patrol Зміст Перше покоління — 4W60 (1951-1960) | Друге покоління — 60 series (1960-1980) | Третє покоління (1980–2002) | Четверте покоління — Y60 (1987–1998) | П'яте покоління — Y61 (1997–2013) | Шосте покоління — Y62 (2010- ) | Посилання | Зноски | Навігаційне менюОфіційний український сайтТест-драйв Nissan Patrol 2010 7-го поколінняNissan PatrolКак мы тестировали Nissan Patrol 2016рвиправивши або дописавши її