11
\$\begingroup\$

To develop my understanding of C++, I've written an overload function that allows users to use & to insert strings within other strings (denoted by a $ character). So far, it only works with one replacement in a string. For example:

"Hello $" & "World" -> "Hello World"

I want to receive feedback, because I'm sure there's a much better way than using three for loops. And I'd like a better foundation before moving forward with multiple replacements in a single string.

#include <string>
#include <iostream>

std::string operator&(const std::string& str, const char* cstr) {

    if (str.empty()) {
        return std::string(cstr);
    }
    if (str.find("$") == std::string::npos) {
        return str + std::string(cstr);
    }

    int string_length = str.length();
    int cstr_length = 0;

    while (cstr[cstr_length] != '\0') {
        cstr_length++;
    }

    int length = string_length + cstr_length;
    std::string result = "";
    int i;

    for (i = 0; i < str.find("$"); i++) {
        result += str[i];
    }

    i++; // Skips past the '$'

    for (int j = 0; j < cstr_length; j++) {
        result += cstr[j];
    }

    for (; i < string_length; i++) {
        result += str[i];
    }
    
    return result;

}

int main(int argc, char** argv) {

    std::string string = "Hello $, My name is Ben!";
    char* cstr = (char*)"World";

    std::string result = string & cstr;
    std::cout << result << std::endl;

    return 0;

}
\$\endgroup\$
3
  • \$\begingroup\$ Two online resources for you that would be helpful en.cppreference.com and cplusplus.com. \$\endgroup\$ Commented Jun 9, 2022 at 15:58
  • \$\begingroup\$ Is the intended behaviour when dealing with format strings that don’t contain a $ character really that the two strings will just be concatenated? Coming from other languages that behaviour could be very unexpected. I feel it’s more sensible to not replace anything and just return the format string. \$\endgroup\$ Commented Jun 10, 2022 at 16:05
  • \$\begingroup\$ Since there are multiple types of these formatting systems already, they seem to have chosen the '%' character as the insertion point I would change to using that over the '$'/ \$\endgroup\$ Commented Jun 10, 2022 at 18:34

4 Answers 4

Reset to default
12
\$\begingroup\$

  1. Keep your includes ordered. Thus, you won't lose track of them, even if there are more of them.

    Common modern order goes from local to universal, to ensure headers are self-contained. Exceptions are fixed or at least annotated. All groups are sorted and separated by an empty line for emphasis:

    1. precompiled header if applicable, for obvious technical reasons
    2. the corresponding header, to ensure it is self-contained
    3. own headers
    4. external libraries headers
    5. system headers
    6. standard headers

    Any of the last three might be amalgamated.

#include <iostream>
#include <string>
#include <string_view>

  1. Don't bolt your own operators on someone else's type. It just invites confusion. Use a properly named free function instead.

  2. Using C++17 std::string_view leads to a more efficient and convenient interface, and simplifies your implementation.

  3. Avoid special cases. They mean more code (which can be wrong), and slow down the common case.

std::string replace_placeholder(std::string_view format, std::string_view a) {
    auto pos = format.find('$');
    auto rest = pos + 1;
    std::string r;
    r.reserve(format.size() + a.size() - (pos != std::string_view::npos));
    if (pos == std::string_view::npos)
        rest = pos = format.size();
    r.append(format.begin(), format.begin() + pos);
    r += a;
    r.append(format.begin() + rest, format.end());
    return r;
}

  1. Don't name arguments you won't use. Thus, you avoid confusing future readers (like yourself) nor cause spurious warnings.
    In the case of main(), you might just leave the arguments out completely.

  2. Eliminate dead code and dead variables. They will just confuse, and if you want to restore them, there is source control. Let the compiler warn you about them as it can.

  3. Don't muzzle the compiler just because it has the audacity to warn you. Figure out what you did wrong instead, and fix that. Yes, sometimes you have to override it, but you have to be judicious and careful there, using the least dangerous tool to get the job done.
    In the specific case, instead of casting away const, fix the pointer type. const_cast would have been less dangerous than an unrestricted C-style cast, if it had been appropriate.

  4. Embrace auto. Almost Always Auto is a good idea to avoid mismatches and unintended conversions, especially if the exact type doesn't matter to you.

  5. Only use std::endl when you need to flush the stream. Actually, belay that, be explicit and use std::flush.
    Also, when the program ends the standard streams are flushed.
    See "What is the C++ iostream endl fiasco?".

  6. return 0; is implicit for main().

int main() {
    std::string string = "Hello $, My name is Ben!";
    auto cstr = "World";

    auto result = replace_placeholder(string, cstr);
    std::cout << result << "\n";
}
\$\endgroup\$
0
8
\$\begingroup\$

This style of substitution is brittle. Suppose we have a string with two placeholders for substitution:

