Look! Smoke! Who do we blame?

Question

Recently, I found a way to generate specific blame for SmokeDetector. That was cool, so I decided to make a C++ program for it. I also have it on GitHub. This program is called Blame Bot.

Blame Bot does not need any flags to compile, so you can use make blame to compile it.

How it works:

Blame Bot takes an input of a number. This input is your chat id. This number is taken from your chat link. Mine would be: https://chat.stackexchange.com/users/301688/grant-garrison, and my number would be 301688. Then, the number is converted to base 7. A switch statement then outputs the characters. (Note: the characters must be in double quotes.)

#include <iostream>

int main(){
    int digit;
    std::string stringDigit, answer, instring;
    std::cin>>instring;
    int in = std::stoi(instring,nullptr);
    while (in != 0){
        digit = in % 7;
        if (digit < 10){
            stringDigit = '0' + digit;
        }else{
            stringDigit = digit - 10 + 'A';
        }
            answer = stringDigit + answer;
            in /= 7;
    }
    std::cout<<"!!/blame\u180E ";
    int i = 0;
    while(answer.length()>=i){
        switch (answer[i]-'0'){
            case 0: std::cout<<"\u180E";
                break;
            case 1: std::cout<<"\u200B";
                break;
            case 2: std::cout<<"\u200C";
                break;
            case 3: std::cout<<"\u200D";
                break;
            case 4: std::cout<<"\u2060";
                break;
            case 5: std::cout<<"\u2063";
                break;
            case 6: std::cout<<"\uFEFF";
                break;
        }
        i++;
    }
    std::cout<<"a\n";
}

What I'm looking for:

I'm looking for efficiency and syntax improvements. I know the program is fast already, but any extra computation cut out is a bonus. I do want to leave the { and } on the if statements though, as I think it improves readability for me. I also do not want to use any libraries.

What I think can be improved:

I think that switching back and forth from std::string and int is inefficient, and I'm sure there is a better way that doesn't need that. I also think that the base conversion algorithm might have a better replacement.

Answer 1

Spacing

The usage of (horizontal) space is not consistent, you have a space after if and switch, but not after else and while. I would generally use more whitespace to increase the readability, in particular after keywords, around operators, etc. For example

while (answer.length() >= i) {

Scope

It is a good habit to declare variables at the narrowest scope where they are used, and not at the top of the function. This applies for example to digit and stringDigit, which are only used inside the while-loop.

Input validation

std::stoi(instring, nullptr) silently ignores leading whitespace and trailing non-digits, that might or might not be wanted. The pos parameter could be used to check if the complete string has been converted successfully.

Also std::stoi(instring, nullptr) throws an std::invalid_argument exception if the input string contains no valid digits at all, you might want to catch that.

Iterating too far

In

int i = 0;
while(answer.length()>=i){
    switch (answer[i]-'0'){
        // ...
    }
    i++;
}

the last iteration accesses answer[answer.length()], which is the null character. Even if this causes no harm (answer[i]-'0' then matches none of the cases) it is probably unintended and you'll want answer.length() > i instead. (See below for further improvements of this part.)

Simplifications

The remainder modulo 7 cannot be >= 10, so the while loop simplifies to

while (in != 0) {
    int digit = in % 7;
    std::string stringDigit;
    stringDigit = '0' + digit;
    answer = stringDigit + answer;
    in /= 7;
}

which can further be simplified to

while (in != 0) {
    char digit = '0' + in % 7;
    answer.insert(0, 1, digit);
    in /= 7;
}

making the stringDigit variable obsolete, and mutating answer instead of concatenating strings.

Iterating over all valid string indices is better done with a for loop:

for (int i = 0; i < answer.length(); ++i) {
    // ... use `answer[i]` ...
}

But actually you don't need the index, only the characters, and iterating over all characters in a string can be done more simply with a range-based for loop (added in C++ 11):

for (char& c : answer)
    // ...
}

The switch statement can be replaced by an array lookup:

std::string blameStrings[] = {
    "\u180E", "\u200B", "\u200C", "\u200D", "\u2060", "\u2063", "\uFEFF"
};
for (char& c : answer) {
    std::cout << blameStrings[c - '0'];
}

(You can probably choose a better name for the array, but I have no idea what those strings stand for.)

Get rid of the intermediate string storage

Your code computes answer as the base-7 representation of the input number, and then iterates over the characters in that string.

