Hey Peter

Reply in bold

Peter Dimov wrote:

Hi Atul,

I could perfectly parse ;-) your patch. It’s remarkable how a few lines of code can be worth more than hundred words ;-).

I’m a bit concerned about adding this readInt() function to BinaryReader because the cohesion of the class would suffer. The thing is that BinaryReader is meant to be used when parsing binary data.

Going by XP, this meaning could be changed. Or rather the meaning should be accomodated.
As I applied Feature Envy and the code is simpler, it seems BinaryReader should have this functionality … What do you think?
It is very similar to QDataStream. The function readInt() actually extracts an int from a text encoding and would suit better a class like TextReader (QTextStream would be the Qt equivalent).

Yes it parses text ( ASCII to be more precise … as the original method used ASCII collating sequences ). So we could move readInt to some new class and use it in BDecoder.
I would suggest that BinaryReader::readCString() is modified and renamed to:

As I understand it, you are suggesting that instead of using indexOf, we use the modified readString. That is a very good refactoring ;-) I will give it a try today…
– cheers atul

atul wrote:Hey Peter

Here is what I mean - the patch ( and all the tests pass ;-)

0. BDecoder::readInt() just verifies the markers ‘i’ etc.
1. readInt() is pushed to BinaryReader ( it is a reader so should read integers too ;-)
2. BinaryReader::readInt() pushes sets up a few things and pushes the ball to QByteArray::toInt…. So we don’t have to worry about the parsing, conversion errors etc…

3. We need BDecoder::readInt() as the marker processing is a decorator - on top of BinaryReader ;-)

4. We don’t worry about the method calls etc. at this stage… clarity first - also decomposing a program into smaller methods/functions could make it faster — this is really something we should not worry about - and maybe after the profiler tells us ;-)

5. We have a bigger chance of being correct ( as the chunk is done by Qt lib which is well tested and debugged )

let me know ( and yes the ‘e’ marker handling is in BinaryReader::readInt but I will remove it … )
— cheerio atul
— cheerio atul

Peter Dimov wrote:

I’m a bit confused, Atul! Are you talking about BDecoder or BinaryReader? Could you elaborate your point a bit please?

Regards,
Peter

Peter Dimov wrote:Hi Atul, the thing is that with integers we might need to swap bytes whereas readBytes() and readCString() should never swap bytes. Or maybe I got you wrong?

Peter

atul wrote:Dear all

Why not use

int QByteArray::toInt() and leave all the processing to it? ;-)

I mean we could write a BinaryReader::toInt() wrapper and that could just call the QByteArray version….

So there would be a lot of less code to maintain ( and more reuse etc… )

Could the same ideas be used for BDecoder::readString?

I think I will write another SourceComment for that ;-)

—- cheerio atul