7
\$\begingroup\$

My programming foundations were formed in Java, but I've decided to only use C++ from now on for my projects as a personal learning experience. Static utility classes are common in Java; but I don't think they work the same as in C++. I'd like the following:

  • Code review of the functions to determine potential function-specific improvements
  • Analysis of the general architecture/structure to determine if static methods are the way to go. If they are not, what would one recommend to achieve the desired result (which is to have a globally accessible set of utility methods)?

Here's the code:

util.h

#ifndef STDAFX_INCLUDE
#define STDAFX_INCLUDE

#include "stdafx.h"

#endif

#include <sstream>
#include <algorithm> 
#include <functional> 
#include <cctype>
#include <locale>
#include <random>

// Utility class to manage misc. functions

class Util {
public:
    static std::string serialize_http_args(std::map<std::string, std::string>);
    static std::string serialize_http_headers(std::map<std::string, std::string>);
    static int randrange(int min, int max);
    static void trim(std::string&);
    static void ltrim(std::string&);
    static void rtrim(std::string&);
    static std::string trim_copy(std::string);
    static std::string ltrim_copy(std::string);
    static std::string rtrim_copy(std::string);
    static std::string int2str(int);
    static std::string lowercase(std::string);
    static std::vector<std::string> split(std::string, std::string);
    static std::vector<std::string> split_once(std::string, std::string);
};

util.c

#include "util.h"

// returns a random number in range min through max, inclusive.
int Util::randrange(int min, int max) {
    std::random_device rd; // obtain a random number from hardware
    std::mt19937 eng(rd()); // seed the generator
    std::uniform_int_distribution<> distr(min, max); // define the range
    return distr(eng);
}

// takes a map of arguments and converts it to a string representation
std::string Util::serialize_http_args(std::map<std::string, std::string> _args) {
    if (_args.size() > 0) {
        size_t counter = 1;
        std::string args_str = "";
        for (auto const& arg : _args) {
            args_str += arg.first + "=" + arg.second;
            if (counter < _args.size())
                args_str += "&";
            counter++;
        }
        return args_str;
    }

    return "";
}

// takes a map of headers and converts it to the string representation
std::string Util::serialize_http_headers(std::map<std::string, std::string> _headers) {
    if (_headers.size() > 0) {
        std::string headers_ser = "";
        for (auto const& header : _headers)
            headers_ser += header.first + ": " + header.second + "\r\n";
        return (headers_ser + "\r\n");
    }

    return "";
}

// splits a string infinitely by delimiter until completely tokenized
std::vector<std::string> Util::split(std::string s, std::string delimiter) {
    std::vector<std::string> output;

    std::string::size_type prev_pos = 0, pos = 0;

    while ((pos = s.find(delimiter, pos)) != std::string::npos)
    {
        std::string substring(s.substr(prev_pos, pos - prev_pos));
        output.push_back(substring);
        prev_pos = ++pos;
    }

    output.push_back(s.substr(prev_pos, pos - prev_pos)); // Last word

    return output;
}

// splits a string by the first instance of delimiter
std::vector<std::string> Util::split_once(std::string s, std::string delimiter)
{
    std::vector<std::string> output;
    std::string::size_type pos = 0;

    if ((pos = s.find(delimiter, pos)) != std::string::npos) {
        output.push_back(std::string(s.substr(0, pos)));
        output.push_back(std::string(s.substr(pos + 1)));
    }

    return output;
}

// trim from start (in place)
void Util::ltrim(std::string & s) {
    s.erase(s.begin(), std::find_if(s.begin(), s.end(),
        std::not1(std::ptr_fun<int, int>(std::isspace))));
}

// trim from end (in place)
void Util::rtrim(std::string & s) {
    s.erase(std::find_if(s.rbegin(), s.rend(),
        std::not1(std::ptr_fun<int, int>(std::isspace))).base(), s.end());
}

// trim from both ends (in place)
void Util::trim(std::string & s) {
    ltrim(s);
    rtrim(s);
}

// trim from start (copying)
std::string Util::ltrim_copy(std::string _s) {
    std::string s = _s;
    ltrim(s);
    return s;
}

// trim from end (copying)
std::string Util::rtrim_copy(std::string _s) {
    std::string s = _s;
    rtrim(s);
    return s;
}

// trim from both ends (copying)
std::string Util::trim_copy(std::string _s) {
    std::string s = _s;
    trim(s);
    return s;
}

// convert string to lowercase version of same string
std::string Util::lowercase(std::string _s) {
    std::string s = _s;
    std::transform(s.begin(), s.end(), s.begin(), ::tolower);
    return s;
}

//convert type integer to string
std::string Util::int2str(int i) {
    std::string out;
    std::stringstream ss;
    ss << i;
    return ss.str();
}

