On Mon, 2010-01-18 at 23:17 +0100, Jiří Techet wrote: > > OK looking at the demos' code I have 4 comments: > > * the api seems fine but we should provide helper functions that > > will build this chain in one call. I doubt so many users will > > have to toy with the internals of the chains. > > Agree - this was actually a third possibility I had in my mind but > forgot to write :-). It could be something like > > champlain_map_source_factory_create_cached_source Sounds good. > > (if you want something less verbose, come with a better name :-) > > > * I don't like how you have to specify the cache's settings on > > creation, especially the cache's path. Those should be > > configurable but should not be passed every time you have to > > create a cache. > > The only problem is that you have to specify these during construction > (we create the cache directory in the path with sqlite database in it > when it doesn't exist yet). But apart from _new_full() we can have > _new() with some sane default values (100MB database [or rather 60day > database now], .cache/champlain as the cache directory and permanent > cache) A default constructor solves this indeed. > > * Regarding coding styles the only errors I've seen are missing > > spaces between function names and (). > > I used astyle --style=gnu, but it doesn't make the spaces. I also > looked at indent, but the gnu style is a bit different as well. Do you > use some pretty printer (and which options) or do everything manually > (if not, I think it would be easier to start using some pretty printer > instead of checking the sources manually). > > > * champlain_map_source_chain_push_map_source is a bit verbose. > > Let's go for champlain_map_source_chain_push. :) > > OK, the hardest part about coding is inventing the function and > variable names :-) Also there is no "style guide" lines for that. :) Pierre-Luc
Attachment:
signature.asc
Description: This is a digitally signed message part