Skip to main content
Code Review

Return to Revisions

6 of 6
Commonmark migration

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';
 }
}
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325
default

AltStyle によって変換されたページ (->オリジナル) /