Python - BeautifulSoup関数の書き直しに関するヒントをより洗練されたものにする

Question

コードは機能していますが、これをもっと適切に書く方法、特にifを使う方法のヒントを探しています。あなたは自然界ではプログラマーではないと言うことができます...ただsysの管理者がPythonでちょっとしたことをしています。あなたが提供できるアドバイスをありがとう。

def findallWileyLinks(): 

pagebase = 'http://onlinelibrary.wiley.com' 
journallist = 'http://onlinelibrary.wiley.com/browse/publications?type=journal&&start=0&resultsPerPage=3000' 

inputList = getinputList() 

if inputList: 
    alljournallistsoup = BeautifulSoup(getwebpage(journallist)) 

    if alljournallistsoup: 
    alljournallisttags = alljournallistsoup.find('ol', attrs={'id' : 'publications'}) 

    for eissn in inputList: 
     journalatag = alljournallisttags.find('a', attrs={'href' : re.compile(eissn.rstrip() + '$')}) 

     if journalatag: 
     journalsoup = BeautifulSoup(getwebpage(pagebase + journalatag.get('href') + '/issues')) 

     if journalsoup: 
      allvolumetags = journalsoup.find('ol', attrs={'class' : 'issueVolumes'}) 
      volumeatags = allvolumetags.findAll('a') 

      for volumeatag in volumeatags: 
       volumesoup = BeautifulSoup(getwebpage(pagebase + volumeatag.get('href'))) 

       if volumesoup: 
       allissuetags = volumesoup.find('li', attrs={'id' : volumeatag.get('id')[:-5]}) 
       issueatags = allissuetags.findAll('a')[1:] 

        for issueatag in issueatags: 
        currentlinksavailiable.append(pagebase + issueatag.get('href') + '\n') 

     else: 
     appendlog('eISSN: ' + eissn.rstrip() + ' not found on alljournallist page.') 

    try: 
     with open(inputDirectory + selectedPublisher + '_currentlinksavailiable.txt', 'w') as f: 
      f.writelines(currentlinksavailiable) 

    except IOError as e: 
     appendlog('findallLinks() Operation failed probably when creating the new link text file with error: %s' % e.strerror) 

Answer 1

ことの一つは、フォームのコードの多くを持っているということです。 forループ内の空のリストを使用すると、ループの本体は実行されません。

小さな点として、ファイルの先頭にあるinputList = []は、すぐに関数呼び出しで上書きするため、削除することができます。

これが大きなスクリプトの一部であるかどうかは不明ですが、ifブロックにスクリプトの本文を含めるのではなく、inputListが空の場合は終了する必要があります。

if not inputList: 
    sys.exit(1) 

ではなく

if inputList: 
    # process inputList 

あなたは仕事にそのため、あなたのスクリプトの先頭にimport sysを追加する必要があります。

Answer 2

あなたは、関数内のコードを入れて、あなたのif文否定し、使用リターンを最大限に活用、ステートメントを続行するか中断する可能性があります。そんなにぎこちないことは避けたい。

また、リストが空の場合はforループがスキップされるため、必要でないように見えるforループの直前にifステートメントがあります。 I. if tags:ラインが冗長であるので

tags = parenttag.findAll('tag') 

if tags: 
    for tag in tags: 
     # do something to tag 

あなたは、tagsはリストがここにあることが保証されています。私に飛び出し

if volumeatags: 
    for volumeatag in volumeatags: 
      ... 

Answer 3

このコードはたくさんありません。いくつかの例：

inputList = [] 
inputList = getinputList() 

Pythonでは、変数を初期化してからデータを割り当てる必要はありません。 2行目は単独で問題ありません。

if volumeatags: 
    for volumeatag in volumeatags: 

volumeatagsが空の場合、forループは実行されません。

Answer 4

ここで書き直しを依頼するのは非常に一般的ではないと思います。とにかく、私のフィードバックです。

if foo: 
    if bar: 
     #code 

（右？

if not foo: 
    return 
if bar: 
    #code 

コメントはあなたがコメントアウトしたコードのハッシュ・小文字を維持し、ハッシュのブランク大文字で始まる必要があり、あなたがdef内ですが、あなたはasusmingと交換Peter Norvigは、テキストコメントに2つのダッシュを入れているようですが、とにかくコメントが多すぎます。コードには十分な説明が必要です。物事を分離するのにも役立ちます。

ループをif fooで保護しないでください（他の回答に記載されています）。

inputList = []は役に立たず、PEP08に準ずるものではありません。inputを試してください。