Modeling an IP Address





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







3












$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 | data_[1] << 16 | data_[2] << 8 | data_[3];
return value;
}

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$



















    3












    $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 | data_[1] << 16 | data_[2] << 8 | data_[3];
    return value;
    }

    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$















      3












      3








      3





      $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 | data_[1] << 16 | data_[2] << 8 | data_[3];
      return value;
      }

      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 | data_[1] << 16 | data_[2] << 8 | data_[3];
      return value;
      }

      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






      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 3 hours ago









      Developer PaulDeveloper Paul

      1162




      1162




      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.






















          3 Answers
          3






          active

          oldest

          votes


















          1












          $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$





















            1












            $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$





















              1












              $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$














                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-ip-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









                1












                $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$


















                  1












                  $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$
















                    1












                    1








                    1





                    $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 1 hour ago

























                    answered 2 hours ago









                    1201ProgramAlarm1201ProgramAlarm

                    3,6032925




                    3,6032925

























                        1












                        $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$


















                          1












                          $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$
















                            1












                            1








                            1





                            $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 1 hour ago









                            Martin YorkMartin York

                            74.2k488272




                            74.2k488272























                                1












                                $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$

                                  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












                                    1








                                    1





                                    $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 1 hour ago









                                    QuuxplusoneQuuxplusone

                                    13k12063




                                    13k12063






















                                        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-ip-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

                                        Liste der Baudenkmale in Friedland (Mecklenburg)

                                        Single-Malt-Whisky

                                        Czorneboh