Documenting interfaces [closed]

Closed. This question is opinion-based. It is not currently accepting answers.

---

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 5 years ago.

Improve this question

I was wondering about how I should document an interface in my application, IResource. Since I'm coding an engine and not a library, I figured the documentation should give guidelines about how the implementations of the interface should be written; is that okay?

Also, could you please take a look at my interface and tell me if the comments are clear enough?

/**
    Interface that should be implemented by all resources. Implementing
    this interface is necessary for compatibility with the ResourceManager
    class template.

    \note Documentation of this interface includes guidelines on how
    implementations should be written.

    \see ResourceManager
                                                                              */
class IResource
{
  public:
    /**
        Loads resource data from a file. If data is already loaded,
        the function should return immediately.

        \throw std::exception Should throw on any failure to load the
        resource. If the resource is already loaded, don't throw, just
        return (as previously indicated).

        \note Access to this function should also be provided directly
        from a constructor. That constructor should catch any exceptions
        and throw them further to its caller.
                                                                              */
    virtual void loadFromFile(const std::string& file) = 0 ;
    /**
        All general guidelines from loadFromFile() also apply to this
        function. Additionally, the resource should not take possession of
        the buffer; the buffer should be safe to delete after loading.
                                                                              */
    virtual void loadFromMemory(const char* buffer, std::size_t size) = 0;
    /**
        Frees the data currently held by the resource object. Should
        return immeditelly if no data is loaded.
                                                                              */
    virtual void free() = 0;
    virtual bool isLoaded() const = 0;
};

---

Edit: Opened a related discussion.

Following mainly the conversation in the comments section of Johann Gerell's answer, I opened a rather lengthy thread over at programmers.stackexchange. You can check i开发者_StackOverflow中文版t out here:

> Single-responsibility and custom data types

---

You've documented the intent well, and that is a very good start.

A few things missing:

• You didn't document the arguments. They are self-obvious, but I can be a bit pedantic (ditto for doxygen).
• What does isLoaded do?
• Turn off the inherited documentation capability in doxygen. While your comments are valid for the interface class, they are not valid for some class that implements the interface.

---

Your comments should specify the contract between the user and implementer; it's not necessary to focus on the perspective of either.

The guidance about taking ownership of the buffer doesn't sound like a recommendation, it sounds like a definite requirement. Use "will": All access to the buffer will be performed before this function returns; the buffer may be safely freed at that time. Similarly: If the resource has already been loaded, this function returns immediately. It's a requirement, not a recommendation.

Parameters should be documented fully. Can the file name be a relative path? What happens if the file does not exist? Is unreadable due to permissions?

You should describe which exceptions may be thrown. Catching and rethrowing exceptions is just bad design.

Using a const char* for non-character data is bad design, use const void* instead. Document that the size is in bytes (or if not, what the units are). Is the data in any common format, or is it unique to the subclass?

Keep per-function documentation specific to that function. Information about interactions between multiple functions, or advice on constructor behavior of derived classes, belongs at the class level.

Use correct spelling, misspelled words distract from the content.

---

Considering the code behavior of your listing instead of the doc parts, I think you should re-think the interface a bit (the other comments on the docs are good enough) in the light of the Single Responsibility Principle and the Liskov Substitution Principle. Right now, the name IResource implies that the type has resource-ish behavior, but I would argue that it has no such behavior at all. Think about how you'd use this type. Most logically you would pass around reference or pointer to an IResource. You would then need to check if it's loaded or not with isLoaded before you do anything with it. Always.

What if you instead separated the two responsibilities of loading a resource versus being a resource:

class IResource
{
public:
    virtual SomeCommonResourceBehavior(...) = 0;

    virtual ~IResource() {}
};

class IResourceFactory
{
public:
    virtual std::unique_ptr<IResource> CreateFromFile(...) = 0;
    virtual std::unique_ptr<IResource> CreateFromMemory(...) = 0;

    virtual ~IResourceFactory() {}
};

This way, when you see a reference or non-null pointer to an IResource anywhere in your code, you know that it's already created.

Also, if you cannot identify any SomeCommonResourceBehavior in IResource, then you've likely thought a bit wrong about your design.

EDIT: If you live in a pre-C++0x land, then boost::unique_ptr<> is an alternative in the factory. If boost is not an alternative, the std::auto_ptr<> is better than a raw pointer.