2017-09-08 3 views
1

Ich schrieb diesen Code mit verschachtelten if else Fällen, aber ich fühle, dass es so hässlich ist und frage mich, ob es irgendeinen Weg gibt, es zu verbessern (oder einen besseren Weg, dies zu erreichen).Jede Möglichkeit, Code mit mehreren zu verschachteln, wenn verschachtelt?

def do_something(self, response): 
    a_url = response.css('a.classA::attr(href)').extract_first() 
    if a_url: 
     a_url = a_url.split('&')[0] 
    else: 
     a_url = response.css('a.classB::attr(href)').extract_first() 
     if a_url: 
      a_url = a_url.split('&')[0] 
     else: 
      logger.error('get no url') 
    if a_url: 
     yield Request(
      url=a_url, 
      dont_filter=True, 
      callback=self.do_next_thing 
     ) 

Das Hauptanliegen ist, dass ich eine URL/Link aus einer Antwort extrahieren will, und dann gespalten es und das erste Element erhalten. Aber a_url existiert nur in einem von zwei (oder mehr) Elementen. Ich kann die split nicht direkt tun, weil a_urlNoneType sein kann. Ich möchte versuchen mit try except else, aber das scheint noch komplizierter zu werden.

Irgendwelche besseren Lösungen?

+2

https://codereview.stackexchange.com/ –

Antwort

4

Ich denke, man es so tun könnte:

def do_something(self, response): 
    a_url = (
     response.css('a.classA::attr(href)').extract_first() 
     or 
     response.css('a.classB::attr(href)').extract_first() 
    ) 

    if not a_url: 
     logger.error('get no url') 
     return # or raise an exception and let the caller do the logging 

    yield Request(
     url=a_url.split('&')[0], 
     dont_filter=True, 
     callback=self.do_next_thing 
    ) 

Dies die kurzen verwendet -Schaltung Verhalten der or operator:

der Ausdruck x oder y wertet zuerst x aus; Wenn x wahr ist, wird der Wert zurückgegeben. Andernfalls wird y ausgewertet und der resultierende Wert zurückgegeben.

Es nutzt auch die „frühe Rückkehr“ Technik, das heißt der Fehlerfall wird zuerst behandelt, und dann der „normale“ Fall außerhalb jeden if oder else erfolgen.

2

Der beste Weg, diesen Code zu vereinfachen, ist es, beiden Klassen in Scrappy in erster Linie zu wählen:

def do_something(self, response): 
    a_url = response.css("a.classA::attr(href), a.classB::attr(href)") 
    if a_url: 
     yield Request(
      url=a_url.split('&')[0], 
      dont_filter=True, 
      callback=self.do_next_thing 
     ) 
    else: 
     logger.error('get no url') 
0

Sie könnten in Betracht ziehen, die Methode in zwei (später sogar drei) aufzuteilen. Denn wie ich es sehe, sind die ersten Zeilen eher eine Vorbereitung als eine tatsächliche Logik. Etwas wie folgt aus:

def prepare_something(self, response): 
    a_url = response.css('a.classA::attr(href)').extract_first() 
    if a_url: 
    return a_url.split('&')[0] 
    else: 
    a_url = response.css('a.classB::attr(href)').extract_first() 
    if a_url: 
     return a_url.split('&')[0] 
    else: 
     logger.error('get no url') 
     return None 


def do_something(self, response): 
    a_url = self.prepare_something(response) 
    if a_url: 
    yield Request(
     url=a_url, 
     dont_filter=True, 
     callback=self.do_next_thing 
    ) 

Auf diese Weise imho ist der Code ein wenig sauberer, und Sie sind in der Lage zu sehen, dass Sie die prepare_something Methode, wie die folgende Refactoring möchten:

def get_a_url_part(self, response, path): 
    a_url = response.css(path).extract_first() 
    return a_url.split('&')[0] if a_url else None 

def prepare_something(self, response): 
    a_url = self.get_a_url_part(response, 'a.classA::attr(href)') 
    b_url = self.get_a_url_part(response, 'a.classB::attr(href)') 
    return a_url if a_url else b_url 

def do_something(self, response): 
    a_url = self.prepare_something(response) 
    if a_url: 
    yield Request(
     url=a_url, 
     dont_filter=True, 
     callback=self.do_next_thing 
    ) 

Aus meiner Sicht könnte dies als eine Verbesserung angesehen werden.

Grüße :)

Verwandte Themen