I have a function that adds to a std::wstring some filler characters. The user gives a single filler character. They can also optionally given an initial filler sequence (std::wstring) and an optional final filler sequence (std::wstring) that start and end the padding. These will only be added however if they fit inside the padded length specified by the user.

static std::wstring fillWString(
    const std::wstring &stringToFill,
    size_t fillLength,
    const wchar_t fillChar = L' ',
    const std::wstring &initialFill = L"",
    const std::wstring &endFill = L"") {

  const size_t originalStringLength = stringToFill.size();
  if (fillLength <= originalStringLength) {
    return stringToFill;
  }

  std::wstring result(stringToFill);
  result.resize(fillLength, fillChar);

  if (originalStringLength + initialFill.size() <= fillLength) {
    result.replace(originalStringLength, initialFill.size(), initialFill, 0u);
    if (originalStringLength + initialFill.size() + endFill.size() <= fillLength) {
      result.replace(fillLength - endFill.size(), endFill.size(), endFill, 0u);
    }
  }
  return result;
}

Here is some output:

Analyzed folder: .... \Path\To\Some\Folder\
Time step: .......... 0.007045000
Out Flow: ........... 1.2460679423999952
In Flow: ............ -1.2461052960000008

where each line was generated using

fillWString(L"string", 22, '.', L" ", L" ") << value;

Specifically, I was wondering what sort of tips you guys might give to improve this. Also, I was just wondering if copy-elision is guaranteed to occur here if the string is indeed padded?

  • Named-value return elision isn't required by the standard, but it would be a poor implementation that didn't bother, at least when optimizing. – Toby Speight Oct 23 at 11:35
  • Caution: in the realm of wstring, there is a possibility of fullwidth characters; a uniform-width assumption may not hold. – 200_success Oct 23 at 18:01
up vote 5 down vote accepted

The point where the parameters stop and the function-body begins isn't easy to see.
Either indent more, or better consider changing to what c2-wiki calls form 6.

You are allocating way too much when passing arguments.
Using std::wstring_view instead of std::wstring const& lets you avoid that overhead and reduces indirection, which is also a win.

Also, you potentially re-allocate after copying the source when widening to the target-length.

static std::wstring fillWString(
    std::wstring_view source,
    size_t target,
    wchar_t fillChar = L' ',
    std::wstring_view startFill = {},
    std::wstring_view endFill = {}
) {
    if (target <= source.size())
        return source;

    std::wstring result;
    result.reserve(target);
    result += source;
    target -= source.size();

    if (startFill.size() > target)
        return result;
    result += startFill;
    target -= source.size();

    if (endFill.size() > target) {
        result.append(target, fillChar);
        return result;
    }
    result.append(target - endFill.size(), fillChar);
    result += endFill;
    return result;
}
  • Thanks for the nice answer, including the use of std::wstring_view and changing the title of the post :D – Slugger Oct 23 at 12:24

Since we always make a copy of stringToFill, we could pass it by value, which will reduce the amount of copying when we use an rvalue as argument:

static std::wstring fillWString(
    std::wstring stringToFill, ...
                                )
{
  ...
  std::wstring& result(stringToFill);
  ...
  return stringToFill;
}

Users can use std::move() if they pass a lvalue that's not required subsequently:

s = fillWString(std::move(s), 15);

Minor points:

  • Whitespace is unusual - most C++ developers expect to see & nestled against the type, rather than the value.
  • Consistently misspelt std::size_t.
  • Default arguments of L"" could be written as {} if you find that more readable.
  • It's not clear from the description that if there's room for initialFill, it will be used even if there's insufficient room for endFill. This may surprise users who want to use paired delimiters (e.g. [ and ]).
  • It's not clear from the description that if initialFill and endFill exactly fit, there will be no fillChar inserted. That's easy enough to allow for if you know about it (just append the char to initialFill or prepend it to endFill), but users need to be informed!
  • Why limit this to std::wstring? A single template argument could make this general to all string types. A disadvantage to this as that template function arguments need to match exactly, preventing automatic conversions; workarounds for this include callers explicitly specifying the template instantiation, and/or providing a small family of forwarding functions.
  • Perhaps rearrange the logic to write just once to each character, rather than filling with fillChar then overwriting some of it.

Modified code

Here's how it looks with my suggestions applied; I've also shortened some of the variable names, which were over-long to my taste:

#include <string>

// Pad the supplied `str` to `width` characters long, using `fill`.

// If there's room for `prefix`, then use that to begin the padding;
// if there's also room for `suffix` then use that to end the padding.

// If `prefix` or `prefix+suffix` pad exactly, then no `fill` characters
// will be used - if at least one is required, add it to the end of
// `prefix`.

