[ckan-dev] coding style

[James Gardner]

Hi Will,

+1 to all this.

I've never had to resort to @classmethod in my own code - helper
functions are fine. Agree that self should be changed everywhere it is
used to cls, it is just misleading at the mo. Also agree there are far
too many little functions/methods that just do one thing. No need.

Let's all chip in and fix these as we find them, or does someone want to
take them on as a single task?

Cheers,

James


. On Wed, 2011-01-26 at 18:30 +0100, William Waites wrote:
> Shall put words to this effect on the coding style wiki page,
> but wanted to bring it up on the dev lists first (sorry for 
> duplicates, crossposting).
> 
> Some things that I've noticed in the various codebases that
> strike me as very ugly:
> 
> Things like this:
> 
>     def _start_call_timing(self):
>         c.time_call_started = self._get_now_time()
> 
>     def _get_now_time(self):
>         import datetime
>         return datetime.datetime.now()
> 
> there is no need whatsoever for these methods or ones like
> this. That's two function calls and five lines of code for
> what could easily be written as:
> 
>     c.time_call_started = datetime.now()
> 
> Also, do not import packages, especially standard ones in
> the body of a function / method unless there's a good reason
> (like the feature is optional and might throw an import error)
> 
> Next, there are three problems with the following method:
> 
>      @classmethod
>      def _uri(self, uri):
>          if isinstance(uri, basestring):
>              return URIRef(uri)
>          else:
>              return uri
> 
> first, there is a bug, URIRef inherits from unicode and from
> basestring so the isinstance check fails -- but this is just
> a bug, no big deal.
> 
> second, why is this a method and not a utility function? it
> has nothing at all to do with the self argument. Functions 
> and methods should use all of their arguments otherwise they
> belong elsewhere.
> 
> third, when using @classmethod, the first argument should be
> cls and not self. self is for instances.
> 
> That's all for now...
> 
> -w