The conversion from digits to characters in a string and back to digits can be avoided by storing the integer digits in a vector instead. And instead of prepending additional digits, we can append them (to avoid that memory needs to be moved repeatedly) and then iterate over the result in reverse order:

#include <vector>

// ...

int in = std::stoi(instring, nullptr);
std::vector<int> digits;
while (in != 0) {
    digits.push_back(in % 7);
    in /= 7;
}

std::cout<<"!!/blame\u180E ";
for (auto it = digits.rbegin(); it != digits.rend(); ++it) {
    std::cout << blameStrings[*it];

}
std::cout<<"a\n";

Answer 2

    int digit;
    std::string stringDigit, answer, instring;
    std::cin>>instring;
    int in = std::stoi(instring,nullptr);

Declare variables as you need them.

Every file should be self-sufficient. Make sure you #include the headers for components you are using. Do not rely on implicit inclusion.

Check that std::cin successfully read a value from the stream.

Rather than jumping through the hoop of std::stoi conversion, just read the value as an integer.

#include <iostream>
#include <string>

int main() {
    int in;
    if (!(std::cin >> in)) {
        std::cerr << "Invalid user-id";
        return EXIT_FAILURE;
    }

---

    while (in != 0){
        digit = in % 7;
        if (digit < 10){
            stringDigit = '0' + digit;
        }else{
            stringDigit = digit - 10 + 'A';
        }
            answer = stringDigit + answer;
            in /= 7;
    }

Use symbolic constants instead of magic constants ('0', 'A', 7).

If you are taking a modulus of 7, digit will never be greater than 7. The else branch is never taken.

---

Keep functions short and simple. Each function should perform a single logical operation. Functions then become simpler to understand, test, and reuse.

You find the remainders, format them to ASCII, unformat them back to the remainders (indices), then format them to your own custom format. Just save the intermediate remainders and map to your own values.

#include <algorithm>
#include <array>
#include <iostream>
#include <iterator>
#include <vector>

int read_int(std::istream& in) {
  // ...
}

std::vector<int> itoa_unformatted(int number, int base) {
  // ...
}

int main() {
  constexpr auto mappings = std::make_array("\u180E", "\u200B", "\u200C", "\u200D", 
                                            "\u2060", "\u2063", "\uFEFF");

  auto user_id = read_int(std::cin);
  auto digits = itoa_unformatted(user_id, mappings.size());
  transform(digits, std::make_ostream_joiner(std::cout, ""),
            [&mappings](auto index) { return mappings[index]; });
  std::cout << "a\n";
}

Note - Example code for exposition only. std::make_array and std::make_ostream_joiner are part of C++20 (Library Fundamentals V2). transform is just a helper for std::transform over a container. If you would like to play with these features now on gcc/clang, see mnmlstc core.

If your base is never going to change, you don't need a generalized base conversion solution then. The base conversion could be specialized for your specific use case, from sizing the appropriate buffer to strength reducing the expensive operations (like division/modulus). Always measure before optimizing.

Answer 3

Missing header

We need to include <string> to get declarations for std::string and std::stoi.

Compile with warnings enabled

You say you don't need any compilation flags. Whilst that's true (providing your compiler defaults to C++11 or later), it's best to enable some warning flags. That helps identify defects such as this:

g++ -std=c++17 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Weffc++ 191455.cpp -o 191455
orig.cpp: In function ‘int main()’:
orig.cpp:20:26: warning: comparison of integer expressions of different signedness: ‘std::__cxx11::basic_string<char>::size_type’ {aka ‘long unsigned int’} and ‘int’ [-Wsign-compare]
     while(answer.length()>=i){
           ~~~~~~~~~~~~~~~^~~

Although the mismatch won't cause an actual bug, it can be trivially fixed simply by changing i to an unsigned type. Future readers can then be more confident about your code if you don't leave traps such as that.

Just read from input

There's little to be gained by reading into a string and then calling std::stoi() without checking the end pointer. Just stream directly into the integer.

Don't write your own code to convert to octal

That's why we have the std::oct stream manipulator.

Don't add '0' to turn numbers into digits and then subtract it again

Either change the case to switch on the digits (e.g. case '0':, etc), or work directly with small integers. I prefer the latter, as we can use a simple array to look up the output characters.

Use for to show looping over elements

Consider this loop:

int i = 0;
while(answer.length()>=i){
    //...
    i++;
}

That's more idiomatically written as:

for (int i = 0;  i <= answer.length();  ++i)

The off-by-one error becomes more obvious. Better to avoid that by iterating over the characters:

for (auto c: answer)

Don't use narrow streams for wide characters

If we're printing wide-character strings, we should be writing to std::wcout rather than to std::cout.

Add some unit tests

Enough said.

---

Replacement code

With no specification, I can't check whether the output is correct, but it produces the same output as the original code for the given test case (301688), as demonstrated by the built-in self-test.

#include <string>
#include <vector>

std::wstring encode(unsigned long userid)
{
    static wchar_t const code[7] = {
        L'\u180E', // 0 => MONGOLIAN VOWEL SEPARATOR
        L'\u200B', // 1 => ZERO WIDTH SPACE
        L'\u200C', // 2 => ZERO WIDTH NON-JOINER
        L'\u200D', // 3 => ZERO WIDTH JOINER
        L'\u2060', // 4 => WORD JOINER
        L'\u2063', // 5 => INVISIBLE SEPARATOR
        L'\uFEFF', // 6 => ZERO WIDTH NO-BREAK SPACE
    };

    std::vector<wchar_t> octalid;
    for (auto i = userid;  i;  i/=7) {
        octalid.push_back(code[i % 7]);
    }

    return L"!!/blame\u180E "
        + std::wstring(octalid.rbegin(), octalid.rend())
        + L"a\n";
}

#include <iostream>
#include <locale>

int main(int argc, char **argv)
{
    if (argc == 1) {
        // no arguments - run self tests
        return
            + (encode(0) != L"!!/blame\u180E a\n")
            + (encode(1) != L"!!/blame\u180E \u200Ba\n")
            + (encode(6) != L"!!/blame\u180E \uFEFFa\n")
            + (encode(7) != L"!!/blame\u180E \u200B\u180Ea\n")
            + (encode(8) != L"!!/blame\u180E \u200B\u200Ba\n")
            + (encode(301688) != L"!!/blame\u180E \u200C\u200D\uFEFF\u2060\u200D\uFEFF\u200Ca\n");
    }

    std::locale::global(std::locale{""});

    for (int i = 1;  i < argc;  ++i) {
        try {
            std::size_t pos;
            unsigned long userid = std::stoul(argv[i], &pos);
            if (argv[i][pos])
                throw std::invalid_argument("not a number");
            std::wcout << encode(userid);
        } catch (std::exception& e) {
            std::cerr << "Invalid argument: " << argv[i]
                      << ": " << e.what() << std::endl;
        }
    }
}

Answer 4

I'm posting this just to give a full round improvement summary, plus I found a few improvements myself, but please remember these mainly are not my improvements. Vote the other answers up! This code was shortened to nearly 1/3 of its size!

Final code:

#include <iostream>
#include <string>

int main(){
    int in;
    std::string answer;
    std::cin>>in;
    std::string blameStrings[] = {
        "\u180E", "\u200B", "\u200C", "\u200D", "\u2060", "\u2063", "\uFEFF"
    };
    while (in != 0) {
        answer.insert(0,blameStrings[(in % 7)]);
        in /= 7;
    }
    std::cout<<"!!/blame\u180E "<<answer<<"a\n";
}

Line by line:

I'm only going explain code I changed. You can visit (and upvote) the other answers for their explanations.

#include <iostream>
#include <string>

The additional #include was suggested by Snowhawk.

int main(){
    int in;
    std::string answer;
    std::cin>>in;

The removal of extra variables was suggested by both Snowhawk and Martin R. The removal of stoi was suggested by Snowhawk. I also personally removed a few extra variables.

    std::string blameStrings[] = {
        "\u180E", "\u200B", "\u200C", "\u200D", "\u2060", "\u2063", "\uFEFF"
    };

The array was suggested by Martin R.

    while (in != 0) {
        answer.insert(0,blameStrings[(in % 7)]);
        in /= 7;
    }

This part of the code was suggested by Martin R. However, I saw that there was a loop iterating through what the first loop had created, so I cut the second loop out.

    std::cout<<"!!/blame\u180E "<<answer<<"a\n";
}

This just outputs everything in one simple call to the std::cout.

Remember, most of these aren't my improvements, so make sure you upvote the answers that the improvements came from.