After spending half of the night in the CVS of PS and in writing this article, I decided to publish this post as an extra thread. It has not much to do with the leaving of kronon... and it should better reside in the development forum.
Originally posted by Vengeance
While I think there are some areas better than others in our code, most of the comments in here have not been about those sections, but instead have been very general comments about the complexity of CS. Yes it is complex, but I\'d like to hear from you which parts you think you can take out.
Ok, Vengeance, you want to have concrete hints, where CRITICS see problems... and damned, you are right. It is tedious to talk about common tasks, when they never come to the bottom. I am not willing to leave me in the corner of the critics without analytic expertise. But perhaps I belong exact to that place? Anyway, I want to show, what I mean. (Btw, YOUR comment was as general as the others too
).
I simply walk in the actual CVS in the heart of the client/gui directory. Took there the first from the (alphabetically sorted) files: chatwindow... header and implementation file. Wandered over the first class and stopped at the second one: pawsChatHistory. Now I examined the declaration AND implementation. Following things I want to mentioned:
Global Issues:- Comment says: \"Class to store chat history. Implementation is a circular buffer\". Hm... looking only to the declaration of the class functions and member variables I thought... this code CRYES for a template class. If there is a need for a similar buffer of an other type the programmer has to copy/paste the whole thing (one of the main source of errors in coding).
- In the function Insert() the copy of the string argument is allocated with csNewStr() - a Crystal Space function. But what, if I only want to code in some parts of the Planeshift code. Is it necessary to now all the details about CS-functionality? So every coder willing to help planeshift is liable to get also close to Crystal Space. Wouldn\'t it be better to HIDE the engine code?
- What does csNewStr? Does it allocate with new or with one of the alloc functions? The resulting buffer is freed with delete. So probably csNewStr follows its name and calls new. But why then is the buffer for the string pointers allocated with calloc? Is mixing the allocation function a standard procedure in planeshift. Are there existing rules? Where is this DOCUMENTED? And how the allocation functions are traced? Does CS this? Is the global ::new allocator overloaded? Where is it DOCUMENTED? How do you check for memory leaks?
- How do you profile the code? I do not see any function traces? Do you get the compiler generating profiling code? Do you take professional programs like Numega TrueCoverage or TrueTime? Where is this DOCUMENTED?
- Why do you decide to use in this class a buffer of pointers and not directly store the strings in the buffer? Fast access to the strings? Or was the reason an easier handling? I do not know. But if I want to learn from your code, I have to extract all your consideration, all your transpiration for myself... and all your preliminary work is for nothing. That is one of the biggest difficulties with Open Source projects. You get soooo much code around your ears... but only very few information, ow the decisions are done. How the organisation runs. How the communication in an international team works. Where are the log files of the IRC-Sessions, where you discussed this? Or even a summarization of this? Does every coder are only responsible for his task? Are there only one boss, which gives out the programming problem?
- It takes me 15 minutes to understand, that this class does NOT manage the history of the chat window... IT IS THE HISTORY OF THE EDIT FIELD WINDOW. There is really a lack of DOCUMENTATION, if I have to work out this from the code!
- Why do you use char and not TCHAR or any other unicode-compatibility? Does localization not play a role in PS?
- Why do you decide not to use the hungarian notation? I find it difficult to distinguish between local and class variables, if there is no naming differences. And more difficult, if I can not see, if the variable is a pointer, an int, float or a string directly in the name.
After I had problems getting the clue of the variable getLoc I made a short test program. Here I must honour the author, because it was a very easy task to program a small console proggy to test the class! But it results to the conclusion, that this class was never tested!
Local Issues:- I get a compile warning in constructor: initsize(argument) is of type size_t and size(member) of type unsigned: could result in a loss of data. You should either do an explicit type cast or declarate of same type or at least comment the line to clarify the decision.
- Why is the check vor an invalid n(argument) in GetCommand() is done AFTER the calculation of the index?
- Why is the allocation of the buffer implemented in constructor and also in SetSize. Wouldn\'t it be better to set size to NULL in the constructor and then call SetSize() as THE function for reallocate the buffer?
- Why differs the type of the argument in constructor and SetSize()... even they mean the same variable?
- In the function Insert() the string is allocated with csNewStr - probably with \"new char[strlen(str)+1]\". Should not the freeing operation be \"delete []\" than \"delete\"?
- In SetSize() getLoc is wrong calculated at the end (that IS an error). Can be result in an invalid value. Try this:construct object with initsize=3; add three words; call three times GetPrev; call SetSize(2); If you now call GetNext() you get a wrong (but valid) string. The string is valid because of the next error.
- The function SetSize() was NEVER tested... The line \"newBuffer = (char **)calloc(size, sizeof(char *));\" shows, that the new buffer is allocated with the size of the old... and not with the argument newSize.
That brings me to the last point: Where are all the little test programs, that verify the correctness of the code? The programs, that take a class or connected collection of classes to show, that they work independent of the rest of the code base. The programs, that create different error cases, which make shure, the implementation is strong and right?
I hope, I could get you a clue about my thoughts and how I learned to develope. I would very, very glad, if people find errors in my statements... \'cause any of those hints helps me to get a better programmer.
Ah, only one additional point (the really last one - promised ;)). I will not accept the answer of the lack of persons and time for the above mentioned things. I know that. I agree with that. I understand it. But for a huge project like this, I think some organisation, testing and thinking SHOULD be done before implemeting only one single line of code.
br, Winterheaven.
p.s. I wrote this post NOT to criticize the work of the planeshift team. I wrote this post, BECAUSE I am willing to help and give my (perhaps little) experiance into planeshift. Vengeance asked for detailed comments... and I wrote them. That is all. If this is helpless... sorry for any inconveniences.
