Back-end: distinguish between creation and opening of entities

[stoewer]

Currently the back end classes (such as BlockHDF5) have constructors used for both, creation and opening of entities. It would be nicer to used constructors just for opening entities and create them using a static member function create.

This is also related to #305

[stoewer]

I just looked at the current implementation of our createSomething() methods in the back-end. A typical example looks like this:

shared_ptr<base::IBlock> FileHDF5::createBlock(const string &name, const string &type) {
    if (name.empty()) {
        throw EmptyString("Trying to create Block with empty name!");
    }
    if (hasBlock(name)) {
        throw DuplicateName("Block with the given name already exists!");
    }
    string id = util::createId();

    Group group = data.openGroup(name, true);
    return make_shared<BlockHDF5>(file(), group, id, type, name);
}

There are two things I'd like to highlight here:

1. The check if(name.empty()) is also implemented in the ctor of NamedEntityHDF5 which is used for creation of entities -> the ctor throws the same exception.
2. We do everything to prevent the exceptions thrown by ctors because we do not handle them in our createSomething() methods (see last line in the listing above).

I therefore suggest the following changes:

1. Implement a static member function init or init_group for all back-end entities. Those functions initialize a newly created group before it is passed to a ctor. They should do all checks and throw exceptions if necessary.
2. Make ctors exception free.
3. Implement all createSomething() methods like this:

shared_ptr<base::IBlock> FileHDF5::createBlock(const string &name, const string &type) {
    if (hasBlock(name)) {
        throw DuplicateName("Block with the given name already exists!");
    }

    Group group = data.openGroup(name, true);

    try {
        BlockHDF5::init(group, util::createId(), type, name)
        return make_shared<BlockHDF5>(file(), group);
    } catch(exception e) {
        data.removeGroup(name);
        throw e;
    }
}

[jgrewe]

Would you then do all the subgroup creation (e.g. for features in Tag or dimensions in DataArray etc) do in the init method? Why delegation to the init method? Wouldn't it be enough to do that in the create method?

[stoewer]

Would you then do all the subgroup creation (e.g. for features in Tag or dimensions in DataArray etc) do in the init method?

Yes, especially everything that can go wrong.

Why delegation to the init method? Wouldn't it be enough to do that in the create method?

To reduce code duplication I would like to call the upper class init method e.g. in BlockHDF5::init.

[stoewer]

Some checks can also move to the front end e.g.

if (hasBlock(name)) {
    throw DuplicateName("Block with the given name already exists!");
}

This would simplify the implementations of back-ends but would lead to some redundant checks within the createSomething() methods in the front-end: almost every create method would start with:

if (name.empty()) {
    throw EmptyString("Trying to create Block with empty name!");
}

[jgrewe]

Duplication in the case that there are several create methods?

D'accord, the checks should go to the front end (EmptyString as well).

[stoewer]

Duplication in the case that there are several create methods?

No, but each init method of an entity that inherits from e.g. NamedEntity has to initialize the name and id. Thus, this can be implemented in NamedEntity::init which can be used in the init methods of its sub classes.