6
\$\begingroup\$

I am looking to simplify my main() function. The nested if statements contain four lines of the same code, and I'm unsure of how to refactor and simplify it. I was attempting to use booleans, which seemed like a safe bet, but the logic gets skewed in the process.

#include <bits/stdc++.h>
#include <iostream>
#include <string>

int bits(unsigned long d) {
    int l;

    if (d <= 255) {
        l = 8;
    }
    else if (d > 255 && d <= 65535) {
        l = 16;
    }
    else if (d > 65535 && d <= 4294967295U) {
        l = 32;
    }
    else {
        std::cout << "32 bits (4294967295) or smaller." << std::endl;
    }

    std::cout << "Bits..................... " << l << std::endl;

    return l;
}

std::string convertToBinary(unsigned long decimal) {
    int l = bits(decimal);
    int i, r;
    std::string str;

    //  creates array
    for (i = 0; i < l; i++) {
        r = decimal % 2;
        decimal /= 2;
        str += std::to_string(r);
    }

    //  reverses array for binary value
    reverse(str.begin(), str.end());

    std::cout << "Binary Value............. " << str << std::endl;

    return str;
}

int main(void) {
    std::string input;
    std::cout << "------------ Input -----------" << std::endl;
    std::cin >> input;

    std::cout << "Input: " << input << std::endl;
    std::cout << "Length: " << input.length() << std::endl;

    int i, count = 0, j = 0;
    int length = input.length();
    int ascii;
    unsigned long nums;

    int numbers[33];
    std::string binaries;
    std::string chars;

    for (i = 0; i < length; i++) {
        //  if next input is digit
        if (isdigit(input[i])) {
            numbers[j] = input[i];
            // place digit in next decimal
            count = (numbers[j] - '0') + count * 10;
            //  if next input is char
            if (i == length - 1) {
                if (isdigit(input[length - 1])) {
                    nums = count;
                    std::cout << "------------ " << nums << " ------------" << std::endl;
                    binaries += convertToBinary(nums) + ' ';
                    count = 0;
                }
            }
            //  next number
            j++;
        }
        //  if next input is char
        else {
            chars[i] = input[i];
            ascii = (int) chars[i];
            //  if next input is digit
            if (count != 0) {
                nums = count;
                std::cout << "------------ " << nums << " ------------" << std::endl;
                binaries += convertToBinary(nums) + ' ';
                count = 0;
            }
            //  != end of letters
            std::cout << "------------ " << chars[i] << " ------------" << std::endl;
            std::cout << "ASCII.................... " << ascii << std::endl;
            binaries += convertToBinary(ascii) + ' ';
        }
    }

    std::cout << "\n------- Binary Value of " << input << " -------" << std::endl;
    std::cout << binaries << std::endl;

    return 0;
}

The program takes an input, then outputs the corresponding number of bits, binary value, and if necessary, an ASCII value. If the user inputs "c357g98", then the program should get ASCII and binary values of "c", then retrieve the binary value of "357" (rather than "3", "5", then "7" individually), so any adjacent digits will be treated as a single int instead of separate integers. Each time I attempt to refactor, the program loses its ability to store the adjacent digits together as a single int.

In the first attempt at refactoring, I tried this, which takes the count and binary parameters:

void getNumbers(int c, std::string b) {
    std::cout << "------------ " << c << " ------------" << std::endl;
    b += convertToBinary(c) + ' ';
    c = 0;
}

Since the count is never returned, the input "c357g98" is treated as "c357g35798" and the numbers never reset.

So in my second attempt, I return the count variable as an int:

int getNumbers(int c, std::string b) {
    std::cout << "------------ " << c << " ------------" << std::endl;
    b += convertToBinary(c) + ' ';
    c = 0;

    return c;
}

And replace the four lines of code with a reference to the function as:

count = getNumbers(count, binaries);

This method converts each value correctly, but does not store the final binary values of the digits, only the characters (since the reference to the string binaries was moved and not returned).

I've also done away with the nums variable entirely.

I'm coming to terms with the idea that my entire program may need heavily refactored in order for the logic to be simplified, but I was hoping to start with the messy main() function for now.

\$\endgroup\$
  • \$\begingroup\$ What have you attempted to do to refactor it? Assuming the four lines you are referring to are the ones beginning with nums = count;, why can't you put those four lines in a function that accepts references to nums and count? \$\endgroup\$ – Null Mar 14 at 21:25
  • \$\begingroup\$ @Null I created a function that takes count and binaries, then returns the count. So the four lines are then replaced with count = getNumbers(count, binaries); This method will skip the digits entirely, and when the program is due to print all final binary values at the end, only the characters are converted. In my second attempt, I changed the function to string getNumbers and had the binaries string be returned instead, and the four lines of code in the nested ifstatements were replaced with binaries += getNumbers(count, binaries); This method does not reset the counter. \$\endgroup\$ – J Pex Mar 14 at 21:46
  • \$\begingroup\$ It's hard to follow that in a comment. Please edit your question with the attempted refactor code. \$\endgroup\$ – Null Mar 14 at 21:56
  • \$\begingroup\$ @Null I apologize, I was unaware of the correct protocol for editing posts. I've edited the post now to include my attempts. \$\endgroup\$ – J Pex Mar 14 at 22:13
4
\$\begingroup\$

