Hi Peter,

thanks for your comments! I’ll try to explain why I’ve made things the way they are.

I was just having a look at TrackerRequestDriver and it occurred to me that QUrl might be extremely helpful for handling the query string parameters - have a look at QUrl::addQueryItem(). It can also handle URL-encoding itself!

Yes, this was my first idea how to do it, however there is one major problem with QUrl encoding - it’s converting input data into utf-8 before they’re actually encoded, so instead of “%80″ I got “%C2%80″ etc (problem is with bytes with their ordinal value higher then 127, e.g. non-ascii). I haven’t found a way how to disable this “feature” so I had to implement data encoding and appending on my own.

I guess a more functional style could be more readable and more unambitious (function calls have no side effects):

temporaryRequestUrl = announceUrl_ + "?";
temporaryRequestUrl += infoHash() + "&";
temporaryRequestUrl += peerId() + "&";
...

This is also a way how to do that, but I’ve chosen my variant for these three reasons (major are 1st and 3rd):

• I don’t have guaranted that the announce URL won’t contain “?” at the beginning (or do I? - haven’t found it in specs). For example, this could be a valid announce URL for me: http://tracker.com/announce.php?someparam=somevalue. This could mean that this particular tracker wants another parameter someparam to be set. So now I simply check what delimiter I should use and I put it there. See also the next reason.
• Now the calls don’t have to be in a specific order (ok, they should, because of tests, but lets forget it) because they just don’t need to be (info_hash could be the last parameter and “nothing happens”) and the delimiter is inserted at the beginning of the next parameter/argument, not at the end (in your example one should (but according to the rfc not necessarily) to omit the last ‘&’ - also see the 3rd reason) because of my appendArgumentDelimiter member function.
• Some of the arguments are only optional and in my case I can easily don’t append them because I test it in my append functions. I also need to “stringify” the argument and TorrentRequest doesn’t do it. This way I can also possibly check it’s validity (see my last comment in this post).

Similarly void writeRequestUrl (QByteARray &) could become QByteArray requestUrl().

Yes, I think I’ll change it - I firstly have this function returning bool if everything was written ok, but maybe it won’t be necessary because now it’s not validating the request data, it only creates a request URL.

Or maybe with QUrl one could:

QList <QPair <QString, QStirng> > parameters;
parameters += pair (ParameterInfoHashName, trackerRequest_.infoHash());
parameters += pair (ParameterPeerIdName, trackerRequest_.peerId());
…

where pair is a helper taking two QStrings and returning a QPair of QStrings. The entries with empty values could be filtered before adding them to the QUrl object using addQueryItem() or setQueryItems().

There is also that “nasty utf-8 conversion problem”, as I described it in the first comment, so I can’t use the QUrl class.

I guess we could assert on an empty infoHash (and others?) because without it we do not have a valid request.

I was thinking how/where/when we will check the request validity, because the TrackerRequest constructor doesn’t do it and TrackerRequestWriter shouldn’t do this because it’s simply not it’s job, so now it’s up to a tracker to find out whether the incoming request is ok or not. But because this is not a solution we should think about something else. The first variant is (as you’ve suggested) to simply use assert on some mandatory arguments, but there are many things that should be verified (from my point of view):

• info_hash length (exactly 20B)
• peer_id length (exactly 20B)
• uploaded (can’t be
• downloaded (can’t be
• left (can’t be
• ip (if not valid then omit this parameter?)
• numwant (can’t be
• trackerid length (if not valid then omit this parameter?)

Question is, whether this should be done in the TrackerRequestWriter… The second way could be creating some TrackerRequestValidator and then the TrackerRequestWriter could take for granted that the passed request is valid (a precondition). The last idea is to check request arguments in the TrackerRequest constructor, but then we’ll need some isValid() member function or a Null object… These two variants assume that we will consider non-validity as a regular error, but maybe asserts will be just fine since these can be considered as programming errors. What do you think?

Each valid Torrent object must have a valid info dictionary. Maybe we could have Torrent calculate its infoHash?

Yes, that’s a good idea. It can simply calculate it from the input QByteArray. I was thinking about QCryptographicHash, but this class is available only in Qt 4.3 and I got only Qt 4.2 :(. Which Qt version should we use, anyway? I think I’ve read somewhere (calitko docs) that we should use only classes/functions that are in the 4.1 version, but I’m not sure.

Ok, I hope I’ve explained the way why it’s done the way it is :). I’ll be glad to have a further discussion on this because that’s the way how to improve this project and gain more experience!

Have a nice day, Petr