Hi Anders,

Thanks for the cooperation at first.

Directly to your points:

[1] – yes you are right. We do not use auto or smart pointers yet. It is a nice idea and we have to think about it.

[2] – in this special case maybe not because there I need all the digits except the 0. In some other cases though, I may use the isdigit function.

[3] – the same as [1]

[4] – here you are right. It is a matter of taste. I just looked at the coding conventions of Calitko and according to it, the brackets are to be removed there.

[5] – it’s a good idea. I also thought about it at first. It is nice to have a more extensive explanation of the errors. We can change this later.

[6] – here I just save myself the time for creation of the pointers at each cycle of the loop J. Actually that is also a matter of taste.

[7], [8] – you already gave an answer to these questions. The destructor of BItem is virtual.

[9] – about try and catch. We assert the errors now. I agree that ignoring is not a solution and we don’t ignore the possible errors.

Thanks for the hints once again. I will wait a little more for other comments before applying the changes and uploading another patch.

Regards Slavcho

anders44 wrote:

Hi,

I am new to this project to checkout code so I started out looking at your code but do not have the full picture yet so bare with me if I have misunderstood something in your code.

– BDecoder.cpp –
p.binaryReader = new BinaryReader

[1] why not use auto_ptr ? - in general it seems auto_ptr is not used, I think it is a must-have for all allocations (part from arrays of course) so that you always have clear owners of any allocated objects.

if (ch > ‘0′ && ch

[2] it is more correct to use “isdigit( ch )” from include instead

return new BErrorItem();

[3] here is an example where I would use an auto_ptr instead. By doing so, you force the caller of the function to take ownership of any returned object. i.e. if the return type of the function is an auto_ptr.

[4] it is not good writing “new classname()” instead “new classname” is better to avoid accidentally mistaking a function call with an allocation

[5] when returning an error BErrorItem why not give more information about the error ? instead of just “there was some error” It makes tracking down errors easier.

BItem *key, *value;

[6] should be moved inside loop below where they are used.

[7] There is no virtual destructor but it is used to inherit from. I would recommend using the standard way of doing classes so that all cases are covered i.e. have a default ctor, dtor, assignment and default copy constructor etc. The reason for this is when doing classes that other people should use, one must expect they are not familiar with the internals of a class to avoid them using the class in a wrong way it is important to tailor the class in a foolproof way.

– BDictionary.cpp –

[8] Generally speaking any class that is inherited from should have a virtual destructor. In your BItem you don’t have a virtual _dtor.

[9] Why are not functions encapsuled in try/catch. In general there seems to be lacking exception and error handling. Best is to detect errors as soon as possible and not ignore anything.

HTH/AJK.

Hi,

I think I may have made a mistake here, there are virtual dtor’s - I was just browsing the .patch file so the full class was not in it.

Cheers/Anders.