current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I am using the Python module urllib3. It occurs to me that there is a better way to design a good class when I rebuild my last site crawler.

class Global:
    host = 'http://xxx.org/'
    proxy=True
    proxyHost='http://127.0.0.1:8087/'
    opts=AttrDict(
        method='GET',
        headers={'Host':'xxxx.org',
              'User-Agent':'Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0',
              'Accept':'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
              'Accept-Language':'en-us;q=0.5,en;q=0.3',
              'Accept-Encoding':'gzip, deflate',
              'Connection':'keep-alive',
              'Cookie':'xxxxxxx',
              'Cache-Control':'max-age=0'
              },
        assert_same_host=False
    )
    def getPool(self,proxy=None):
        if proxy is  None:
            proxy = self.proxy
        if(self.proxy):
            http_pool = urllib3.proxy_from_url(self.proxyHost)
        else:
            http_pool = urllib3.connection_from_url(self.host)
        return http_pool


class Conn:
    def __init__(self, proxy):
        self.proxy= proxy
        self.pool = Global().getPool(self.proxy)
    def swith(self):
        self.pool = Global().getPool(not self.proxy)
    def get(url, opts=Global.opts):
        try:
          self.pool.urlopen(
              method=opts.method, 
              url= url, 
              headers= opts.headers, 
              assert_same_host=opts.assert_same_host
              )
        except TimeoutError, e:
            print()
        except MaxRetryError, e:
            # ..

I try to make class contains all global configs and data for others to call, just like I do in JavaScript. But, are these classes too tightly coupled? Should I just merge them together?

share|improve this question

migrated from stackoverflow.com Oct 27 '12 at 16:21

This question came from our site for professional and enthusiast programmers.
    
I think that those two are really parts of the same class. And should be combined. –  Dan D. Oct 27 '12 at 5:28
    
Does anyone need access to this stuff besides Conn? If not, it probably belongs in Conn. If you're worried about creating a separate copy of all of these constants in every Conn instance, you can make them class variables instance of instance variables. (And you may want to make getPool a @classmethod, too.) –  abarnert Oct 27 '12 at 5:32
    
Actually I just need change configs and grab rules for every specifical site, merge the two class together means I should create a lot of different Conns and make code lager.I just wonder is it deserved? –  android.ikaros Oct 27 '12 at 6:00

2 Answers 2

up vote 6 down vote

Class design

A good class in general is one that corresponds to an Abstract Data Type, which is a collection of data and operations that work on that data. A good ADT should have a single, clear responsibility.

The Global class is not a good ADT:

  • It does two unrelated things:
    • Contain configuration information
    • Manage a proxy pool
  • Possibly as a consequence of the previous point, it is poorly named

It would be better to split this class into two:

  • Configuration or Config: contain configuration information
  • ProxyPoolManager: manage a proxy pool

Note that ProxyPoolManager should not reference Configuration directly: it will be best to pass to it the proxyHost and proxy as constructor parameters.

As the poorly named Conn class already does half of the proxy pool management, it would be better to rename it to ProxyPoolManager and move getPool method into it and rename Global to Configuration.

Coding style

You have several coding style violations, not conforming to PEP8:

  • Use spaces around = in variable assignments, for example:
    • proxy = True instead of proxy=True
    • opts = AttrDict(...) instead of opts=AttrDict(...)
  • No need to put parentheses in if(self.proxy):
  • There should be one blank line in front of method definitions of a class: put a blank line in front of get and switch methods in the Conn class
  • You mistyped switch as swith

Other issues

This code doesn't compile:

class Conn:
    def get(url, opts=Global.opts):
        try:
          self.pool.urlopen(...)

Because it makes a reference to self which was not passed in as method parameter.

This is a bit hard to understand, because of reusing the name "proxy" both as method parameter and as attribute:

def getPool(self,proxy=None):
    if proxy is  None:
        proxy = self.proxy
    if(self.proxy):
        http_pool = urllib3.proxy_from_url(self.proxyHost)
    else:
        http_pool = urllib3.connection_from_url(self.host)
    return http_pool

The purpose of this method would be more clear if you used a different name for the method parameter.

share|improve this answer
up vote 0 down vote

getPool() doesn't use the proxy argument. The method starts by using the class value to set a default value. However, the if statement just uses the class value and ignores the argument it just assigned a default value to.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.