Implementing HTTP data range representation class

Question

I need a class representing a parsed Range HTTP header value. I've done it like this:

namespace net {

namespace http {

    class DataRange {
    public:
        static const uint64_t UnspecifiedBytePos;
        static const DataRange Full;

        explicit DataRange(uint64_t firstBytePos, uint64_t lastBytePos);
        DataRange(const DataRange& other);

        uint64_t FirstBytePos() const;
        uint64_t LastBytePos() const;

        uint64_t Size() const;

        bool HasUnspecifiedBytePos() const;

        bool operator==(const DataRange& other) const;
        bool operator!=(const DataRange& other) const;

    private:
        uint64_t firstBytePos_;
        uint64_t lastBytePos_;
    };

    typedef std::list<DataRange> DataRanges;

    DataRange SpecifyDataRangeBytePos(const DataRange& dataRange, uint64_t dataSize);


} // namespace http


} // namespace net

I don't show the implementation here since it's quite simple. In this implementation, DataRange may have unspecified borders, representing Range headers like this one:

Range: -1234

To turn DataRange to one with specified borders, I've made a function SpecifyDataRangeBytePos:

DataRange SpecifyDataRangeBytePos(const DataRange& dataRange, uint64_t dataSize) {

    uint64_t specifiedFirstBytePos = dataRange.FirstBytePos();

    if ((dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos) &&
            (dataRange.LastBytePos() != DataRange::UnspecifiedBytePos)) {

        uint64_t rangeUpperBorder = std::min(dataSize, dataRange.LastBytePos());
        specifiedFirstBytePos = dataSize - rangeUpperBorder;
    }

    if ((dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos) &&
            (dataRange.LastBytePos() == DataRange::UnspecifiedBytePos)) {

        specifiedFirstBytePos = 0;
    }

    uint64_t specifiedLastBytePos = dataRange.LastBytePos();

    if ((dataRange.LastBytePos() != DataRange::UnspecifiedBytePos) &&
            (dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos)) {

        specifiedLastBytePos = (dataSize > 0) ? (dataSize - 1) : dataSize;
    }

    if ((dataRange.LastBytePos() == DataRange::UnspecifiedBytePos) &&
            (dataRange.FirstBytePos() != DataRange::UnspecifiedBytePos)) {

        specifiedLastBytePos = (dataSize > 0) ? (dataSize - 1) : dataSize;
    }

    if ((dataRange.LastBytePos() == DataRange::UnspecifiedBytePos) &&
            (dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos)) {

        specifiedLastBytePos = (dataSize > 0) ? (dataSize - 1) : dataSize;
    }

    DataRange result(specifiedFirstBytePos, specifiedLastBytePos);
    assert(!result.HasUnspecifiedBytePos());

    return result;
}

There are plenty of ifs, which seems very awkward, but I cannot do better. Any suggestions?

Answer 1

Starting from bottom. If you have too many ifs and you want to get rid of them then you should start to name things (as you already did for HasUnspecifiedBytePos()).

This expression dataRange.FirstBytePos() == DataRange::UnspecifiedBytePos is saying IsFirstBytePosSpecified (or HasFirstBytePos). Just add two simple functions to your DateRange class:

bool HasFirstBytePos() const {
    return FirstBytePos() == UnspecifiedBytePos;
}

Now you can rewrite your first if like this:

if (!dataRange.HasFirstBytePos() && dataRange.HasLastBytePos()) {
    uint64_t rangeUpperBorder = std::min(dataSize, dataRange.LastBytePos());
    specifiedFirstBytePos = dataSize - rangeUpperBorder;
}

It's still there but it's shorter and easier to understand at first sight. The same should be applied to all the other checks. Let's do a further step in this direction and refactor out code to calculate specifiedFirstBytePos into its own method (this may stay here or in DataRange class, I don't see how and where you use it).

uint64_t ResolveFirstBytePos(const DataRange& dataRange, uint64_t dataSize) const {
    if (dataRange.HasFirstBytePos())
        return dataRange.FirstBytePos();

    if (dataRange.HasLastBytePos()) {
        uint64_t rangeUpperBorder = std::min(dataSize, dataRange.LastBytePos());
        return dataSize - rangeUpperBorder;
    }

    return 0;
}

Everything together:

DataRange SpecifyDataRangeBytePos(const DataRange& dataRange, uint64_t dataSize) {
    DataRange result(
        ResolveFirstBytePos(dataRange, dataSize),
        ResolveLastBytePos(dataRange, dataSize));

    assert(!result.HasUnspecifiedBytePos());

    return result;
}

Note that you may move assertion inside your constructor (I don't know if it's a valid condition then here I kept original code).

When you will read this code in future you won't need to deeply understand how it is doing things but in few lines you will understand what it is doing.

More things:

• explicit on a constructor with two parameters is useless (no default value for lastBytePos), you may simply drop it.

• I'd pick better names for FirstBytePos and LastBytePos. Without reading all your (unavailable) code I may just guess it's position (in bytes) within input data. If type is obvious (bytes) then you may simply use BeginInData and EndInData but I'd need more context to suggest anything meaningful. (BytePos comes from domain - its named like this directly in RFC - then it's a good name to use)

• IMO HasUnspecifiedBytePos() expresses more an implementation detail (position, in bytes, is unspecified) than a behavior/state/property of your object. It should be renamed to something from domain, for example IsNull().

• Your constructor has dataSize parameter but you pass LastBytePos() value. dataSize seems to indicate this parameter is an offset from firstBytePos. If it's not an offset then you should rename it, if it is then code is broken (when firstBytePos > 0)