std::cout << "& will be in &\n" & person & place;

If person contains & (e.g. "Bill & Ted"), then the second substitution will occur in the wrong place.

It's much better to create a proper format object from the format string, and use that object to interpolate the values.

Substitution based on the order of the substitution characters in the format string doesn't work very well when it is translated. The target language's grammar can (and often does) cause the placeholders to appear in an order that doesn't agree with the code. Avoid this by including numbers as part of the substitution indicators, then using those numbers to substitute arguments in their correct positions.

\$\endgroup\$
2
  • \$\begingroup\$ Good point about successive substitutions. Though it really shouldn't be done anyway due to inefficiency. \$\endgroup\$ Commented Jun 9, 2022 at 16:03
  • \$\begingroup\$ Yep - multiple substitutions can be reasonably efficient, but only by breaking it up and deferring the reassembly until all the pieces are present. \$\endgroup\$ Commented Jun 10, 2022 at 9:43
7
\$\begingroup\$

General Observations

You have mixed C and C++ in this program, since it is C++ there is no reason to mix the two. A thing to remember with C++ strings is that you don't have to manipulate the characters, except where you are replacing $ with a string. C++ strings support string concatenation. You can also use Iterators which is the preferred way in C++ or you can use the old pointer mechanism from C.

Algorithm

The steps you need to take are

  1. Find $.
  2. Copy the rest of the string following the $ into a temporary string variable using assignment.
  3. Terminate a copy of the string at $.
  4. Create the return value using concatenation (+).

Let the Library do the Work for You

Even with just the C language this loop is unnecessary:

    while (cstr[cstr_length] != '\0') {
        cstr_length++;
    }

You can use the C library routine strlen():

    cstr_length = strlen(cstr);

However, since you don't need to allocate the space your really don't need the size.

Use C++ Casts Rather Than C Casts

While this line in main() compiles because C++ is backward compatible with C, it is not the preferred way to do a cast in C++:

    char* cstr = (char*)"World";

C++ has static_cast<> and dynamic_cast<> and what you need in this case is a static_cast.

    const char* cstr = static_cast<const char*>("World");

It isn't really clear why you are casting a string to char *, you could just pass in a separate string.

Use size_t for Indexing Into Arrays

In the operator you declare the variables i and cstr_length as integers, instead they should be declared as size_t which is the largest unsigned integer the system can represent. This is true in both C and C++.

\$\endgroup\$
7
  • \$\begingroup\$ Hi, I'm not a C++ expert so I'm not really going to write my own answer. But I was wondering if it was possible to allocate a buffer for the resulting string in advance, because we know the final length (it will be length(str) - 1 + length(cstr) as long as str contains the placeholder, and length(str) + length(cstr) if there is no placeholder, although im not sure why OPs code concatenates the two when there is no placeholder, I'd rather expect it to return just str or even throw). And so avoiding the need for any concatenations. Would this approach have any performance impact? \$\endgroup\$ Commented Jun 9, 2022 at 6:03
  • \$\begingroup\$ Also one more question, isn't the literal constatnt "World" already of type const char*? Is there a need to cast it at all? \$\endgroup\$ Commented Jun 9, 2022 at 6:05
  • 1
    \$\begingroup\$ @slepic string literal in C and C++ is of lvalue type const char[len] where len is length of the literal including null terminator. Notice that modifying the literal is undefined behavior. \$\endgroup\$ Commented Jun 9, 2022 at 9:02
  • \$\begingroup\$ @sleptic The std::string class is a variable length object, there is not reason to pre-allocate the buffer, it will expand as necessary, that is why it is better to use in this case than an array of characters. \$\endgroup\$ Commented Jun 9, 2022 at 15:50
  • 1
    \$\begingroup\$ @slepic You can allocate the string in advance, but you have to count the $s first (it may be in there more than once), and you might want to decide if the replacement string is allowed to contain $s, etc. \$\endgroup\$ Commented Jun 9, 2022 at 19:58
6
\$\begingroup\$

You've already gotten some good reviews, so I'll just cover the things not already mentioned.

Eliminate unused variables

Unused variables are a sign of poor quality code, and you don't want to write poor quality code. In this code, length, argc and argv are unused. Your compiler is smart enough to tell you about this if you ask it nicely.

Be careful with signed and unsigned

The current code compares an int i with a possibly unsigned result of str.find(). It would be better to declare i to also be std::string::size_type.

Use the standard function

This is probably a good learning opportunity, but for production code, I'd recommend using the C++20 <format> library instead:

#include <format>
#include <iostream>

int main(int argc, char *argv[]) {
    std::string message = std::format("Hello {}, My name is Ben!", argv[1]);
    std::cout << message << '\n';
}

See https://godbolt.org/z/TEa6K6axx for a live demonstration.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.