A simple C++ trading system demo

I need my trading system demo reviewed. It implemented a simple system which parse the trading flow (logged in file) of different trader and calculate some important feature.

packets.h

#ifndef PACKET_H
#define PACKET_H
#include 

class Header{
public:
    char marker[3];
    uint8_t msg_type;
    uint64_t sequence_id;
    uint64_t timestamp;
    uint8_t msg_direction;
    uint16_t msg_len;
};

class OrderEntry{
public:
    uint64_t price;
    uint32_t qty;
    char instrument[11];
    uint8_t side;
    uint64_t client_assigned_id;
    uint8_t time_in_force;
    char trader_tag[4];
    uint8_t firm_id;
    char firm[257];//via
};

class OrderAck{
public:
    uint32_t order_id;
    uint64_t client_id;
    uint8_t order_status;
    uint8_t reject_code;
};

class OrderFill{
public:
    uint32_t order_id;
    uint64_t fill_price;
    uint32_t fill_qty;
    uint8_t no_of_contras;
//reapeating group ignored
};
#endif

parser.h

#ifndef PARSER_H
#define PARSER_H
#include 
#include 
using namespace std;

class Header;
class OrderAck;
class OrderFill;
class OrderEntry;

class Parser {
public:
    static void readHeader(Header& h, fstream& fin);
    static  void readOrderEntry(uint16_t len, fstream& fin, OrderEntry& o);
    static  void readOrderAck(uint16_t len, fstream& fin, OrderAck& o);
    static  void readOrderFill(uint16_t len, fstream& fin, OrderFill& o);
private:
    static uint16_t readUInt16(char* buf);
    static uint16_t readUInt8(char* buf);
    static uint64_t readUInt64(char* buf);
    static uint32_t readUInt32(char* buf);
};

#endif

parser.cpp

```
#include "packets.h"
#include "parser.h"
//#define CHECK_BORDER
void Parser::readHeader(Header& h, fstream& fin){
char buf[8];
fin.read((char*)h.marker, 2);
h.marker[2] = 0;
fin.read((char*)&h.msg_type, 1);
fin.read(buf, 8);
h.sequence_id = readUInt64(buf);
fin.read(buf, 8);
h.timestamp = readUInt64(buf);
fin.read((char*)&h.msg_di

I have found a couple of things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. In particular you should never use it in a header file.

Use the appropriate headers

Instead of using stdint.h you should instead use:

#include 

This puts things from that header into the std:: namespace instead of only into the global namespace and will save headaches later. (Note that it might additionally put items into the global namespace, but as a programmer you should avoid relying on that implementation-defined behavior.)

Eliminate unused variables

This code declares a number of variables (term, qty, nread) but then does nothing with them. Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.

Avoid magic numbers

Lines that include numbers without explanation are a maintenance problem waiting to happen. For example, the code includes this line with no comment:

len = len -2 -1 -8 -8 -1 -2;

It's probable that those aren't actually random but there is no hint as to the significance of those numbers.

Don't use while (!fin.eof())

It's almost always an error to write code that uses while (!fin.eof()) or the equivalent, because what you're interested in determining is if there is any data left and not whether it happens to be the end of the stream. See this question for more details on that.

Don't proliferate useless variables

The code currently includes this pair of lines:

string instrument(orderEntry.instrument);
currentOrderInstrument = instrument;

The instrument string is never used again. Why not just write this instead?

currentOrderInstrument(orderEntry.instrument);

Use appropriate control flow structures

Within main, the code decides how to handle the data based on the msg_type field. It has a cascading if...else with an empty final else clause. However, this would be much clearer written as a switch statement with each case representing a different message type.

Use standard algorithms

The current code include this sequence:

uint32_t maxActiveQty = 0;
std::string maxActiveTrader;
for (std::map::iterator it = activeTraderQty.begin(); it!=activeTraderQty.end(); ++it){
    if (it->second > maxActiveQty){
        maxActiveQty = it->second;
        maxActiveTrader = it->first;
    }
}

The purpose seems to be to identify the largest trading quantity and the associated trader. However there is already an algorithm for this which is std::max_element

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

Use objects more completely

Your code has classes but they seem almost exclusively used a simple containters, such as OrderEntry and OrderAck. It would make your code simpler and easier to understand and maintain if you also added member functions to them to do the things that are currently being done to these structures from within main.

StackExchange Code Review Q#88488, answer score: 6