Class Uri: review

Hi Peter,

I’ve checked the Uri class and below are my ideas/comments to the current code. I must say that the idea with the Uri class was really briliant! I’m sure that this class will simplify the TrackerRequestWriter because nearly everything I’ve been doing there is now in the Uri class (and there are also no UTF-8 conversions). In addition, as you’ve suggested, we can use it in other modules.

As for these UTF-8 conversions - I’d personally first make our Uri class using only ASCII characters (because this is what we currently need). I haven’t studied appropriate RFCs very deeply, though.

Petr

——————————————————————————–

\todo Validation? Setting the individual fields or complete URI to Null is invalid data is passed in?

What about this way: If invalid data are passed “as one piece” then the whole Uri object will be “set to Null”. If invalid data are passed only to some setter function then simply this setter function will do nothing.


void setUnencoded (const QByteArray &);
void setEncoded (const QByteArray &);

I’d maybe change the names into setAsUnencoded and setAsEncoded because I first thought that these two functions set some kind of a state (encoded/unencoded), but that’s only my impression :).


bool Uri::shouldEncode (char ch, const QByteArray &charsToExclude) const
{
...

I’d change the implementation of this function to this one:


if (ch >= 'a' && ch = 'A' && ch = '0' && ch

or is there some reason to use int literals instead of char literals?

\todo Should we better use the following regular expression from RFC 3986 to parse the URI?

I’d give it a try (QRegExp).


void appendQuery (const QByteArray &bytes);
void appendQueryItem (const QByteArray &key, const QByteArray &value);

Maybe we should add there some input validation (e.g. if bytes/key/value is valid/not empty), but that’s related to the validation “question”.


#ifndef UTILS_URI_H
#define UTILS_URI_H
#endif // UTILS_URI_H

Change it to UTILS__URI_H to keep consistent naming.


void ensureNotNull (QByteArray &bytes);

I’d made this function either const or static.

// \note Maybe class Uri could be moved to a subpackage of Utils, or maybe somewhere in Protocols?

I agree to move it “somewhere” in Protocols, but don’t know where :).

// \todo move the typedefs to Uri?

typedef QPair QueryItem;
typedef QList QueryItemList;

Agreed.

\todo Should we implement URI-reference?

Maybe, but I think that we won’t need them in the near future (I won’t need them in the TrackerRequestWriter).

Would you like to post a relpy?


This post starts a thread.
Follow-ups:
Re: Class Uri: review
Hi Petr, Thanks for the review! Comments are inline: As for these UTF-8 conversions - I’d personally first make our Uri class using only ASCII characters (because this is what we currently (more...)