Hi Petr,

You’ve done some really great job! I totally liked your commit messages! I’ll have to write mine more like yours ;-). Below are further comments on each of your revisions.

rev 111

Yes, I think you’re right, we’d better user the C++
.

rev 112

I see you have configured your editor to remove trailing spaces and I just did the same :-)! Maybe we should also write this in the coding conventions?

It’s nice that BDictionary::item() and BDictionary::keys() are const. maybe item() should return a const BItem pointer too. Otherwise it would be possible to change the contents (item) of a const dictionary.

Actually we don’t need the inline for the function declaration inside the class, because for the compiler it is enough to see the inline in the function definition. I read once, and I agree, that the user of the class does not need to care whether a function is inline or not.

Adding the function hasReadAllCorrectly() is actually a good idea and later I realized it was in fact a good refactoring too (it used to be a function used in the tests). I’m even now wondering whether we at all need hasReadPastEnd() and hasReadAll(). The same reasoning could be applied to BinaryReader too.

rev 113 to 115

Following Marcus’s comment about the __ prefixed identifiers being reserved by the standard library I removed the leading __ from the include guards. Would you please do the same for your new headers?

Torrent.h

It’s great that Torrent already uses implicit sharing of private data!

Same comment as above about the inline in the class function declarations.

Regarding the UNIT_TEST ifdef, I’m getting to believe more and more firmly that one should try to only test using the public interface. Testing private methods, which are part of the implementation means that our test will have to change when our implementation changes. We would actually want to use the same tests even when we change the implementation. I haven’t checked your tests yet, but generally, I would pass Torrent::fromRawData() different inputs and will make sure that the parsed Torrent has the exact properties that I expect. I would not go and test the private function individually.

Torrents.cpp

I think I’d actually drop the Torrent (const QByteArray &) ctor. I’m also thinking, wouldn’t it be nice to have a TorrentParser class do the parsing? Since Torrent is like an advanced struct which also have reference counting with implicit sharing, we could add setters to Torrent and TorrentParser could just start with an empty Torrent and set the fields as they are parsed. Just an idea… let me know what you think.

Great source code comments!

QSharedDataPointer does provide a copy ctor and an assignment operator for itself, so you are not required to provide these for Torrent – the compiler will do it automatically.

I think I’d drop the bool and ! operators. The semantics of if (torrent) may no be obvious and I’d personally prefer if (torrent == Torrent()). What do you think?

Is the line const int Torrent::HASH_VALUE_SIZE; really required? I thought the definition in the header would be enough. We also tend to write constants in the same case as classes, i.e. HashValueSize.

TorrentTest.cpp

Maybe we’d better turn areEqualTorrents() into a member operator==(). That’s actually the same refactoring you did with hasReadAllCorrectly() in BDecoder.

I’m not sure if we really need this getBytes(). We could just as well use the QByteArray(const char *) ctor. The only difference with fromRawBytes() is that the ctor copies the bytes and fromRawBytes() attaches the returned QByteArray to the raw buffer.

When I first started writing unit tests I wrote a single test function that had multiple sub-scopes that essentially were different cases of the same or similar test scenario. After reading and experimenting more I started writing numerous individual test functions instead and used names that try to clearly state what each test is doing, so that it is easier to the reader to figure out what’s going on. For example, testLoadTorrentDataEmptyTorrnetFile, testLoadTorrentDataSimpleTorrent, testLoadTorrentDataTorrentWithAnnounceLists, etc. Then if I see that it’s pretty much the same code but using different data, I refactor the repeating code in a test scenario function, which is in essence a parametrized test case. For example, scenarioLoadTorrentData (const Qstring &fileName, const Torren &torrent) and I call the scenario function from the individual tests, passing in the test data. You can see this and other approaches in my recent tests.

Using the above presented approach you could probably rework e.g. testAnnounceListCreation() to a scenario which takes the raw announce lists dencoded data and the expected parsed list. The scenario function could use the raw data to create a minimum raw torrent, parse it, and only validate the value of announceList(). This way we would not be testing the private function explicitly but only implicitly. This way our test will verify that the private function is both correctly implemented and integrated.

I think that was all for now! Hope you also have some interesting comments on my comments ;-)

Best regards,

Peter