1
\$\begingroup\$

I'm writing software that trades on the market, keeping order books in sync. I keep wondering about the code readability, as I have written it myself, I am biased. Could anyone please review the code and show where I can improve on code readability?

#include "Market.hpp"

Market::Market(uint32_t mId, bool ime, CsvManager &cm)
    : marketId(mId), isMatchingEnabled(ime), lowestAsk(ENDPRICE),
      csvManager(cm), highestAsk(0), lowestBid(ENDPRICE), highestBid(0) {
    insertGenesisOrder();
}

void Market::insertGenesisOrder() {
    Order emptyOrder(0, ENDINDEX, "", true, OrderType::NEW, OrderSide::SELL, 0,
                     0);
    emptyOrder.next = ENDINDEX;
    emptyOrder.prev = ENDINDEX;
    orders.push_back(emptyOrder);
}

void Market::handleOrder(Order &order) {
    if (order.type == OrderType::NEW) {
        if (isMatchingEnabled) {
            sendAcknowledgement(order);
        }
        newOrder(order);
        if (!isMatchingEnabled && order.accepted) {
            sendAcknowledgement(order);
        }
    } else if (order.type == OrderType::CANCEL) {
        if (order.userOrderId <= orders.size() &&
            orders[order.userOrderId].userOrderId <= orders.size()) {
            order = orders[order.userOrderId];
            removeOrder(order.userOrderId);
            order.accepted = true;
            sendAcknowledgement(order);
        } else {
            sendReject(order);
        }
    } else if (order.type == OrderType::FLUSH) {
        flushOrders();
    }
    if (isTopUpdate(order) && order.accepted) {
        sendTopUpdate(order);
    }
}

bool Market::isTopUpdate(Order &order) {
    if (order.type == OrderType::NEW || order.type == OrderType::CANCEL) {
        if (order.side == OrderSide::BUY && order.price >= highestBid) {
            return true;
        } else if (order.side == OrderSide::SELL && order.price <= lowestAsk) {
            return true;
        }
    }
    return false;
}

void Market::sendTopUpdate(Order &order) {
    OutputMessage om;
    om.type = OutputMessageType::TOPOFBOOK;

    if (order.quan == 0) {
        om.side =
            (order.side == OrderSide::BUY) ? OrderSide::SELL : OrderSide::BUY;
    } else {
        om.side = order.side;
    }

    if (om.side == OrderSide::BUY) {
        om.price = highestBid;
        om.quan = getBidQuantityForPrice(highestBid);
    } else {
        om.price = lowestAsk;
        om.quan = getAskQuantityForPrice(lowestAsk);
    }
    csvManager.writeCsvFile(om);
}

void Market::sendAcknowledgement(Order &order) {
    OutputMessage om;
    om.type = OutputMessageType::ACKNOWLEDGEMENT;
    om.side = order.side;
    if (order.side == OrderSide::BUY) {
        om.userIdBuy = order.userId;
        om.userOrderIdBuy = order.userOrderId;
    } else {
        om.userIdSell = order.userId;
        om.userOrderIdSell = order.userOrderId;
    }
    csvManager.writeCsvFile(om);
}

void Market::sendReject(Order &order) {
    OutputMessage om;
    om.type = OutputMessageType::REJECTION;
    om.side = order.side;
    if (order.side == OrderSide::BUY) {
        om.userIdBuy = order.userId;
        om.userOrderIdBuy = order.userOrderId;
    } else {
        om.userIdSell = order.userId;
        om.userOrderIdSell = order.userOrderId;
    }
    csvManager.writeCsvFile(om);
}

void Market::sendTrade(Order &order1, Order &order2) {
    OutputMessage om;
    om.type = OutputMessageType::TRADE;
    if (order1.side == OrderSide::BUY) {
        om.userIdBuy = order1.userId;
        om.userOrderIdBuy = order1.userOrderId;
        om.userIdSell = order2.userId;
        om.userOrderIdSell = order2.userOrderId;
    } else {
        om.userIdBuy = order2.userId;
        om.userOrderIdBuy = order2.userOrderId;
        om.userIdSell = order1.userId;
        om.userOrderIdSell = order1.userOrderId;
    }
    om.quan = order1.quan;
    om.price = order1.price;
    csvManager.writeCsvFile(om);
}

