Convert characters to octal, binary, and integer

Question

I have implemented a sample program that converts vector of characters to its integer representation depending on the bit size specified. I was hoping to get some input on how to simplify the sequence. I think my approach is convoluted.

#include "stdafx.h"

#include <iostream>
#include <vector>
#include <iomanip>
#include <bitset>
#include <sstream>
#include <algorithm>
#include <iterator>

int OctalToBinary(int octalNum) {
    int decimalNum = 0, binaryNum = 0, count = 0;

    while (octalNum != 0) {
        decimalNum += (octalNum % 10) * pow(8, count);
        ++count;
        octalNum /= 10;
    }
    count = 1;
    while (decimalNum != 0) {
        binaryNum += (decimalNum % 2) * count;
        decimalNum /= 2;
        count *= 10;
    }
    return binaryNum;
}

int main()
{
    std::vector<char> buffer = { '3','7','4','3','0'};
    std::vector<int> bitsize = { 3,3,8 };
    std::vector<char> collection;
    std::vector<std::string> output;

    // convert characters to octal and 

    for each ( char c in buffer)
    {
        int x = c - '0';
        std::stringstream opt;
        opt << std::setw(3) << std::setfill('0') << OctalToBinary(x) << "\n";

        std::string val = opt.str();

        std::copy(val.begin(), val.end()-1, std::back_inserter(collection));
    }

    int start_index = 0;
    int end_index = 0;

    // convert binary values to integer
    for (int i = 0; i < bitsize.size(); i++)
    {
        start_index = end_index;
        end_index += bitsize[i];

        std::string temporary_string(collection.begin() + start_index, collection.begin() + end_index);
        int converted_integer = std::stoi(temporary_string.c_str(), nullptr, 2);
        std::cout << temporary_string << " " << converted_integer << "\n";
    }

    // output should be 3, 7 , 140

    int x;
    std::cin >> x;
    return 0;
}

Answer 1

Unnecessary Headers
The #includes for iterator and bitset are not needed, the code compiles fine without them, iterator may be included indirectly through vector.

Adding unnecessary headers can increase compile/build times and may cause other problems in larger more complex programs.

Portability
This program is currently not portable for 2 reasons, the use of the Visual Studio generated #include "stdafx.h" and the use of the non-standard for each loop. It might be better to embed #include "stdafx.h" within ifdef/endif.

#ifdef windows
#include "stdafx.h"
#endif

I compiled this in Visual Studio 2017 on Windows 10 and it actually reported a compile error on the for each loop. The suggested fix was to use a range based for loop.

1>------ Build started: Project: octbinintconv0, Configuration: Debug Win32 ------
1>octbinintconv0.cpp
1>d:\projectsnfwsi\codereview\octbinintconv0\octbinintconv0\octbinintconv0.cpp(18): warning C4244: '+=': conversion from 'double' to 'int', possible loss of data
1>d:\projectsnfwsi\codereview\octbinintconv0\octbinintconv0\octbinintconv0.cpp(40): error C4496: nonstandard extension 'for each' used: replace with ranged-for statement
1>d:\projectsnfwsi\codereview\octbinintconv0\octbinintconv0\octbinintconv0.cpp(55): warning C4018: '<': signed/unsigned mismatch
1>Done building project "octbinintconv0.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

You might want to replace the for each loop with the following code, which uses an iterator and is part of standard C++.

    for (auto c : buffer)
    {
        int x = c - '0';
        std::stringstream opt;
        opt << std::setw(3) << std::setfill('0') << OctalToBinary(x) << "\n";

        std::string val = opt.str();

        std::copy(val.begin(), val.end() - 1, std::back_inserter(collection));
    }

Type Mismatches
A good practice is treating warning messages as errors, since they point to potential logic errors. The pow() function returns double and the result is being truncated, it might be better to use a static cast in the code to show that this is intentional.

    decimalNum += (octalNum % 10) * static_cast<int>(pow(8, count));

In this for loop it would be better to define the local variable i as size_t rather than int, in any of the STL container classes the function size() returns size_t.

    for (size_t i = 0; i < bitsize.size(); i++)

Complexity
Creating the function OctalToBinary(int octalNum) was good but there are two more functions that main() can be broken up into, they have even been identified by comments, both of the for loop are good candidates for functions.