Hi Petr,

Btw, what is your opinion about the TrackerRequestWriter::getPathForHttpHeader() function and the \todo there?

Sorry I’ve overseen that! I actually only checked the changeset and not all the source code. Now that I looked at TrackerRequestWriter in more detail I’m thinking whether or not it would be a good idea to make this class a static helper. So instead of creating a TrackerRequestWriter object and then calling requestHeader on it, just do

RequestHeader header = TrackerRequestWriter::makeRequest(announceUrl, request)

Which makes me think, couldn’t we put announceUrl inside request too?

Which makes me think, couldn’t we get rid of TrackerRequestWriter and use a static member of TrackerRequest like toRequestHeader(). But I’m not really sure about that, especially after reading an interview with Bjarne Stroustrup (and more specifically the section “Designing Simple Interfaces”). Our case could be considered a different one though because TrackerRequest is not such a generic thing as Date but I love the minimalistic approach to design. What do you think?

Back to your \todo question, you actually need only part of the whole encoded URI (path and query). QUrl solves that nicely by passing flags to toString() which could be e.g. RemoveScheme | RemoveAuthority. I think we could extend Uri::quoted() and Uri::unquoted() to take such flags indicating what should be included and what ignored. How does this sound?

Another thing is in TrackerRequestWriter::requestHeader(). I think setting “Host” better be done in a central location and not by each client that wants to send a HTTP request. I feel like this could be responsibility of HttpRequestSession (which is not yet implementeted but could be mocked). All that TrackerRequestWriter actually needs to “write” is the URI. I imagine code like that:

Uri trackerRequestUri = TrackerRequestWriter::makeRequest (trackerRequest);
httpRequestSession->get (trackerRequestUri);

I imagine get() creating the RequestHeader for you (so you don’t need to do anything with RequestHeader in TrackerRequestSession), set the “Host” header, convert the header to a QByteArray() and send it to the HTTP server.

If we want to pass some specific headers to get() we could use an overload like:

get (const Uri &requestUri, const Header &extraHeaders);

get() would then append the fields from extraHeaders to the RequestHeader object.

And I was writing that I thought that possibly TrackerRequestWriter could be merged into TrackerRequestSession. I think that’s even better object design - passing a TrackerRequest object to TrackerRequestSession, which internally converts it to Uri and passes it to HttpRequestSession.

I too like this variant better:

Q_ASSERT (false && “peerInfo does not contain peer ID!”);

the other is more obscure and a novice may wonder what kind of magic it exploits :-). Someone (I think Donald Knuth) has said that it is easy to write code that machines understand but it is difficult to write code that other people understand. Quite true actually :-)

Regards,

Peter