Ciao Martin, comments below... On 8/30/07, Martin Desruisseaux <[EMAIL PROTECTED]> wrote: > Hello Simone > > I had a look at some classes modified in the coverage module. I did some > changes, hopping that I'm not causing trouble. Comments (hopefully minor) > below: > > Private vs protected fields in ImagingParameter > ----------------------------------------------- > Some private fields has been given protected access. In the particular case of > ImagingParameters, I assume that it was for the ImagingParameterDecorator > needs, > which live in the same package. In such case, we don't need to make those > fields > "protected"; the package-privated access is suffisient and safer. > > Lets recall that "protected" is like "public": it is commited API. If we want > "protected like" access without commiting API, the package-private access is > often applicable. > > > New protected methods in OperationJAI > ------------------------------------- > Some piece of code in the OperationJAI.doOperation(...) method has been > factored > out, which is a good idea. However I don't think that all of them should be > given protected access. For example the following variable: > > Boolean requireGeophysicsType > > is really an implementation trick; a public API should rather use an > enumeration > (this work was started with ViewType but not finished). > > I conserved the new "protected" access for the following method: > > getDescriptor > prepareParameters > getQuantitative > > I had some hesitation about the following ones. I made a search in the > Geotools > code base and didn't find a usage for them, so I believe it is safer to keep > them private. I made this change on the 2.4 branch only (not yet merged to > trunk). If it is causing a problem, please let me know: > > extractSources > postProcessResult > ensureRenderedImage > getSubCRS > ensureStableDimensions
All your opinions seem reasonable to me and as far as the tests I added for Extrema and Histogram work I am happy. However, there is reason behind not using private fields and methods too much and this reason is that we are an open source project. Even though author's opinion and design goals are important, keeping things as much extensible as possible is far more important to me. So this is why I usually try to keep private filed down to a minimum so that people can extend classes and change behaviour and I don't care id they should not do it, if they do something bad, that's their fault. Protected and package private have very different scopes. I leave things protected so that other people can use whtever package they want and still subclass my classes. I have seen that you use a lot of package private access to tie classes together and private methods to hide utilities methods, well this some time is a big limitation because providing design paths for developers is good but building walls to deny them to take some other paths is pretty bad, at least IMHO. > > Note about the @author tag in the new methods: author name is already in the > class javadoc, which is nice. Maybe it is not necessary to put them in new > method javadoc? Especially since the new methods were essentially > copy-and-paste > of existing code. I simply thought that adding the author to the newly added methods would have helped the review since most people I know don't really like to spend hours on diff :-). > > > Adding @deprecated Decorators > ----------------------------- > I added the @deprecated tag to ImagingParameterDecorator and > ImagingParameterDescriptorDecorator, not because the classes are not good but > just because I would like to warn users to avoid these classes for now, > because > we are likely to merge them with ImagingParameter and > ImagingParameterDescriptor > when we will have a chance and the decorator would be removed after the merge. > That's really not a problem, I would probably add more operations like Extrema and Histogram, but once we move those classes I will update the relevant operations. Could we do something to avoid the usual deprecate-remove cycle for these classes due to their intrinsic experimental nature? > Despite that, if I'm not missing some points those classes are not really > decorators since they extend existing classes. They do not wraps an existing > instance as decorator usually do... > A wrapper wraps, a decorator decorates :-). Seriously, it is just a name, the previous one was even worse, so feel free to change it. I was thinking about a name that gives the idea of "you can change the behaviour of the underlying ImageParametersBlahBlah by replacing some params" then someone suggested decorator or adapter since you are decorating/adapting the JAI parameters. Probably adapter would be a better suffix, it is up to you! > > Formatting > ---------- > Replaced tabulations by spaces, which is the norm in the coverage and > referencing modules. Some use 4-spaces tabulation, other use 8-spaces > tabulation. Unless the Geotools PMC agree on a tabulation size (maybe it does; > I'dont remember), it is safer to stick to spaces. > > I noticed that some "overridden" words in javadoc have been replaced by > "overrided". I'm really not sure since I do a lot of English mistakes, but > isn't > "override" an irregular verb? > My brain is probably still snorkling at the Elba Island! > > > Please let me know if those changes are causing any issues... The reason for > those changes is to be conservative about what we put in public API of > supported > library modules. > > Martin > -- ------------------------------------------------------- Eng. Simone Giannecchini President /CEO GeoSolutions S.A.S. Via Carignoni 51 55041 Camaiore (LU) Italy phone: +39 0584983027 fax: +39 0584983027 mob: +39 333 8128928 http://www.geo-solutions.it ------------------------------------------------------- ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/ _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
