[Tutor] is there a better way to organise this code

[Joel Goldstick]

On Wed, Nov 30, 2011 at 8:46 PM, Dave Angel <d at davea.name> wrote:

> On 11/30/2011 07:49 PM, Steven D'Aprano wrote:
>
>> Norman Khine wrote:
>>
>>> hello,
>>>
>>> is there a better way to organise this code or optimise it.
>>> http://pastie.org/2944797
>>>
>>
>> Is that a question? Because I get a syntax error in my brain when I parse
>> it without the question mark. <wink>
>>
>> Sorry to pick on you, but it astonishes me when people don't bother with
>> basic English syntax, and yet try writing code where syntax is *much* more
>> important. If they can't be bothered with writing correct English, that
>> sends all the wrong signals about the quality of their code.
>>
>> You should write as if you were coding, otherwise people will assume you
>> code like you write.
>>
>> Laziness is one of the cardinal virtues of the programmer, but it has to
>> be the right sort of laziness. "Don't reinvent the wheel, use an existing
>> library" is good laziness. "Leave out required syntax elements and hope
>> someone else will fix them" is not.
>>
>> Before worrying about optimising the code, how about checking whether it
>> works?
>>
>> (1) What is CSVFile? It appears to be a class, because you inherit from
>> it, but it isn't defined anywhere and isn't a builtin. So your code fails
>> on the very first line.
>>
>> (2) You have a class WorldSchema with no methods, and a top-level
>> function get_world that *looks* like a method because it has an argument
>> "self", but isn't. The indentation is wrong. See what I mean about syntax?
>> Syntax is important. So is get_world a wrongly indented method, or a poorly
>> written function?
>>
>> (3) Since get_world doesn't use "self" at all, perhaps it should be a
>> top-level function of no arguments? Or perhaps a static method of
>> WorldSchema?
>>
>> (4) You have a class called "getCountries", which seems to be a poor name
>> for a class. In general, classes should be *things*, not *actions*. Also I
>> recommend that you follow PEP 8 for naming conventions. (Google "PEP 8" if
>> you don't know what I mean, and remember, it isn't compulsory, but it is
>> recommended.) A better name might be CountryGetter.
>>
>> (5) The use of classes appears on first reading to be a Java-ism. In
>> Java, everything must be a class Just Because The Powers Who Be Said So. In
>> Python, we are allowed, and encouraged, to mix classes and functions. Use
>> the right tool for the job. But without any idea of the broader context, I
>> have no idea if classes are appropriate or not.
>>
>> (6) getCountries has a method called get_options. Based on the name, a
>> reasonable reader would assume it returns some sort of list or dictionary
>> of options, right? But according to the documentation, it actually returns
>> a JSON "ser", whatever that is. Server? Service? Serialization (of what)?
>> Something else?
>>
>> (7) Other problems:  Enumerate, MSG and iana_root_zone are used but not
>> defined anywhere. Documentation is lacking, so I don't understand what the
>> code is intended to do. Another class with a poor name, "getRegions". There
>> may be others, but I stopped reading around this point.
>>
>>
>>  I stopped looking at his pastie once the background turned black.  I'd
> have had to copy it elsewhere to even read it.
>
>
> --
>
> DaveA
>
>
> There is a button in the upper right corner of the paste bin that lets you
switch the hideous black background to something more pleasant. :)

-- 
Joel Goldstick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/tutor/attachments/20111201/5b4f74da/attachment.html>