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();
}
```
3 Answers 3
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.
-
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– leaustinwile2019年05月08日 22:28:07 +00:00Commented May 8, 2019 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– cariehl2019年05月08日 22:36:35 +00:00Commented May 8, 2019 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– leaustinwile2019年05月08日 22:44:19 +00:00Commented May 8, 2019 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 intostdafx.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 tostdafx.h
). If your compilation time isn't a big issue, then precompiled headers probably aren't necessary. \$\endgroup\$cariehl– cariehl2019年05月09日 15:03:15 +00:00Commented May 9, 2019 at 15:03 -
\$\begingroup\$ No. This will probably be a small to medium sized project upon completion. I don't think that compilation time will be a problem. I'm going to remove it for clarity sake. I just thought I'd use it in the beginning because I wanted to try out new C++ features I've never used! :) \$\endgroup\$leaustinwile– leaustinwile2019年05月09日 17:41:16 +00:00Commented May 9, 2019 at 17:41
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 useempty()
. This is idiomatic and saves you from costly surprises like with container implementations whereempty()
is a constant time method whilesize()
needs linear time.At least for both
serialize_http_args
andserialize_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 usestd::to_string
.
-
\$\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– leaustinwile2019年05月09日 17:39:40 +00:00Commented May 9, 2019 at 17:39
-
\$\begingroup\$ @leaustinwile Sure. For example the
size
method ofstd::list
could have been a linear-time method pre-C++11. IIRC, this had something to do with splicing then. \$\endgroup\$Juho– Juho2019年05月09日 17:49:44 +00:00Commented May 9, 2019 at 17:49
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.