3 ms·
Fun read, but using automatic properties might lead you down a path that isn't optimal; Take this for example: public byte[] Infohash { get; private set;
by NKCSS 10y ago
Fun read, but using automatic properties might lead you down a path that isn't optimal;
Take this for example:
public byte[] Infohash { get; private set; } = new byte[20];
public string HexStringInfohash { get { return String.Join("", this.Infohash.Select(x => x.ToString("x2"))); } }
public string UrlSafeStringInfohash { get { return Encoding.UTF8.GetString(WebUtility.UrlEncodeToBytes(this.Infohash, 0, 20)); } }
You have an automatic property and two 'properties' that actually perform work every time you call the getter (might be smarter to make functions of those, so you know it's not just retrieval of data, but work is done).
If you were to rewrite this a bit, you could make sure the 'work' is done only when needed, and the properties become actual simple data retrieval properties like:
public class Hashes
{
byte[] _infohash;
string _hexStringInfohash, _urlSafeStringInfohash;
public byte[] Infohash
{
get { return _infohash; }
private set
{
_infohash = value;
_hexStringInfohash = String.Join("", this.Infohash.Select(x => x.ToString("x2")));
_urlSafeStringInfohash = Encoding.UTF8.GetString(WebUtility.UrlEncodeToBytes(this.Infohash, 0, 20));
}
}
public string HexStringInfohash { get { return _hexStringInfohash; } }
public string UrlSafeStringInfohash { get { return _urlSafeStringInfohash; } }
public Hashes()
{
Infohash = new byte[20];
}
}
Going further through the article, I spot many more items to improve; but let's not forget your did great work and the code is quite readable.
One thing that might help; is building some indexes to know how files are fragmented; you have the following code multiple times:
if ((start < Files[i].Offset && end < Files[i].Offset) ||
(start > Files[i].Offset + Files[i].Size && end > Files[i].Offset + Files[i].Size))
continue;
If you'd build an index to know which piece hits which files, you don't have to enumerate this every time.
Another general remark is to always 'retrieve' an indexed item from the array and use that instead of keep calling the 'indexed' record.
So; do:
var file = Files[i];
if ((start < file.Offset && end < file.Offset) ||
(start > file.Offset + file.Size && end > file.Offset + file.Size))
continue;
The code becomes more readable and allows you to change the structure later on more easily since you don't have 100 references tot he same array now and only use an itermediate.
- cheatdeath 10y agoThanks for the comments! I'm all for improved readability. I definitely wasn't aiming for much performance wise, at least initially. These were two areas (of many) that I felt could do with some improvement (especially the file IO). I will look into making some modifications like those suggested when I get a chance.