C++

Forum

 
Code review and design suggestion needed.

vincegata (112)
Hi,

This is a prototype from a large application that shows a point. It's an order management system that 1) creates an order 2) serializes and deserializes an order.

My concern is about order type, specifically I have two classes to serve order types. First, I have enum class order_status_class that's used for creating orders and it's convenient to use. However to serialize and deserialize an order I have to have another class, order_status_names_class.

Can someone suggest an improvement over the existing design. I'd like to have only one data structure to serve creating orders and serialization/deserialization of orders, that is convenient to use and offers strong type checking as enum class does.

Thank you.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
 
#include <iostream>

enum class order_status_class : unsigned {neworder=0, confirmed, filled, shipped};

class order_status_names_class
{
public:
      std::string get_status_name(const order_status_class& ot);
};

std::string order_status_names_class::get_status_name(const order_status_class& ot)
{
      std::string status_name;
      if (order_status_class::neworder == ot)
            status_name = "neworder";
      else if (order_status_class::confirmed == ot)
            status_name = "confirmed";
      else if (order_status_class::filled == ot)
            status_name = "filled";
      else if (order_status_class::shipped == ot)
            status_name = "shipped";
      return status_name;
}

class order_class
{
public:
      void set_order_id(const unsigned param) {order_id=param;}
      void set_order_status(const order_status_class param) {order_status = param;}
      unsigned get_order_id() const {return order_id;}
      order_status_class get_order_status() const {return order_status;}
private:
      unsigned order_id = 0;
      order_status_class order_status= order_status_class::neworder;
};

class create_order_class
{
public:
      int create_order(unsigned, order_status_class, order_class&);
};

int create_order_class::create_order(unsigned p_order_id, order_status_class p_order_status, order_class& oc)
{
      oc.set_order_id(p_order_id);
      oc.set_order_status(p_order_status);
      return 0;
}

class serialize_order_class
{
public:
      // Serializes order object into a string: "order_id=2014001;ordet_status=filled;"
      int serialize(const order_class& oc, std::string& message);
      // Parses order message and creates an order object. Not implemented.
      int deserialize(const std::string& message, order_class& oc);

private:
      std::string order_id_tag = "order_id=";
      std::string order_status_tag = "order_status=";
};

int serialize_order_class::serialize(const order_class& order, std::string& message)
{
      order_status_names_class status_names;
      message = "";
      message =
            order_id_tag + std::to_string(order.get_order_id()) + ";" +
            order_status_tag + status_names.get_status_name(order.get_order_status()) + ";";
      return 0;
}

int main()
{
      order_class order;
      create_order_class co;
      co.create_order(2014001, order_status_class::confirmed, order);

      serialize_order_class ser;
      std::string order_msg;
      ser.serialize(order, order_msg);
      std::cout << order_msg << std::endl;
}
Catfish666 (666)
The first thing I notice is that you're a bit too class-happy.
Are you sure you cannot simplify the code, and define less classes?

A minor observation, I think appending "_class" to class names is a bit overkill.
The traditional way to make class names stand out is to use CamelCase style.

The first thing I'd to in order to simplify the code, would be to use a map instead of a dedicated function for the name. Of course there must exist much better approaches, but this is what popped up in mind first.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
#include <iostream>
#include <map>
#include <string>

enum class OrderStatus
{
    NewOrder,
    Confirmed,
    Shipped,
    Filled
};

const std::map<OrderStatus, std::string> order_names {
    {OrderStatus::NewOrder,     "New Order."},
    {OrderStatus::Confirmed,    "Confirmed."},
    {OrderStatus::Shipped,      "Shipped."},
    {OrderStatus::Filled,       "Filled."}
};

int main()
{
    std::cout << order_names.at(OrderStatus::Confirmed) << std::endl;
}
 
Confirmed.


Right now I can't think of any immediate improvement, because I'm not 100% sure what you want the program to actually do.

My personal advice is: try to simplify your code. Keep it short and simple. Don't write tons of classes and functions just to juggle with statuses.
Disch (13742)
use CamelCase style.


<technicality>

Caps is not camel case.

thisIsCamelCase

called camel case because the "humps" (caps) are in the middle, like on a camel.
Catfish666 (666)
@ Disch: not to derail the thread but at least Wikipedia recognizes ThisStyle as a camelCase variation.

http://en.wikipedia.org/wiki/CamelCase

And to get back on topic: does anyone have a better suggestion than using a map to get the status names?

For some reason it bothers me a little. Not too much though, if it throws a std::out_of_range exception, you deserve it.
JLBorges (13770)
> The traditional way to make class names stand out is to use CamelCase style.

The traditional Java / Microsoft way.

The traditional C and C++ way is the way shown by the standard C++ library:
std::recursive_timed_mutex, std::priority_queue, std::basic_ios etc.

And if the names are common enough to warrant emphasizing that it is a type, then suffix with _type or _t:
std::string::size_type, std::size_t, std::regex_constants::match_flag_type etc.

Boost is quite traditional.

IMHO.


> does anyone have a better suggestion than using a map to get the status names?

I wouldn't use a map to look up four entries.

But, more fundamentally: I wouldn't pollute the enclosing namespace with order status.
An unscoped enum at class scope is preferable to a scoped enum at namespace scope.

I would do something like this:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
 
#include <iostream>
#include <string>
#include <cassert>
#include <stdexcept>

struct order
{
    enum class status : unsigned int { NEW_ORDER = 0, CONFIRMED = 1, SHIPPED = 2, FILLED = 3 };

    static std::string to_string( status s ) ;
    
    // ...
};

inline std::ostream& operator<< ( std::ostream& stm, order::status s )
{ return stm << order::to_string(s) ; }


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
 
// ------------- cpp file -----------------

namespace
{
    constexpr const char* const order_status_string[] =
    { "NEW_ORDER", "CONFIRMED", "SHIPPED", "FILLED" } ;

    inline bool initialized( order::status s ) { return s <= order::status::FILLED ; }
}

std::string order::to_string( order::status s )
{
    assert( "order::status has been initialized" && initialized(s) ) ;
    static const std::string prefix = "order::" ;
    if( initialized(s) ) return prefix + order_status_string[ std::size_t(s) ] ;
    throw std::domain_error( "from 'order::to_string(order::status)': uninitialized order::status" ) ;
}

int main()
{
    try
    {
        std::cout << order::status::CONFIRMED << std::endl ;
        order::status uninitialized ;
        std::cout << uninitialized << '\n' ;
    }
    catch( const std::exception& e )
    {
        std::cerr << "*** error: " << e.what() << '\n' ;
    }
}

http://coliru.stacked-crooked.com/a/f5c079c5ce521744
vincegata (112)
Thanks for the responses, I'll go the easiest route and use enum class and a map.

Topic archived. No new replies allowed.