Thread: [pygccxml-development] config_t and software design in general
Brought to you by:
mbaas,
roman_yakovenko
From: Matthias B. <ba...@ir...> - 2006-03-08 17:37:59
|
Hi, I was just going over the config_t class to add some more doc strings when I noticed a few things I would just like to mention. When I wanted to document the "gccxml_path" argument of the config_t constructor I had a look at the code to find out what exactly it is supposed to contain and what values are valid. The first thing that surprised me was that the actual code that checks this argument is not in config_t but in source_reader_t.__raise_on_wrong_settings(). So the data and the related code is separated (even into different source files) which seems to be against the object-oriented programming principles. Then, the class uses properties to "shield" its attributes, but actually this is only exploited for about half of the attributes to make them read-only. The other attributes are read/write and *any* value is allowed. So why are they implemented as properties at all? There is no advantage in doing so if the values are simply passed through, you could just have used regular attributes which would have made the code smaller and easier to read. Especially as you have explicitly decided to do validity checks outside the class. The advantage of properties is that they can invoke actions and values can be checked and possibly rejected or corrected. This is exactly what __raise_on_wrong_settings() is doing. So in my opinion, a better place for that code would have been the __set_gccxml_path() method right in config_t. This way, config_t would be a "smart" container and other parts of the code could rely on the parameters always holding valid values which don't have to be checked again. Apart from that, I also noticed some flaws in the way the gccxml_path check is done. For example, when I specify a file and this file doesn't exist (maybe because of a typo or because some directory isn't mounted), then the source_reader just switches back to using "gccxml", entirely ignoring the fact that the user explicitly specified another version of gccxml to use. In such cases, I expect a program to use a default value only when the user did not specify any value at all. But if the user did specify a value and this value is invalid for some reason, the program should create an error (or at least issue a warning that the user's value is not used and the program switches back to its default behavior). The above cases are not really serious things, but I wanted to mention them anyway as I think they show what I would consider to be basic flaws in the design which could be improved in the future. - Matthias - |
From: Allen B. <al...@vr...> - 2006-03-09 01:14:19
|
Matthias Baas wrote: > Hi, > > I was just going over the config_t class to add some more doc strings > when I noticed a few things I would just like to mention. > > When I wanted to document the "gccxml_path" argument of the config_t > constructor I had a look at the code to find out what exactly it is > supposed to contain and what values are valid. The first thing that > surprised me was that the actual code that checks this argument is not > in config_t but in source_reader_t.__raise_on_wrong_settings(). So the > data and the related code is separated (even into different source > files) which seems to be against the object-oriented programming > principles. > Then, the class uses properties to "shield" its attributes, but > actually this is only exploited for about half of the attributes to > make them read-only. The other attributes are read/write and *any* > value is allowed. So why are they implemented as properties at all? > There is no advantage in doing so if the values are simply passed > through, you could just have used regular attributes which would have > made the code smaller and easier to read. Especially as you have > explicitly decided to do validity checks outside the class. > The advantage of properties is that they can invoke actions and values > can be checked and possibly rejected or corrected. This is exactly > what __raise_on_wrong_settings() is doing. So in my opinion, a better > place for that code would have been the __set_gccxml_path() method > right in config_t. This way, config_t would be a "smart" container and > other parts of the code could rely on the parameters always holding > valid values which don't have to be checked again. I agree with you that there is an overuse of properties that often makes the code more complex without gaining anything. I had hesitated to mention it earlier because it seemed like a little bit of nit picking. :( I was talking to one of my co-workers yesterday and describing the pygccxml and pyplusplus codebase to him. After describing the issues I was having understanding the codebase he pointed out something I had not thought about before. Namely it seems that some parts of the code are using non-Pythonic idioms. For example in python the only time you normally would use properties is to make something read-only or to trigger code (such as validation) when a value is change. On the other hand with javabeans the properties with getters/setters are used for everything. There are other areas in the code base that use metaphors I have not seen widely used in Python. For example: the class naming convention seems more like C code then python, filters and flattening are used quite often and in ways that look to be copying and iterating more then necessary. To be clear, I am not saying the code is wrong or should change I am just trying to point out that I have seen some strange things in the code as well. None of these are critical. The important thing is that the code works and works well right now. I would rather have working code that confuses me then clear code that fails. :) -Allen > > Apart from that, I also noticed some flaws in the way the gccxml_path > check is done. For example, when I specify a file and this file > doesn't exist (maybe because of a typo or because some directory isn't > mounted), then the source_reader just switches back to using "gccxml", > entirely ignoring the fact that the user explicitly specified another > version of gccxml to use. In such cases, I expect a program to use a > default value only when the user did not specify any value at all. But > if the user did specify a value and this value is invalid for some > reason, the program should create an error (or at least issue a > warning that the user's value is not used and the program switches > back to its default behavior). > > The above cases are not really serious things, but I wanted to mention > them anyway as I think they show what I would consider to be basic > flaws in the design which could be improved in the future. > > - Matthias - > > > > ------------------------------------------------------- > This SF.Net email is sponsored by xPML, a groundbreaking scripting > language > that extends applications into web and mobile media. Attend the live > webcast > and join the prime developer group breaking into this new coding > territory! > http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642 > _______________________________________________ > pygccxml-development mailing list > pyg...@li... > https://lists.sourceforge.net/lists/listinfo/pygccxml-development > |
From: Roman Y. <rom...@gm...> - 2006-03-09 06:28:39
|
On 3/9/06, Allen Bierbaum <al...@vr...> wrote: > I agree with you that there is an overuse of properties that often makes > the code more complex without gaining anything. I had hesitated to > mention it earlier because it seemed like a little bit of nit picking. :( No, no, no. If [I, we] want the project to be successful I need to hear such critic. Take it, as it is your responsibility to critic me. > I was talking to one of my co-workers yesterday and describing the > pygccxml and pyplusplus codebase to him. After describing the issues I > was having understanding the codebase he pointed out something I had not > thought about before. Namely it seems that some parts of the code are > using non-Pythonic idioms. You don't even know how much he is right!!!! I make my living from coding pretty big system in C++. I had few reasons, when I started to develop pyplusplus. One of them is was to get an experience with coding in Python. ( The main reason is that I believe in multi language development environment, and this is a tiny thing I can do in order to promote this approach ) If you take an abstract look on the code you will see C++ there: 1. all members are private 2. there is accessor ( get,set ) for every member 3. coding convention 4. I almost don't use Python dynamism 5 I am sure you can fill the list >For example in python the only time you > normally would use properties is to make something read-only or to > trigger code (such as validation) when a value is change. On the other > hand with javabeans the properties with getters/setters are used for > everything. There are other areas in the code base that use metaphors I > have not seen widely used in Python. For example: the class naming > convention seems more like C code then python, I explained this >filters I like stl algorithms as an approach to solve the problems. >and flattening > are used quite often and in ways that look to be copying and iterating > more then necessary. You are right. There is only one reason: get the job done in shorter time. I don't like to write complex code, but may be by using the full power of Python, I still can get an expressiveness and simplicity. You will need to teach me. Also, this specific critic is right, in general, it lacks the numbers. I did profiling and I am satisfied with the numbers. I never saw in profiler I am paying fo= r copying and iterating. Lets say that if new approach will improve speed by = 20% we will commit it. > To be clear, I am not saying the code is wrong or should change I am > just trying to point out that I have seen some strange things in the > code as well. Because this is a C++ code using Python :-). You and Allen have commit permissions, \ right? Go ahead and fix it, please, please, please. I think we should not even discuss such things. The only things we need a discussion is design and interface. I want to believe, that design is right. If you think different, please say it. I am sure it will be an interesting conversation :-). > None of these are critical. Don't you think, that after documentation this is a second reason why people don't use pyplusplus? > The important thing is that the code works > and works well right now. I would rather have working code that > confuses me then clear code that fails. :) You are 100% right. > -Allen Thank you and Matthias for raising those points. -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |
From: Allen B. <al...@vr...> - 2006-03-09 15:43:55
|
> >Don't you think, that after documentation this is a second reason why >people don't use >pyplusplus? > > Yes, in my opinion this is definitely a reason that people may not want to use pyplusplus. The current design is very good but there are parts of the interface that look complex and this can scare people away. Before making too many changes in this regard though I think we should all agree to what the goal is for the changes. Perhaps agree to some rough coding standards or coding idioms to use. -Allen |
From: Roman Y. <rom...@gm...> - 2006-03-09 15:48:38
|
On 3/9/06, Allen Bierbaum <al...@vr...> wrote: > > > > >Don't you think, that after documentation this is a second reason why > >people don't use > >pyplusplus? > > > > > Yes, in my opinion this is definitely a reason that people may not want > to use pyplusplus. The current design is very good but there are parts > of the interface that look complex and this can scare people away. So as you can see you pointed to the big problem. > Before making too many changes in this regard though I think we should > all agree to what the goal is for the changes. Perhaps agree to some > rough coding standards or coding idioms to use. Yes. I will read Python coding guide-lines. May be will will change somethi= ng for this release may be not. Anyway if you have some ideas - please share t= hem. > -Allen -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |
From: Roman Y. <rom...@gm...> - 2006-03-09 05:53:25
|
On 3/8/06, Matthias Baas <ba...@ir...> wrote: > Hi, > > I was just going over the config_t class to add some more doc strings > when I noticed a few things I would just like to mention. > > When I wanted to document the "gccxml_path" argument of the config_t > constructor I had a look at the code to find out what exactly it is > supposed to contain and what values are valid. The first thing that > surprised me was that the actual code that checks this argument is not > in config_t but in source_reader_t.__raise_on_wrong_settings(). So the > data and the related code is separated (even into different source > files) which seems to be against the object-oriented programming > principles. :-). Please critic me as much as you can. I like this kind of arguments. You are right. > Then, the class uses properties to "shield" its attributes, but actually > this is only exploited for about half of the attributes to make them > read-only. The other attributes are read/write and *any* value is > allowed. So why are they implemented as properties at all? There is no > advantage in doing so if the values are simply passed through, you could > just have used regular attributes which would have made the code smaller > and easier to read. Especially as you have explicitly decided to do > validity checks outside the class. And you are right here > The advantage of properties is that they can invoke actions and values > can be checked and possibly rejected or corrected. This is exactly what > __raise_on_wrong_settings() is doing. So in my opinion, a better place > for that code would have been the __set_gccxml_path() method right in > config_t. This way, config_t would be a "smart" container and other > parts of the code could rely on the parameters always holding valid > values which don't have to be checked again. Hmm. I am not sure how I should implement this. From the one side I don't want to force user to give all arguments in constructor. I think that next coding style should be supported, but I could be wrong. cfg =3D config_t() cfg.gccxml_path =3D ... cfg.... =3D .... Do I need to support it? If now it is pretty clear to me how to fix this cl= ass. > Apart from that, I also noticed some flaws in the way the gccxml_path > check is done. For example, when I specify a file and this file doesn't > exist (maybe because of a typo or because some directory isn't mounted), > then the source_reader just switches back to using "gccxml", entirely > ignoring the fact that the user explicitly specified another version of > gccxml to use. In such cases, I expect a program to use a default value > only when the user did not specify any value at all. But if the user did > specify a value and this value is invalid for some reason, the program > should create an error (or at least issue a warning that the user's > value is not used and the program switches back to its default behavior). This analisys should be moved to config_t.gccxml_path property, right? > The above cases are not really serious things, but I wanted to mention > them anyway as I think they show what I would consider to be basic flaws > in the design which could be improved in the future. Please answer my question and I will improve it in a near future. ( You can do it also :-) ) > - Matthias - -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |