patterncppMinor
NMEA 0183 checksum validation
Viewed 0 times
checksum0183validationnmea
Problem
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:
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*23using 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 AsciiValues;
{
bool done = false;
while (!done)
{
int h ( MsgLine.at (i) );
i++;
AsciiValues.push_back(h);
if ( i == j )
done = true;
}
}
//XOR validation
bitset a = (AsciiValues[0]);
bitset b = (AsciiValues[1]);
bitset 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 z = PosOne;
bitset 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;
}Solution
I believe this can be simplified quite a bit. My immediate reaction would be to write code something like this:
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:
...can be turned into something like this:
Or, you could just use
...could be simplified down to something like this:
...or you could do something closer to the
#include
#include
#include
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.Code Snippets
#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";
}bool done = false;
while (!done)
{
int h ( MsgLine.at (i) );
i++;
AsciiValues.push_back(h);
if ( i == j )
done = true;
}for (int i=0; i<j; i++)
AsciiValues.push_back((int)MsgLine.at(i));bool done = false;
while (!done)
{
a = (AsciiValues[d]);
b = c;
c = a ^ b;
d++;
if ( d == e )
done = true;
}do {
a = AsciiValues[d];
b = c;
c = a ^ b;
++d;
} while (d != e);Context
StackExchange Code Review Q#118379, answer score: 8
Revisions (0)
No revisions yet.