void Market::newOrder(Order &order) {
    if (order.side == OrderSide::BUY) {
        insertBid(order);
    } else {
        insertAsk(order);
    }
    if (isMatchingEnabled) {
        match(order);
        order.accepted = true;
    } else {
        handleCrossBook(order);
        if (!order.accepted) {
            cancelOrder(order);
        }
    }
}

void Market::cancelOrder(Order &order) { removeOrder(order.userOrderId); }

void Market::flushOrders() {
    orders.clear();
    asks.clear();
    bids.clear();
    setLowestAsk(ENDPRICE);
    setHighestAsk(0);
    setLowestBid(ENDPRICE);
    setHighestBid(0);
    insertGenesisOrder();
}

void Market::insertAsk(Order &order) {
    Price *curPrice = &asks[order.price];
    Price *nextPrice = findAsksNextHigherPrice(order.price);
    Price *prevPrice = findAsksNextLowerPrice(order.price);
    setAskExtremes(order.price);
    insertOrder(order, curPrice, nextPrice, prevPrice);
}

void Market::insertBid(Order &order) {
    Price *curPrice = &bids[order.price];
    Price *nextPrice = findBidsNextLowerPrice(order.price);
    Price *prevPrice = findBidsNextHigherPrice(order.price);
    setBidExtremes(order.price);
    insertOrder(order, curPrice, nextPrice, prevPrice);
}

Price *Market::findAsksNextHigherPrice(const uint32_t price) {
    auto it = asks.find(price);
    if (it == asks.end() || std::next(it) == asks.end()) {
        return nullptr;
    }
    return &(++it)->second;
}

Price *Market::findAsksNextLowerPrice(const uint32_t price) {
    auto it = asks.find(price);
    if (it == asks.begin() || std::prev(it) == asks.end()) {
        return nullptr;
    }
    return &(--it)->second;
}

Price *Market::findBidsNextHigherPrice(const uint32_t price) {
    auto it = bids.find(price);
    if (it == bids.end() || std::next(it) == bids.end()) {
        return nullptr;
    }
    return &(++it)->second;
}

Price *Market::findBidsNextLowerPrice(const uint32_t price) {
    auto it = bids.find(price);
    if (it == bids.begin() || std::prev(it) == bids.end()) {
        return nullptr;
    }
    return &(--it)->second;
}

void Market::setAskExtremes(const uint32_t price) {
    if (price > highestAsk) {
        setHighestAsk(price);
    }
    if (price < lowestAsk || lowestAsk == ENDPRICE) {
        setLowestAsk(price);
    }
}

void Market::setBidExtremes(const uint32_t price) {
    if (price > highestBid) {
        setHighestBid(price);
    }
    if (price < lowestBid || lowestBid == ENDPRICE) {
        setLowestBid(price);
    }
}

void Market::setLowestAsk(uint32_t price) { lowestAsk = price; }

void Market::setHighestAsk(uint32_t price) { highestAsk = price; }

void Market::setLowestBid(uint32_t price) { lowestBid = price; }

void Market::setHighestBid(uint32_t price) { highestBid = price; }

void Market::match(Order &order) {
    Price *curPrice =
        (order.side == OrderSide::BUY) ? &asks[lowestAsk] : &bids[highestBid];
    uint32_t sum = 0;
    Order *curOrder = &orders[curPrice->orders.front()];
    while (curOrder->userOrderId != ENDINDEX &&
           priceCanMatch(curOrder, order.price) && sum < order.quan) {
        if (sum + curOrder->quan >= order.quan) {
            sendTrade(order, *curOrder);
            curOrder->quan -= order.quan - sum;
            if (curOrder->quan == 0) {
                removeOrder(curOrder->userOrderId);
            }
            order.quan = 0;
            removeOrder(order.userOrderId);
            return;
        }
        sum += curOrder->quan;
        Order *nextOrder = &orders[curOrder->next];
        removeOrder(curOrder->userOrderId);
        curOrder = nextOrder;
    }
    order.quan -= sum;
    if (order.quan == 0)
        removeOrder(order.userOrderId);
}

