[okfn-dev] [ckan-dev] coding style

[Seb Bacon]

On 26 January 2011 17:50, James Gardner <james at 3aims.com> wrote:
> +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?

It's too big a task to do in one -- that's mainly what I spent last
week doing on the harvesting code -- about 1.5 days for quite a small
module.  Let's just agree to refactor as we go along?

Seb

> . 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
>
>
>
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
>