2011-10-27 3 views
2

코드가 작동하지만 좀 더 적절하게 작성할 수있는 방법에 대한 팁을 찾고 있습니다. 특히 if를 사용하는 것이 좋습니다. 당신이 말할 수 있듯이 필자는 본질적으로 프로그래머가 아닙니다 ... 단지 파이썬에서 약간의 작업을하는 시스템 관리자입니다. 제공 할 수있는 조언을 주셔서 감사합니다.Python - BeautifulSoup 함수를 다시 작성하는 방법에 대한 더 자세한 내용은

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) 

답변

3

한 가지 양식의 코드를 많이 가지고있다. for 루프에서 빈 목록을 사용하면 루프 본문이 실행되지 않습니다.

작은 점으로서, 파일의 처음에 inputList = []은 즉시 함수 호출로 덮어 쓰면 제거 될 수 있습니다.

큰 스크립트의 일부인지 여부는 확실하지 않지만 if 블록에 스크립트 본문을 포함하는 것이 아니라 inputList이 비어 있으면 종료해야합니다.

if not inputList: 
    sys.exit(1) 

보다는

if inputList: 
    # process inputList 

당신은 그 작동하려면 스크립트의 상단에 import sys를 추가해야합니다

.

+0

의견을 보내 주셔서 감사합니다. if 앞에있는 if는 inputList = [] 및 currentlinksavailiable = []와 함께 제거되었습니다. 이것은 더 큰 스크립트의 일부입니다. 이것은 특정 게시자에 대한 링크를 가져 오기 위해 작성되었습니다. 일단 이것이 세련되면, 다른 출판사에 더 비슷한 기능을 쓰는 템플릿으로 사용할 것입니다. – Brad

1

당신은 함수에 코드를 삽입하고 경우 문 부정 사용 수익의 대부분을, 문을 계속하거나 중단 할 수있다. 그렇게 많은 들여 쓰기를 피할 수 있습니다.

또한 for 문이 필요없는 것처럼 보이는 for 문 바로 앞에 if 문이 있습니다. 목록이 비어 있으면 for 루프가 건너 뜁니다. 즉, if tags: 라인이 중복 그래서

tags = parenttag.findAll('tag') 

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

당신은 tags이 목록은 여기 보장됩니다 나를 밖으로 점프

if volumeatags: 
    for volumeatag in volumeatags: 
      ... 
+0

에 대한 의견을 보내 주셔서 감사합니다. 그것은 의미가 있으며 불필요한 몇 줄을 제거했습니다. 들여 쓰기를 최소화하기를 원하기 때문에 return/continue/break 문을 사용합니다. 다시 감사합니다. – Brad

0

많은 코드가 필요하지 않습니다. 몇 가지 예 :

inputList = [] 
inputList = getinputList() 

파이썬에서는 변수를 초기화하기 전에 변수를 초기화 할 필요가 없습니다. 두 번째 줄은 독자적으로 괜찮을 것입니다.

if volumeatags: 
    for volumeatag in volumeatags: 

volumeatags이 비어 있으면 for 루프가 실행되지 않습니다.

0

여기서 재 작성을 요청하는 것이 일반적이라고 생각하지 않습니다. 어쨌든, 여기 내 의견이 있습니다.

if foo: 
    if bar: 
     #code 

(당신이 def 내부에 asusming로 교체,하지만 당신은 오른쪽입니까?

if not foo: 
    return 
if bar: 
    #code 
, 해시 - 빈 - 대문자로 시작하는 당신이 주석 처리 한 코드에 대한 해시 소문자를 유지해야

댓글 피터 노빅 (Peter Norvig)은 텍스트 주석에 두 개의 대시를 넣는 것처럼 보이지만 어쨌든 너무 많은 주석이 있습니다. 코드에 충분히 설명이 있어야합니다. 작업 블록에 하위 헤더가 필요한 경우 함수에서이 설명을 함수 이름으로 사용하십시오. 사물을 격리하는 데 도움이 될 것입니다.

루프를 if foo으로 지키지 마십시오. 다른 답변에서 언급했듯이.

inputList = []은 쓸모없고 PEP08을 준수하지 않으므로 input을 시도하십시오.

+0

'foo와 bar'인 경우 더 좋을 것입니다. 더 적은 수의 줄이 더 자주 더 좋습니다 :-) –

+0

감사합니다. 가독성을 높이기 위해 원래의 게시글에서 주석을 제거합니다. 나는 sys 관리자이기 때문에 필자는 프로그래밍이 반드시 필자의 강점이자 의견을 많이 듣는다. – Brad

관련 문제