Hey guys, this is my solution to a simple problem and I was looking for some feedback on my code. Please post any criticism you may have about it.

//Test project for string to binary conversion//

#include <iostream>
#include <string>

using namespace std;

void binConvert(string ConvertMe){

    int ArraySize = ConvertMe.length();
    int* asciiArray = new int [ArraySize];
    int binValues[8];
    int decValues[8] = {128,64,32,16,8,4,2,1};
    // Convert each char of ConvertMe into its ascii value
    for(int i = 0; i < ArraySize; i++){
        asciiArray[i] = ConvertMe[i];
    }

// Convert asciiArray values from decimal to binary and output to screen
    for(int i = 0; i < ArraySize; i++){
        // convert each ascii value into its binary 8 bit equivalent
        for (int a = 0; a < 8; a++){
            if (asciiArray[i] >= decValues[a]){
                binValues[a] = 1;
            }
            else if (asciiArray[i] < decValues[a]){
                binValues[a] = 0;
            }
            if (binValues[a] == 1){
                asciiArray[i] = asciiArray[i] - decValues[a];
            }
        }

        for (int a = 0; a < 8; a++)
            cout << binValues[a];
    }
}

int main(){

    string ConvertMe;

    cout << "What do you want to convert: ";
    getline(cin, ConvertMe);

    binConvert(ConvertMe);
}

I do have a question about my use of for loops in lines 20-36. Is it an accepted practice for me to nest the two for loops inside the first, or is something like that generally frowned upon? If it's frowned upon, what's another way I could accomplish the same thing?
Also, I didn't see anything about it in the rules, would it be okay for me to post my projects like this regularly as I get better? Writing code and receiving criticism(good or bad) on it seems to help me learn faster.

Edited 3 Years Ago by Sci3nc3F1cti0n

You don't need the second loop as you can call cout immediately after you have written the value in the first loop.

Additionally, I would have picked better variable names for your loop indexers, but this is only personal preference.

Your variable naming doesn't follow a single style/standard, pick one and stick to it :) Points noted were your use of capitals and miniscule on the first word of local variable names and method names should start with a capital letter.

class MyClass
{
    private: int _member;
    public: int Member;

    void Method()
    {
        MyLocalVariable localVariable;
    }
};

Above is the style I use :)

The arithmatic on line 30 can be placed inside your if statement on line 23. There's no need to do a second check as you already know that the statement will be true here. Also you don't need the else if just else will do. There can't be any other condition and so saves doing another condition check.

That is my quick evaluation :)

Edited 3 Years Ago by Ketsuekiame

You can also shorten your code considerably using the pow function and leveraging the implicit cast between char and int. something like this:

string ToBinary(int input)
{
    //Fill the array with 0's and only change the needed elements to a 1
    char output[9] = "00000000";
    for(int i= 7; i >= 0; i--)
    {
        if(input >= pow(2.0,i))
        {
            //This reverses the output to match what you're code did
            output[7-i] = '1';
            input = input - pow(2.0,i);
        }

    }
    return output;
}
string binConvert(string input)
{
     string output;
     for(int i= 0;i <input.length();i++)
        output += ToBinary((int)input[i]);
     return output;
}

Then just cout << binConvert(ConvertMe); in main.
In my tests it produces the same output.
I split off the actual conversion as it would be more useful in other situations this way.

Edited 3 Years Ago by tinstaafl

@Ketsuekiame: Thank you! I went through and changed the names so they all fit one style, and I also made the other changes you recommended.

@tinstaafl: Hmm, I didn't even think about doing it that way. As far as I can tell output += ToBinary((int)input[i]); gets rid of my need for the ASCII conversion for loop while concatenating each 8 bit output into a single output, and (int)input[i] is what leverages the implicit cast, right? Thank you!

Edited 3 Years Ago by Sci3nc3F1cti0n

(int)input[i] is what leverages the implicit cast, right?

Yes the char type is basically an int in a specicific range.

You're very welcome. Please remember to mark this solved thanks.

This question has already been answered. Start a new discussion instead.