Base class design

[o3hC5Di1]

Hi everyone,

As I'm still quite new to C++, I'm still unsure at times about what the best practice is to implement things.

My current project is writing a library, mostly for personal use.
It required a lot of "crudl" operations, so I decided that would make a good abstract base class:

template <typename ItemType, typename ListType>
class crudl_base
{
    virtual operation_status create(ItemType&)=0;
    virtual operation_status read(ItemType&)=0;
    virtual operation_status update(ItemType&)=0;
    virtual operation_status destroy(ItemType&)=0;
    virtual operation_status list(ListType&)=0;
    //...
};

//Example subclass:
class person_crudl : crudl_base<person, personlist>
{
    operation_status create(person& p) { //write to database etc. }
    //...
};

//Example usage:
person_crudl pc;
person p;
p.name = "foo"
operation_status status = pc.create(pc);

if (status == operation_status::SUCCESS)
    std::cout << "Person created";
if (status == operation_status::FAIL_DB
    std::cout << "Database failure";
if (status == operation_status::FAIL_UNIQUE)
    std::cout << "Person name must be unique.";

Now, I have 2 main questions, which raise some related questions:

1. Is this a proper design of the crudl_base class? More in particular, is it good practice for the functions to just take an ItemType object? This is a uniform interface, but what if a property which is needed by the function was not set? (i.e. the above example, what if p.name was not set before passing it in?). Should there be additional functions taking int (for id), and strings (for a secondary index)?

2. The error handling, as you can see operation_status is a class used to return status codes from functions. Initially I preferred this over exceptions, because I don't require stack unwinding and I read several arguments advocating to use them only in "exceptional" circumstances. They do make it clearer to the user though which functions can throw which exceptions (using throw lists) and class-specific exceptions are more clear than class-specific status-codes. Just FYI, the operation_status are actual objects, not enums.

Sorry for the long question, but I do hope someone will be so kind as to give their advice. I'm really still lacking insight into best practices and such, hopefully that will come with time.

Thanks very much in advance.

All the best,
NwN

Edit: Rephrased the question

[o3hC5Di1]

*polite bump* - anyone? :)

Thanks,
NwN

[o3hC5Di1]

*polite bump #2* - nobody?

-N

[JLBorges]

1. class crudl_base ; Why do you need this base class with virtual functions?
When it is already compile-time polymorphic wrt ItemType and ListType


> Should there be additional functions taking int (for id), and strings (for a secondary index)?

An Xml string, perhaps

Something like:

template <typename ItemType, typename ListType>
class crudl_base
{
    operation_status create( const std::string& xml_data, ItemType&) ;
    operation_status read( const std::string& key_xml, ItemType&);
    // etc..
};

> The error handling ,... return status codes from functions.

The obvious problems with this is that errors may not get handled, (sometimes just because there is no good way to discriminate between different errors), proper clean up may not take place, RAII goes for a toss, and you make it incredibly difficult for me write robust code for:

class my_class
{
      // ...

      private: your_class object ;

     // ...
};

> They do make it clearer to the user though which functions can throw which exceptions (using throw lists) ...

Dynamic exception specifiers are deprecated.
Static exception specifiers are not: http://www.stroustrup.com/C++11FAQ.html#noexcept

[o3hC5Di1]

Hi JLBorges,

Thanks a lot for your input, very informative as always.

1) The base class actually provides more functionality than this (database connection for instance). The other functions serve as a contract for derived classes, it makes sense that crudl classes implement crudl operations. I made it a template because a class person_crudl would work with person objects, whilst a class animal_crudl would work on animal objects. Does that make sense or am I missing something?

2) Thanks for the suggestion on the XML string. I hadn't thought of that as an option. Did you mean to say that the data inputted to these functions should be XML, rather than an object with "random" member set? Would you be so kind to elaborate on the benefits on that, or is it just as a future precaution should anyone want to use XML with the library?

3) I see where you're coming from with the error handling safety issues. The errors I refer to are not "program errors" however, they are more like status reports of the function. The function can report it was successful, or that it failed because the persons name was too long, or because there was a database connection error, etc. Of course, you have a good point about the fact that these are easy to ignore. It seems counter intuitive for me to make these into "proper" error situations, however, I suppose having a database failure would result in not having the resource available to display a record to the user, so that would constitute a situation that is "exceptional".

Just some more of my thoughts, I'm not trying to debate you, just explain my motives a bit further because I need to learn to immediately think in the right way when it comes to design.

Any more input v\would be very much appreciated.

All the best and thanks again,
NwN

Edit: Thanks also for the heads up on dynamic exception specifiers, I wasn't ware of that.

[JLBorges]

> The base class actually provides more functionality than this (database connection for instance).
> The other functions serve as a contract for derived classes,
> it makes sense that crudl classes implement crudl operations. I
> made it a template because a class person_crudl would work with person objects,
> whilst a class animal_crudl would work on animal objects

My question was: these crudl classes are designed to be compile-time polymorphic.
Then why does the base class require virtual functions?

template < typename ListType > struct crudl_base
{
    // common operations 
    // database connection etc.
    // read and write xml data  
    // etc...
    // none of them are virtual
    // they could perhaps be static?
};

class person_crudl : crudl_base<personlist>
{
    /* static? */ operation_status create( /* ... */ ) { /* ... */ }
    // etc...
};

template< typename T, typename L, template <typename,typename> class CRUDL > 
void foo( C<T,L>& crudl, const T& object )
{
    crudl.create( object /* ... */ ) ; // compile time polymorphic
};

> Did you mean to say that the data inputted to these functions should be XML?

Yes.

