Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I was hoping for a bit of advice on cleaning a couple of areas of this code up. In particular, the hex-to-binary section, the checksum is in hex, and the rest is in ASCII. I tried for a good day to do it with tables etc, but in frustration I ended up going with this.

Is there a better way of handling the "Store individual characters in vector" section? I went that way because the message can be of variable size (plus 1 or 2 characters).

Here's an example of the type of message:

!AIVDM,1,1,,A,13HOI:0P0000VOHLCnHQKwvL05Ip,0*23

using namespace std;

int main()

{
//Checksum Function

//Get Characters for Checksum
ifstream AisMsg("nmea.txt", ios::in);

    string MsgLine;
    getline(AisMsg, MsgLine, '*');
    MsgLine.erase(0,1);

        string HexVal;
        getline(AisMsg, HexVal);

AisMsg.close();

//Store individual characters in an element of an vector
int i = 0;
int j = ( MsgLine.length() );
vector <int> AsciiValues;
{
    bool done = false;
    while (!done)
    {
        int h ( MsgLine.at (i) );
        i++;
        AsciiValues.push_back(h);

    if ( i == j )
        done = true;
    }
}
//XOR validation
bitset<8> a = (AsciiValues[0]);
bitset<8> b = (AsciiValues[1]);
bitset<8> c = a ^ b;
    int d = 2;
    int e = (AsciiValues.size());

{
    bool done = false;
    while (!done)
    {
        a = (AsciiValues[d]);
        b = c;
        c = a ^ b;
        d++;

    if ( d == e )
        done = true;
    }
}
//Hex Checksum to Binary Conversion (messy, but does the job)
int PosOne = HexVal.at(0);
int PosTwo = HexVal.at(1);

    bitset<4> z = PosOne;
    bitset<4> x = PosTwo;

        ofstream Cheat("Amateur Hour.txt", ios::out | ios::app);
        Cheat << z << x << endl;
        ifstream Cheat1("Amateur Hour.txt", ios::in | ios::binary);

            string Chksm;
            getline(Cheat1,Chksm);

        Cheat.close();
        Cheat1.close();
        remove ("Amateur Hour.txt");


return 0;
}
share|improve this question

I believe this can be simplified quite a bit. My immediate reaction would be to write code something like this:

#include <sstream>
#include <iostream>
#include <numeric>

int main() { 
    std::istringstream in("$AIVDM,1,1,,A,13HOI:0P0000VOHLCnHQKwvL05Ip,0*23");

    std::string sentence;

    std::getline(in, sentence, '*');

    std::string received_checksum;

    // Save the checksum that was at the end of what we received.
    std::getline(in, received_checksum);

    // Re-compute the check-sum:
    char check = std::accumulate(sentence.begin()+1, sentence.end(), 0,
        [](char sum, char ch) { return sum ^ ch; });

    // Print out both the received and computed checksums:
    std::cout << "received checksum: " << received_checksum;
    std::cout << "\ncomputed checksum: " << std::hex << (int)check << "\n";
}

Looking at specific details of the code, I see a few things that even if you were going to preserve roughly your current implementation, you could still simplify and clarify. The first point would be to fix your indentation. Right now, it appears to be almost entirely arbitrary, and unrelated to the structure of the code.

Then you could clean up some of your loops. For example:

bool done = false;
while (!done)
{
    int h ( MsgLine.at (i) );
    i++;
    AsciiValues.push_back(h);

if ( i == j )
    done = true;
}

...can be turned into something like this:

for (int i=0; i<j; i++)
    AsciiValues.push_back((int)MsgLine.at(i));

Or, you could just use std::copy. Likewise, your code for computing the checksum:

bool done = false;
while (!done)
{
    a = (AsciiValues[d]);
    b = c;
    c = a ^ b;
    d++;

if ( d == e )
    done = true;
}

...could be simplified down to something like this:

do {
    a = AsciiValues[d];
    b = c;
    c = a ^ b;
    ++d;
} while (d != e);

...or you could do something closer to the accumulate (it still seems a lot cleaner to me to use std::accumulate), as I showed to start with.

share|improve this answer
    
Thanks for the reply and your code. Could you recommend a good book or online tutorial? – Ohcir Jan 31 at 1:29
    
@Ohcir: stackoverflow.com/q/388242/179910 – Jerry Coffin Jan 31 at 10:41
    
@ Jerry Coffin Thanks for the link. Trying again using 'stringstream' and 'accumulate'. Cheers for the pointers. – Ohcir Feb 1 at 6:43

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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