Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Modified code

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

#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:

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:

A better forwarding function
Source Link
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325
#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 CharT>String>
std::basic_string<CharT>String fillString(std::basic_string<CharT>String str,
 const stdtypename String::size_tsize_type width,
 const CharTtypename String::value_type fill = ' ',
 const std::basic_string<CharT>&String& prefix = {},
 const std::basic_string<CharT>&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 functionsfunction, for convenience
std::string fillString(std::string str, const std::size_t width, const char fill = ' ',
 const std::string& prefix = {}, const std::string& suffix = {})
{
 return fillString<char>(std::move(str), width,template<typename fillCharT, prefix,typename... suffix);
}
Args>
std::wstringbasic_string<CharT> fillString(std::wstringbasic_string<CharT> str, const std::size_t width, const wchar_t fill = ' ',
 const std::wstring& prefix = {}, const std::wstring& suffix =Args... {}rest)
{
 return fillString<wchar_t>fillString<std::basic_string<CharT>>(std::move(str), width, fill, prefix, suffix);
}

std::u16string fillString(std::u16string str, const std::size_t width, const char16_t fill = ' ',
 const std::u16string& prefix = {}, const std::u16string& suffix = {})
{
 return fillString<char16_t>(std::move(str), width, fill, prefix, suffix);
}
std::u32string fillString(std::u32string str, const std::size_t width, const char32_t fill = ' ',
  const std::u32string& prefix = {}, const std::u32string& suffix = {})
{
 return fillString<char32_t>(std::moveforward<Args>(strrest), width, fill, prefix, suffix...);
}
#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 CharT>
std::basic_string<CharT> fillString(std::basic_string<CharT> str,
 const std::size_t width,
 const CharT fill = ' ',
 const std::basic_string<CharT>& prefix = {},
 const std::basic_string<CharT>& 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 functions, for convenience
std::string fillString(std::string str, const std::size_t width, const char fill = ' ',
 const std::string& prefix = {}, const std::string& suffix = {})
{
 return fillString<char>(std::move(str), width, fill, prefix, suffix);
}

std::wstring fillString(std::wstring str, const std::size_t width, const wchar_t fill = ' ',
 const std::wstring& prefix = {}, const std::wstring& suffix = {})
{
 return fillString<wchar_t>(std::move(str), width, fill, prefix, suffix);
}

std::u16string fillString(std::u16string str, const std::size_t width, const char16_t fill = ' ',
 const std::u16string& prefix = {}, const std::u16string& suffix = {})
{
 return fillString<char16_t>(std::move(str), width, fill, prefix, suffix);
}
std::u32string fillString(std::u32string str, const std::size_t width, const char32_t fill = ' ',
  const std::u32string& prefix = {}, const std::u32string& suffix = {})
{
 return fillString<char32_t>(std::move(str), width, fill, prefix, suffix);
}
#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)...);
}
Show forwarding non-template functions as suggested by @Deduplicator
Source Link
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325
  • 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.
#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 CharT>
std::basic_string<CharT> fillString(std::basic_string<CharT> str,
 const std::size_t width,
 const CharT fill = ' ',
 const std::basic_string<CharT>& prefix = {},
 const std::basic_string<CharT>& 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 functions, for convenience
std::string fillString(std::string str, const std::size_t width, const char fill = ' ',
 const std::string& prefix = {}, const std::string& suffix = {})
{
 return fillString<char>(std::move(str), width, fill, prefix, suffix);
}
std::wstring fillString(std::wstring str, const std::size_t width, const wchar_t fill = ' ',
 const std::wstring& prefix = {}, const std::wstring& suffix = {})
{
 return fillString<wchar_t>(std::move(str), width, fill, prefix, suffix);
}
std::u16string fillString(std::u16string str, const std::size_t width, const char16_t fill = ' ',
 const std::u16string& prefix = {}, const std::u16string& suffix = {})
{
 return fillString<char16_t>(std::move(str), width, fill, prefix, suffix);
}
std::u32string fillString(std::u32string str, const std::size_t width, const char32_t fill = ' ',
 const std::u32string& prefix = {}, const std::u32string& suffix = {})
{
 return fillString<char32_t>(std::move(str), width, fill, prefix, suffix);
}
#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" [."s", L"]"sL"]") << '\n';
 }
}
  • 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.
  • Perhaps rearrange the logic to write just once to each character, rather than filling with fillChar then overwriting some of it.
#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 CharT>
std::basic_string<CharT> fillString(std::basic_string<CharT> str,
 const std::size_t width,
 const CharT fill = ' ',
 const std::basic_string<CharT>& prefix = {},
 const std::basic_string<CharT>& 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;
}
#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" [."s, L"]"s) << '\n';
 }
}
  • 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.
#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 CharT>
std::basic_string<CharT> fillString(std::basic_string<CharT> str,
 const std::size_t width,
 const CharT fill = ' ',
 const std::basic_string<CharT>& prefix = {},
 const std::basic_string<CharT>& 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 functions, for convenience
std::string fillString(std::string str, const std::size_t width, const char fill = ' ',
 const std::string& prefix = {}, const std::string& suffix = {})
{
 return fillString<char>(std::move(str), width, fill, prefix, suffix);
}
std::wstring fillString(std::wstring str, const std::size_t width, const wchar_t fill = ' ',
 const std::wstring& prefix = {}, const std::wstring& suffix = {})
{
 return fillString<wchar_t>(std::move(str), width, fill, prefix, suffix);
}
std::u16string fillString(std::u16string str, const std::size_t width, const char16_t fill = ' ',
 const std::u16string& prefix = {}, const std::u16string& suffix = {})
{
 return fillString<char16_t>(std::move(str), width, fill, prefix, suffix);
}
std::u32string fillString(std::u32string str, const std::size_t width, const char32_t fill = ' ',
 const std::u32string& prefix = {}, const std::u32string& suffix = {})
{
 return fillString<char32_t>(std::move(str), width, fill, prefix, suffix);
}
#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';
 }
}
External linkage and rename (now it's general)
Source Link
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325
Loading
Improved main() to exercise all code paths
Source Link
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325
Loading
Source Link
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325
Loading
lang-cpp

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