void Market::handleCrossBook(Order &order) {
    if (order.side == OrderSide::BUY && order.price >= lowestAsk) {
        order.accepted = false;
        sendReject(order);
    } else if (order.side == OrderSide::SELL && order.price <= highestBid) {
        order.accepted = false;
        sendReject(order);
    } else {
        order.accepted = true;
    }
}

bool Market::priceCanMatch(const Order *curOrder, const uint32_t orderPrice) {
    if (curOrder->side == OrderSide::BUY) {
        return curOrder->price >= orderPrice;
    } else {
        return curOrder->price <= orderPrice;
    }
}

void Market::removeOrder(const uint64_t id) {
    Order *curOrder = &orders[id];
    Order *prevOrder = &orders[curOrder->prev];
    Order *nextOrder = &orders[curOrder->next];
    curOrder->quan = 0;
    if (prevOrder->userOrderId != ENDINDEX) {
        prevOrder->next = curOrder->next;
    }
    if (nextOrder->userOrderId != ENDINDEX) {
        nextOrder->prev = curOrder->prev;
    }
    if (curOrder->side == OrderSide::BUY) {
        bids[curOrder->price].orders.remove(curOrder->userOrderId);
        if (bids[curOrder->price].orders.size() == 0) {
            if (curOrder->price == lowestBid) {
                lowestBid = (prevOrder->userOrderId == ENDINDEX)
                                ? ENDPRICE
                                : prevOrder->price;
            }
            if (curOrder->price == highestBid) {
                highestBid =
                    (nextOrder->userOrderId == ENDINDEX) ? 0 : nextOrder->price;
            }
            bids.erase(curOrder->price);
        }
    } else {
        asks[curOrder->price].orders.remove(curOrder->userOrderId);
        if (asks[curOrder->price].orders.size() == 0) {
            if (curOrder->price == lowestAsk) {
                setLowestAsk((nextOrder->userOrderId == ENDINDEX)
                                 ? ENDPRICE
                                 : nextOrder->price);
            }
            if (curOrder->price == highestAsk) {
                setHighestAsk((prevOrder->userOrderId == ENDINDEX)
                                  ? 0
                                  : prevOrder->price);
            }
            asks.erase(curOrder->price);
        }
    }
}

void Market::insertOrder(Order &order, Price *curPrice, Price *nextPrice,
                         Price *prevPrice) {
    if (curPrice->orders.size() == 0) {
        if (nextPrice == nullptr) {
            order.next = ENDINDEX;
        } else {
            order.next = nextPrice->orders.front();
        }
        if (prevPrice == nullptr) {
            order.prev = ENDINDEX;
        } else {
            order.prev = prevPrice->orders.back();
        }
    } else {
        order.next = orders[curPrice->orders.back()].next;
        order.prev = curPrice->orders.back();
    }
    pushBackOrReplace(order);
    curPrice->orders.push_back(order.userOrderId);
    if (order.next != ENDINDEX) {
        orders[order.next].prev = order.userOrderId;
    }
    if (order.prev != ENDINDEX) {
        orders[order.prev].next = order.userOrderId;
    }
}

void Market::pushBackOrReplace(Order &order) {
    if (order.userOrderId > orders.size()) {
        orders.resize(order.userOrderId);
    }
    if (order.userOrderId == orders.size()) {
        orders.push_back(order);
    } else {
        orders[order.userOrderId] = order;
    }
}

uint32_t Market::getAskQuantityForPrice(const uint32_t price) {
    Order *curOrder = &orders[asks[price].orders.front()];
    uint32_t totQuan = 0;
    while (curOrder->userOrderId != ENDINDEX && curOrder->price == price) {
        totQuan += curOrder->quan;
        curOrder = &orders[curOrder->next];
    }
    return totQuan;
}

