Hi Petr,

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.

Hm, haven’t thought of that - it makes sense with multi tracker torrents but I think it would be rare that all are queried at the same time (maybe only as the multitracker torrent is initially started). Let’s create a separate request for each announceUrl and move announceUrl to the TrackerRequest.

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.

OK, let’s keep the classes separate! I think that the right place for eventToString() is in TrackerRequestWriter. It knows the binary format of the request and evenToString() returns the binary format of the event field. No user of TorrentRequest would need to know how this field maps to a URI query item value.

I was thinking again about what Bjarne Stroustrup said it that interview - he suggested that such helper functions are outside a class. I guess we could simply have a Utils module containing a few helper functions like trackerRequestToRequestUri() and the static class member vars would be moved to the cpp file. Just an idea - let’s stick to TrackerRequestWriter for now.

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

I suggested passing flags to encoded() and unencoded() because your code in getPathForHttpHeader() actually concatenated the path and query components (to produce the URI for the GET request) and thus duplicated the logic already implemented in Uri. I agree that in addition to extending encoded() and decoded() we could add a parameter to each of the setters defaulting to unencoded. Similarly, we could add a parameter to the getters defaulting to unencoded. Yet another option would be to provide static helpers that can encode/decode strings (an issue here is that we exclude different chars from the encoding).

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

Alternatively the flags could say what to be included like IncludeAuthority | IncludePath.

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.

Agreed!

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?

That’s really great!

Regards,

Peter