Hi Peter,

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)

Because we’re using Uri for creating URLs, there won’t be a problem to refactor TrackerRequestWriter to behave just as a static helper - it will also save a line or two for a user when creating requests.

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

Yes, I think that putting announceUrl inside request may be better because these two pieces of data are passed together. My original idea was to split them because we would be able to send a single request to various trackers without needing creating a request for each announceUrl, but we can create a separate request for each announceUrl, though.

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?

I was first thinking about adding member functions like uploadedAsString(), peerInfoAsString(), etc. to the TrackerRequest, but in the end I used only eventToString which is the only *String function that’s really needed. The reason was the BS interview (and also some other articles) - I think that these member functions are just not necessarily needed in the TrackerRequest and they would only make the TrackerRequest interface more inflated. I better like the TrackerRequestWriter::requestUrl() approach.

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?

Either this variant (formatting flags) or just adding an extra parameter `bool encoded` to appropriate member functions (like QByteArray query(bool encoded = false) const;) would be useful. The latter one will cause an extra parameter in more functions, but calls will be simplified from my point of view, like:

RequestHeader requestHeader ("GET", requestUrl.query (true));

The former one (flags) will cause only one extra parameter in Uri::[un]encoded(), but calls will become longer, like:

RequestHeader requestHeader ("GET", requestUrl.encoded (Uri::RemoveScheme | Uri::RemoveUserInfo | Uri::RemoveAuthority | Uri::RemovePath | Uri::RemoveFragment));

Either way, any of these two aproaches is better than the current way :). There are surely other pros/cons for both variants.

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 like the idea with HttpRequestSession::get (const Uri &requestUrl, const Header &extraHeaders)! However, I think that TrackerRequestWriter should stay as a separate class, because from my point of view it’s better for testing (we can easily test URLs creation, so in TrackerRequestSession we can test only it’s behavior) and also this class has (will have) only necessary functions/data and clear purpose - creating request URLs. What about keeping this class, TrackerRequestSession will have a member function void sendRequest (const TrackerRequest &trackerRequest); (announceUrl will be stored in the trackerRequest) and will (actually, it already does that) internaly use TrackerRequestWriter to create a request URL which will be sent using HttpRequestSession::get(). Maybe we could change TrackerRequestWriter class name to something like TrackerRequestUrlCreator?

Regards,

Petr