uint32_t Market::getBidQuantityForPrice(const uint32_t price) {
    Order *curOrder = &orders[bids[price].orders.front()];
    uint32_t totQuan = 0;
    while (curOrder->userOrderId != ENDINDEX && curOrder->price == price) {
        totQuan += curOrder->quan;
        curOrder = &orders[curOrder->next];
    }
    return totQuan;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ It's really hard to review this without the contents of "Market.hpp" - we're missing the knowledge of CsvManager, Order, OrderType, OrderSide, OutputMessage and much more. \$\endgroup\$ Jan 12 at 8:01
  • \$\begingroup\$ And is uint32_t aliased to std::uint32_t somewhere, or is it something different? \$\endgroup\$ Jan 14 at 9:33

2 Answers 2

2
\$\begingroup\$

Note up front: Generally, it would have been better if you had provided the header file, too, or even combined the two for review. Knowing whether a method is private or whether it is static makes a difference. Also, it would have provided a slightly bigger picture of what the code should do, maybe even a few comments that explain things.

Long Parameter Lists

Can you spot the bug in this code?

Order emptyOrder(0, true, "", ENDINDEX, OrderType::NEW, OrderSide::SELL, 0,
                 0);

I as a casual reader can't, perhaps this even compiles without any diagnostic. As a replacement for these long parameter lists, I'd suggest using the Builder Pattern:

auto emptyOrder = OrderBuilder::instance()
    .setRange(0, ENDINDEX)
    .setType(OrderType::NEW)
    .setSide(OrderSide::SELL)
    .build();

Other languages have named parameter lists to make code clearer.

Unnecessary Deep Nesting

Looking at Market::isTopUpdate() here, I see this pattern:

if (/condition/) {
    /code/
}
return false;

To clarify this, I'd check the inverse of the condition and return false right away. To me as a reader, it allows me to split the function in two halves. After the check and subsequent return, I know that the condition is not met for the rest of function. I know that this means you will have two return false code paths, which is an argument against an early return here.

Further, in the same function, there is a case where an else is placed after a return. This else formally (not by indentation) increases nesting and it can be removed because it doesn't have any effect here.

Const Correctness

In bool Market::isTopUpdate(Order &order), neither order nor this are written to, so both should be made const.

Use Of Pointers vs. References

In Market::priceCanMatch(const Order *curOrder, const uint32_t orderPrice), the first parameter should be a reference. The pointer here kind-of implies that it can be null, but that's not really the case.

Buy Or Sell

Lots of the code there makes a switch depending on whether the order is of type SELL or BUY. On a cursory look, it even seems that one operation with an order can make repeated checks for the type due to nested calls. I think that the code could become clearer if the code paths for the two types were completely separated, maybe even by using two separate types.

Inconsistent Formatting

Don't put simple forwarding functions in one line. This neither improves readability nor performance of these functions. Those functions are just not special enough, so resist the temptation to make an exception here.

Use Of uint32_t

Why? Why not go with plain unsigned instead? Using this type implies "I need exactly 32 bits" but I don't see a reason here.

\$\endgroup\$
1
\$\begingroup\$

Your style of naming member variables means that they are quite similar to the style of you function names and local variables, have you considered using the m_ prefix?

Personally I don't like the java style of braces in C++) I don't like it in Java either TBH). Always stick braces on their own line, it spaces the code better and makes it look less cramped.

Same applies to the initializer, I would place each on a different line. You have an if statement which has an && in. Again this is just me but I would put the && at the start of the second line rather than the end of the first. In my simple mind it makes it clearer.

Finally Your code is really clear to read, but I have no idea what it is doing and why because there are no comments anywhere. It may be clear to you today, but will it still be clear when you have to fix a bug in a years time?

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Thank you for having a look at the code. I used the LLVM code style, and therefore the first three paragraphs are determined by that code style. The code was for a code assignment where 48h were given, there was no time to add too many comments, I agree with your point, I am leaning too much to the advise of the book Clean Code where the code should be the documentation, without comments. I received feedback about poor readability, which interestingly you seem to disagree with. Then the lack of comments is most likely the reason. \$\endgroup\$
    – Dweezahr
    Apr 22, 2022 at 17:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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