Printing log messages with varying length fit into screen width

Question

I'm writing a simple sanity tool for a simple USB driver.

The customer is directly facing the output so I have to care about how it looks. The log messages that I need to print are varying length, and they have "[ SUCCESS ]" and "[ FAIL ]" trailing part too and the middle is filled with dots.

So what I do is calculate the number of dots that should be in the message. For a example,

#define SCREEN_SIZE_X 80
void print_message(const char* pMsg, bool isPassed)
{
#define SPRINTF_BUFFER_SIZE 1024
 char* sprintfBuffer=(char*) malloc(SPRINTF_BUFFER_SIZE);
 int iLengthMsg = strlen( pMsg);
 const char* strTrailingPart= isPassed?"[ PASSED ]":\
 "[ FAILED ]"; 
 int iLengthTrailingPart=strlen(strTralingPart);
 int iNumDots= SCREEN_SIZE_X - iLengthMsg - iLengthTrailingPart;
 sprintf( sprintfBuffer,pMsg);
 for(int i=0; i< iNumDots; i++)
 {
 sprintf( sprintfBuffer+strlen( sprintfBuffer),\
 "%c" , '.');
 }
 sprintf(sprintfBuffer+strlen(sprintfBuffer), \
 strTrailingPart);
 printf( sprintfBuffer);
 free (sprntfBuffer); 
} 

Any better way to do this thing?

• Mat: Any reason why you're only using C facilities if this is indeed C++?
• sandun dhammika: No much reason, not at all. It just a pattern , when I land on firmware or inside kernel mode, then I don't have to re-think my pattern so.
• Roland Illig: This code doesn't compile. You have several typos.

Answer 1

Let me assume you're writing in C++ (leaving a quick review of C++ Cish code for the end.)

For clarity I prefer to split code in separate functions, they may be even reused.

void print_message(const std::string& message, bool isPassed)
{
 const std::string result = resolve_result_message(isPassed);
 std::cout << message << get_line_padding(message, result) << result << std::endl;
}
std::string resolve_result_message(bool isPassed) {
 // Avoid hard-coded strings, move these to constants
 return isPassed ? "[ FAILED ]" : "[ PASSED ]";
}
std::string get_line_padding(const std::string& message, const std::string& result) {
 const int requiredPadding = SCREEN_WIDTH - message.length() - result.length();
 if (requiredPadding <= 0)
 return "";
 return std::string(requiredPadding, '.');
}

---

About original C code:

You have a maximum output size (which, BTW, will be unaligned with other lines) set to SPRINTF_BUFFER_SIZE. Note that if input is bigger than this then your application may crash and you do not perform any check about this. You may want to cut output (in this case just reused SCREEN_SIZE_X + 1 constant).

If you're using a C compiler then do not cast return value of malloc().

sprintf() to print characters is absolutely an overkill, after all this is enough (don't forget to add a NUL character at the end):

for (int start = strlen(sprintfBuffer), i = 0; i < iNumDots; ++i)
 sprintfBuffer[start + i] = '.';

Now you can see it's equivalent to (after checking if iNumDots is positive):

memset(sprintfBuffer + strlen(sprintfBuffer), '.', iNumDots);

If this function isn't invoked by multiple threads you can avoid to allocate/free the buffer each time marking it as static.

---

If you're working on resource limited environment you may want to avoid unnecessary allocations, for this example let me assume your code is single threaded and message is short enough to fit one line, let me write it in C:

void print_message(const char* message, bool isPassed) {
 static char buffer[SCREEN_WIDTH + 1] = { 0 };
 const char* passed = isPassed ? "[ FAILED ]" : "[ PASSED ]";
 memset(buffer, '.', SCREEN_WIDTH);
 memcpy(buffer, message, strlen(message);
 memcpy(buffer + SCREEN_WIDTH - strlen(passed), passed, strlen(passed);
 puts(buffer);
}

Answer 2

There's no need to backslash newlines that aren't being preprocessed - newline is as valid as any other whitespace there.

We normally prefer static const int over #define.

You misspelt strTralingPart - the code didn't compile until I changed that to strTrailingPart.

There are several issues with your memory allocation:

• failure to include the necessary header for malloc() (not to mention free(), sprintf() and others)
• using malloc() at all - we normally use new[] (or, preferably, std::make_unique()) in C++.
• allocating memory at all - the lifetime of this memory is the function call, so why not just allocate it on the stack?

You don't truncate the message if it's too long to fit - you should prefer strncpy() and snprintf() to strcpy() or sprintf().

There's a format-string injection vulnerability in printf( sprintfBuffer); - you should use printf("%s", sprintfBuffer); to avoid problems when the buffer contains %. (If you finished the message with \n, you could use puts() instead, but I'm assuming you want to be able to overwrite the status with print_message("\r.... A similar format-string vulnerability exists in sprintf( sprintfBuffer,pMsg);.

---

It may be simpler to write the background dots to the buffer (which only needs to store SCREEN_SIZE_X characters plus a trailing null), then write the message and status over the top:

#include <cstring>
#include <cstdio>
static const int SCREEN_SIZE_X = 80;
void print_message(const char* message, bool isPassed)
{
 char buffer[SCREEN_SIZE_X+1];
 // fill with dots
 std::memset(buffer, '.', (sizeof buffer) - 1);
 auto len = std::snprintf(buffer, (sizeof buffer) - 1, "%s", message);
 // overwrite the null, if snprintf wrote one
 // if it failed, len is negative, and becomes very large after cast
 if ((size_t)len < sizeof buffer)
 buffer[len] = '.';
 // Safe to use strcpy here: we ensure it won't overrun
 const auto status = isPassed?"[ PASSED ]":"[ FAILED ]";
 std::strcpy(buffer + (sizeof buffer) - std::strlen(status) - 1, status);
 std::printf("%s", buffer);
}
int main()
{
 print_message("foo", true);
 std::puts("");
 print_message("0123456789012345678901234567890123456789"
 "0123456789012345678901234567890123456789",
 false);
 std::puts("");
}

(I've changed some of your variable names, as the originals were hard to pronounce).

If you want to make this more user-friendly, you might want to consider allowing code to pass a format string and parameters - you'll then want to replace the snprintf() with a suitable vsnprintf().

---

If you don't want to accept a format string, then there's no need to copy the input - you can send it directly to printf() if you specify an appropriate precision for each string:

int print_message(const char* message, bool isPassed)
{
 static const auto dots =
 "........................................"
 "........................................";
 const auto status = isPassed ? "[ PASSED ]" : "[ FAILED ]";
 const int status_len = std::strlen(status);
 const int message_len = std::strlen(message);
 if (message_len + status_len >= SCREEN_SIZE_X) {
 // truncate the message
 return std::printf("%.*s%s",
 SCREEN_SIZE_X - status_len, message,
 status);
 } else {
 // pad the message
 return std::printf("%s%.*s%s",
 message,
 SCREEN_SIZE_X - status_len - message_len, dots,
 status);
 }
}