XML is text based and is not implementation-specific. An integer in XML is a number; size, representation, endianness do not come into play. Text data can be portably transferred over the network and through firewalls.

XML is easy to version; the database schema and the C++ class are decoupled, and one can be modified without affecting the other.

XM is structured data can be stored and retrieved from text files, relational or object databases, spreadsheets etc., can be easily displayed on a web page, can be put in a SOAP envelope etc.

XML may not be appropriate if the data set is huge, but I presumed that that would not be the case here. If we write an overload of :

template < ITEM_TYPE > std::string to_xml( const ITEM_TYPE& object ) ;
template < ITEM_TYPE > ITEM_TYPE from_xml( const std::string& xml ) ;

for each type (animal, person etc),
then the bulk of basic crudl operations can be factored into common reusable code.



> The errors I refer to are not "program errors" however, they are more like status reports of the function.

Returning a status code is fine, then. As long as the no other information other than the the status code would be required to handle the error.


> I suppose having a database failure would result in not having the resource available to display a record to the user, so that would constitute a situation that is "exceptional".

Yes. Or else, the problem of discriminating between errors would remain.

enum status { /* ... */ NETWORK_ERROR, DATABASE_ACESS_FAILURE /* ... */ };

If a remote database is not accessible because the network is down, what status code would you return? Creating a new status code when this scenario arises would beak a lot of existing error handling code.

With exceptions:

struct network_error : public virtual std::runtime_error { /* ... */ } ;
struct db_access_error : public virtual std::runtime_error { /* ... */ } ;

We can discriminate further by adding derived classes as and when the need arises.

void foo( /* ... */ )
{
    // ...

    struct network_db_access_error : public virtual network_error, public virtual db_access_error { /* ... */ } ;
    throw network_db_access_error( /* ... */ ) ;

    // ...    
}

[o3hC5Di1]

Very valuable feedback, I hope you will allow me to ask a few more questions.
I probably should have probably created two separate topics for these questions, I'll need to remember that for the future.

I'm starting to get a more full understanding of the use of the virtual keyword now thanks to your mentioning "compile time polymorphism". I guess I just thought you made a member funciton virtual if you want to be able to override it in the derived class, which is only a part of it. You meant to say that it does not make sense to use them here, because the template instance created at compile time leaves no ambiguity about which "create()" function to call, correct?

I'm very grateful for that pointer to the use of XML here, I wouldn't have thought of that otherwise. I'm actually using a web framework to build this with, so data for these functions would be mostly supplied HTTP POST/GET. However, in the case of AJAX/comet XML would be useful too. 2 further questions:

1) Are there any reasons not to use JSON over XML (the web framework I use has good JSON support)?

2) You said "...then the bulk of basic crudl operations can be factored into common reusable code.", so I am assuming that the XML should contain query data then, like this:

<personquery>
    <query>insert into tbl_persons (name) values (?) where id=?</query>
    <person>
        <name>John Doe</name>
        <id>34</id>
    </person>
</personquery>

I have indeed started to recognise the maintainability problems of these "status codes", as well as having no guarantee that they will be handled. I believe what's confusing me is that there can be both exceptions (database error) and "minor problems" like no persons being found in the database. I think I may just use all exceptions, in which case the crudl operations can actually return a bool or Person object where appropriate:

template <typename ItemType, typename ListType>
class crudl_base
{
    void create(const std::string &XMLstring);
    ItemType read(const std::string &XMLstring);
    void update(const std::string &XMLstring);
    void destroy(const std::string &XMLstring);
    ListType list(const std::string &XMLstring);
    //...
};

Would it be safe for a caller to assume that the void functions succeed if they don't throw an exception then?

My appologies for all the questions, but as I mentioned I really lack industry experience, as this is a pretty basic part of the library, I really want to get this right.

Thanks once again.

All the best,
NwN

[JLBorges]

> You meant to say that it does not make sense to use them here, because
> the template instance created at compile time leaves no ambiguity about which "create()" function to call, correct?

Yes. The virtual mechanism is useful if and only if you lose type information via the conversion of a reference or a pointer. That won't happen when template code is instantiated and the type involved is a template parameter.


> Are there any reasons not to use JSON over XML (the web framework I use has good JSON support)?

The first requirement for JSON is that you must have control over what runs on both the client and the server; while XML is universally supported, JSON isn't. For instance, there may be a web service which uses SOAP and not JSON.

JSON is easier to parse, and IMHO, is more readable than XML. If the data is not very large and consists of integers, dates etc., JSON is clearly faster - it transmits much fewer number of bytes over the wire. If you have Javascript, programming with JSON is far easier - Javascript understands JSON


> You said "...then the bulk of basic crudl operations can be factored into common reusable code.",
> so I am assuming that the XML should contain query data then, like this:

Yes, that was the general idea. Though you would have to design the XML schema with some care.


> I think I may just use all exceptions, in which case the crudl operations
> can actually return a bool or Person object where appropriate:

Yes. The code would be a lot cleaner too.


> Would it be safe for a caller to assume that the void functions succeed if they don't throw an exception then?

If that is the way you have designed it.

While exceptions are clearly better for ItemType read(const std::string &XMLstring);,
for void update(const std::string &XMLstring);,
bool update(const std::string &XMLstring); is a possibility.

[JLBorges]

> I really lack industry experience, as this is a pretty basic part of the library, I really want to get this right.

Perhaps you should start a new thread, once you have worked out a basic outline. No one else seems to be interested in this one.

The design would benefit if more ideas, from people with diverse perspectives, poured in.

[o3hC5Di1]

Thank you very very much JLBorges for that, having your input was invaluable, I will ponder this for a few days and do as you say, create a new thread.

All the best,
NwN