```
\$\endgroup\$
7
\$\begingroup\$

Your functions themselves are pretty broad, so I'm not going to review each one. However, I can tell you that what you're probably looking for is a namespace. A namespace is just a way to group together similar functions/classes/static objects. They're a little bit like Java's package in that they can be used to organize your code into related groups.

A class with only static functions can be converted to a namespace very easily:

#ifndef STDAFX_INCLUDE
#define STDAFX_INCLUDE

#include "stdafx.h"

#endif

#include <sstream>
#include <algorithm> 
#include <functional> 
#include <cctype>
#include <locale>
#include <random>

// Namespace containing utility functions

namespace Util {
    std::string serialize_http_args(std::map<std::string, std::string>);
    std::string serialize_http_headers(std::map<std::string, std::string>);
    int randrange(int min, int max);
    void trim(std::string&);
    void ltrim(std::string&);
    void rtrim(std::string&);
    std::string trim_copy(std::string);
    std::string ltrim_copy(std::string);
    std::string rtrim_copy(std::string);
    std::string int2str(int);
    std::string lowercase(std::string);
    std::vector<std::string> split(std::string, std::string);
    std::vector<std::string> split_once(std::string, std::string);
}

Note how class was changed to namespace, public: was removed (all items in a namespace are public), static was removed (it has different meaning outside of a class) and there is no semicolon after the final closing brace.

Each function is referenced the same as it was before, e.g. int random = Util::randrange(0, 100).

Your include files also look strange compared to what you're actually using - based on the given function definitions, you should only need <string>, <map> and <vector> in this header. Best practice with include files is to only include the headers that you need for the current file. A user of this utility namespace doesn't care if your randrange function uses the <random> library for its implementation - that is a detail that should only appear in the source file.

As a side note, since it looks like you're developing on Windows, best practice is to put

#include "stdafx.h"

as the first line in every source file, and leave it out of your header files completely. See this answer on StackOverflow for more details.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you. I did not know that about the includes, I thought best practice was to put all includes in the header file. But that makes sense. With regards to stdafx. I've seen several different answers about when/how/bestpractices for stdafx. Could you update your answer to explain how it would apply in this case? \$\endgroup\$ – leaustinwile May 8 at 22:28
  • \$\begingroup\$ @leaustinwile I've updated my post with a link to a relevent StackOverflow answer, and added some more information about namespaces at the beginning. \$\endgroup\$ – cariehl May 8 at 22:36
  • \$\begingroup\$ For sake of brevity and clarity, would you recommend against using stdafx? This is intended to be a cross-platform project upon completion. Operable on at the minimum, linux and windows. \$\endgroup\$ – leaustinwile May 8 at 22:44
  • \$\begingroup\$ @leaustinwile That depends on your project. stdafx represents the concept of precompiled headers, which can be used to drastically improve compilation time for a large project. The idea is that you put all commonly-used headers into stdafx.h, which is then precompiled and cached, reducing the cost of subsequent compilations. The trade-off is that you lose the ability to see, at a glance, the dependencies for each file in your project (since common library includes are extracted to stdafx.h). If your compilation time isn't a big issue, then precompiled headers probably aren't necessary. \$\endgroup\$ – cariehl May 9 at 15:03
  • 1
    \$\begingroup\$ Makes perfect sense to me! Thanks a lot. \$\endgroup\$ – leaustinwile May 9 at 21:30
5
\$\begingroup\$
  • In randrange, you initialize the random device, the engine, and the distribution on every call. So this is not suitable for every situation, i.e., when performance matters.

  • In serialize_http_args and in general, never query the size of a container when you want to determine whether it is empty or not. For that, you should use empty(). This is idiomatic and saves you from costly surprises like with container implementations where empty() is a constant time method while size() needs linear time.

  • At least for both serialize_http_args and serialize_http_headers, you might consider first checking whether _args.empty() (or _headers.empty()), and then returing an empty string, i.e.,:

    std::string serialize_http_args(std::map<std::string, std::string> _args) 
    {
        if (_args.empty())
            return "";
    
        size_t counter = 1;
        std::string args_str = "";
        for (auto const& arg : _args) 
        {
            args_str += arg.first + "=" + arg.second;
            if (counter < _args.size())
                args_str += "&";
            counter++;
        }
    
        return args_str;
    }
    

    I find this cleaner and easier to read, especially if the functions were any longer.

  • You don't need your own int2str anymore, you can just use std::to_string.

\$\endgroup\$
  • \$\begingroup\$ Good points. I'm still learning all the methods and functions I have available so I've just been hacking it together with my Java/Python know-how. Could you clarify on constant vs. linear time methods/functions and what they are? \$\endgroup\$ – leaustinwile May 9 at 17:39
  • \$\begingroup\$ @leaustinwile Sure. For example the size method of std::list could have been a linear-time method pre-C++11. IIRC, this had something to do with splicing then. \$\endgroup\$ – Juho May 9 at 17:49
2
\$\begingroup\$

The function for serializing URL parameters is horribly broken since it leaves out URL escaping. Don't invent these functions yourself. Since you're from a Java background, have a look at Apache Commons and Spring Framework, they already implemented this function and took care about all edge cases.

There's probably a C++ library that provides URL encoding. You're definitely not the first one to need this function in a C++ program.

\$\endgroup\$

Your Answer

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

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