Re: Helping each other to get started…

Hi Petr,

That looks pretty good!

I’ve made the TorrentParser class a friend of the Torrent class because then we can validate input data only in the TorrentParser during parsing and we don’t have to validate them on two places (this would be needed if we were using setter functions). Now there is no way how to change (and possibly corrupt) Torrent data directly (as far as I know that wouldn’t be necessary at all) without using TorrentParser. Let me know what do you think about that.

Your argument about torrent object corruption makes sense, so I agree we’d better not have setters. But maybe instead making TorrentParser a friend class we could provide a Torrent ctor that initializes all torrent fields without extra validation. We could thus assume that the object is created correctly. TorrentParser would in fact be a helper that creates a Torrent object after parse the fields from the raw data and initialize a new Torrent object from it. At a later point (let’s give that low priority for now) we could also create a TorrentBuilder, which would build a torrent for one or more files. This class could similarly use the Torrent ctor to initialize the new object and we would not need to declare it a friend. I’m fine with the friend solution, so if you want stick to it.

In TorrentParser::parseAndLoadTorrent() I think I’d assign the local torrent object to the out parameter torrent right before the function returns true, instead of reseting it when the function returns false. Note also that implementing the function this way will make it meet the strong guarantee of exception safety, namely the function will produce no side effects if an exception gets thrown (e.g. from loadAllTorrentData). In this case meeting the strong guarantee does not cost us anything, so we should meet it.

Now that we have the TorrentParser that only returns a valid Torrent object we could probably get rid of the isValid member.

I’ve refactored unit tests as you’ve suggested. There are now two separate unit tests - one for the Torrent class and one for the TorrentParser. I’ll add more tests very soon (various data parsing testing) and improve current tests.

Yes, that would be great!

Good night,

Peter

Would you like to post a relpy?


This post is a reply to:
Re: Helping each other to get started…
Hi Peter, so I've refactored the original Torrent class by splitting it's parsing functions into a new class - TorrentParser (as you've suggested). Changelog and some other information are in (more...)

Follow-ups:
Re: Helping each other to get started…
Hi Peter, Your argument about torrent object corruption makes sense, so I agree we’d better not have setters. But maybe instead making TorrentParser a friend class we could provide a Torrent (more...)