template<typename String>
String fillString(String str,
                  const typename String::size_type width,
                  const typename String::value_type fill = ' ',
                  const String& prefix = {},
                  const String& suffix = {})
{
    const auto originalLength = str.size();
    if (originalLength >= width) {
        return str;
    }

    str.reserve(width);

    if (originalLength + prefix.size() <= width) {
        // enough space for prefix
        str += prefix;

        if (str.size() + suffix.size() <= width) {
            // enough space for suffix as well
            str.resize(width - suffix.size(), fill);
            str += suffix;
        } else {
            str.resize(width, fill);
        }
    } else {
        str.resize(width, fill);
    }


    return str;
}

// forwarding function, for convenience
template<typename CharT, typename... Args>
std::basic_string<CharT> fillString(std::basic_string<CharT> str, Args... rest)
{
    return fillString<std::basic_string<CharT>>(std::move(str),
                                                std::forward<Args>(rest)...);
}

And a demo program (note that std::literals::string_literals is a namespace that's intended to be imported wholesale, unlike std):

#include <iostream>

int main()
{
    using namespace std::literals::string_literals;
    static const auto s = L"FooBarBazQuux"s;

    for (auto i = 5u;  i < s.size();  ++i) {
        std::wcout << fillString(s.substr(0, i), 10, L'.', L" [.", L"]") << '\n';
    }
}
  • Thanks for a nice answer! – Slugger Oct 23 at 12:19
  • You know templating interferes with implicit-conversion? Still, one could template on the character-type, and let the caller disambiguate as needed. Or use a resolving forwarder / multiple forwarders to the single common implementation. – Deduplicator Oct 23 at 12:29
  • I've added a forwarding function to convert arguments based on the string type. I resisted the temptation to also template it on the traits_type and allocator_type... – Toby Speight Oct 23 at 12:56

If I understand your starting postulate, the user give:

  1. A wstring as a starting point
  2. A maximal length if padding occurs
  3. A char to repeatedly add after the string for padding
  4. A prefix and a suffix that have to enclose the string only if they fit (both or just the first or none*) inside the maximal length.

    (*) In my opinion, a policy "both or none" is more consistent.

Review

static std::wstring fillWString

I don't see advantage to declare your function as static.

  • If it's a static method from a class, since you "extracted" it from the class for review, just remove the keyword.
  • If it's to hide the function from other translation units, just replace it by an anonymous namespace.
  • Maybe you added it for another reason, if so, tell me.

Next, for the function signature

const std::wstring &stringToFill,
size_t fillLength,
const wchar_t fillChar = L' ',
const std::wstring &initialFill = L"",
const std::wstring &endFill = L""

As said by @Deduplicator, you should use wstring_view instead of const wstring& this will reduce number of allocations and help you to reduce overhead. Your const-correctness is good, just apply it to the size_t to stay consistent and allow optimizations.

At this point, we can already see another problem: your naming convention. You wrote 26 times the word "fill" for only 16 lines of code (i dont count curly braces nor the empty lines). That pollute the reading since we already know from the function name that's about filling a string.

const size_t originalStringLength = stringToFill.size();
if (fillLength <= originalStringLength) {
  return stringToFill;
}
std::wstring result(stringToFill);
result.resize(fillLength, fillChar);

You return the original string if at least as long as the given length.

std::wstring result(stringToFill);
result.resize(fillLength, fillChar);

if (originalStringLength + initialFill.size() <= fillLength) {
  result.replace(originalStringLength, initialFill.size(), initialFill, 0u);
  if (originalStringLength + initialFill.size() + endFill.size() <= fillLength) {
    result.replace(fillLength - endFill.size(), endFill.size(), endFill, 0u);
  }
}

You allocate the final size, but after put the filling char in each byte of padding, if needed, you change again bytes where prefix and suffix occurs. Try to avoid overwriting when you can. Furthermore, you did many times same computation, caching is a option.

Here you can see also how too long names make the reading more difficult.

Proposal (using policy "both or none")

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

std::wstring fillString(
    const std::wstring_view source,
    const size_t length,
    const wchar_t filler = L' ',
    const std::wstring_view prefix = {},
    const std::wstring_view suffix = {}
) {
    if (length <= source.size()) {
        return std::wstring{source};
    } 
    std::wstring result;
    result.reserve(length);
    result = source;

    if (prefix.size() + result.size() + suffix.size() <= length ) {
      result.append(prefix);
      result.append(length - result.size() - suffix.size(), filler);
      result.append(suffix);
    }
    else {
      result.append(length - result.size(), filler);
    }
    return result;
}

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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