It's not only the main function that you should try to improve upon. Rather, when you build your supporting functions in a nice way, a simplified main function follows naturally.

First, as general comments, your program suffers from a bad separation of logic. Your functions are doing too much: they do processing, print output to the user, and return some value. The same problem is echoed in the main: input parsing is tightly knit together with printing and processing.

  • Don't include <bits/stdc++.h>, it's not portable. Also, see here for more on why not do it.

  • Your program crashes on multiple inputs due to out-of-indexing issues, so avoid C-arrays like int numbers[33]. Also, avoid magic numbers like 33 which makes the reader go "Huh, what 33?".

  • As explained in the other review(s), declare variables as late as possible because you write C++ and not C.

  • Standard functions and data structures will simplify your code massively. We can logically think about the following: read the input, split it into sensible parts, and process each part. That's it!

So in full, we could write your program also as follows:

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>
#include <bitset>
#include <iterator>

constexpr unsigned long MAX_BITSIZE = 32;

int bits(unsigned long d)
{
    if (d > 65535) {
        return 32;
    }
    else if (d > 255) {
        return 16;
    }
    else {
        return 8;
    }
}

// Return a bit string corresponding to the input.
std::string to_binary(unsigned long decimal)
{
    std::bitset<MAX_BITSIZE> bs(decimal);
    return bs.to_string().substr(MAX_BITSIZE - bits(decimal));
}

// Split the input string into parts, 
// e.g., "abc123e6g" is split into ("abc", "123", "e", "6", "g").
std::vector<std::string> get_parts(const std::string& str)
{
    std::vector<std::string> parts;

    for (auto first = str.cbegin(); first != str.cend(); )
    {
        auto change = std::adjacent_find(first, str.cend(),
            [](char a, char b) { return isdigit(a) != isdigit(b); });

        if (change != str.cend())
        {
            ++change;
        }

        parts.emplace_back(std::string(first, change));

        first = change;
    }

    return parts;
}

int main()
{
    // Reading input could be done a function.
    // We also omit all checks, and just assume it is valid.

    std::string input;
    std::cout << "------------ Input -----------\n";
    std::cin >> input;

    std::cout << "Input: " << input << "\n";
    std::cout << "Length: " << input.length() << "\n";

    const auto cont = get_parts(input);

    std::vector<std::string> binary;

    for (const auto& e : cont)
    {
        // Processing an integer
        if (isdigit(e.front()))
        {
            const std::string b = to_binary(stoi(e));

            std::cout << "------------ " << e << " ------------\n";
            std::cout << "Bits..................... " << bits(stoi(e)) << "\n";
            std::cout << "Binary value............. " << to_binary(stoi(e)) << "\n";

            binary.push_back(b);
        }
        // Processing individual characters
        else
        {
            for (const auto& ch : e)
            {
                const std::string b = to_binary(ch);

                std::cout << "------------ " << ch << " ------------\n";
                std::cout << "ASCII.................... " << int(ch) << "\n";
                std::cout << "Bits..................... " << bits(ch) << "\n";
                std::cout << "Binary value............. " << to_binary(ch) << "\n";

                binary.push_back(b);
            }
        }
    }

    std::cout << "\n------- Binary value of " << input << " -------\n";
    std::copy(binary.cbegin(), binary.cend(), std::ostream_iterator<std::string>(std::cout, " "));
}

A few comments about the above program:

  • Luckily, std::bitset has methods for printing a binary string that we can use to massively simplify your "to-binary" conversion function. Notice that this function does no printing; it's not its job. Remember: one function, one responsibility. Whoever uses this function decides when, how, and what to print (if anything).

  • The function get_parts takes care of all that messy processing of your main function (which is buggy - but don't feel bad, it's hard to get it right when you go so low level). The magic is taken care of std::adjacent_find which is used by a suitable lambda function that checks if the two adjacent indices contain characters of "different type" (i.e., a digit and a non-digit). If we wanted to, we could also modify this slightly to further split "abc" to "a", "b", and "c".

  • The main function is quite pretty now (and very readable compared to your original): just get the parts and process each. Printing could be further divided into more functions.

\$\endgroup\$
4
\$\begingroup\$

The first 3 lines in main can be turned into a function:

std::string getInput()
{
    std::string input;
    std::cout << "------------ Input -----------" << std::endl;
    std::cin >> input;
    return input;
}

int main()
    std::string input = getInput();

There are several portions of main that could become functions.

Declare Variable when needed

In the function convertToBinary the integer variables i and r are declared at the beginning of the function. They are only needed within the for loop.

    for (int i = 0; i < l; i++) {
        int r = decimal % 2;
        decimal /= 2;
        str += std::to_string(r);
    }

This is also true in the main function for i and other variables.

The contents of the then clause and the else could both be moved into functions.

Does the inner if statement (if (i == length - 1)) need to be within the for loop or could it be executed after the for loop is done? You might want to think about loop invariants.

Reduce Complexity When You Can

In the function bits, checking from the top down might make reading easier because the top limit isn't needed:

    if (d > 65535) {
        l = 32;
    } else if (d > 255) {
        l = 16;
    } else {
        l = 8;
    }
\$\endgroup\$
4
\$\begingroup\$

The use of the term ASCII is misleading. What's used here is actually the execution character coding, which may or may not be ASCII. C++ is intentionally portable enough to work on systems regardless of the encoding they use for characters.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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