첫번째 PR 리뷰

Karl 은 우리 회사 Software Third Party Provider 업체인 Nearly Human 의 사장님이다. 우리 회사 메인 어플리케이션인 EnergyScoreCards 두번째 버전을 개발하는 프로젝트에 참여중인데 내 첫번째 PR 에 Karl 이 직접 리뷰를 해줬다. 배운걸 까먹고 싶지 않아 간단히 기록한다.

 

1. 상수는 클래스 안에 넣어라, 함수 안이 아니라. (Put constant variable under class, not under function)

# original code
def _get_data(self, dataset_id, version_id, access_token):
        print('  ',(colors.green & colors.bold)["+"], 'Getting the latest dataset')
		
        ......
        
        chunk_size = 900000


# revised code
class CallConserviceApi(cli.Application, WithConfig):
    VERSION = "1.0.1"
    VERSION = "1.0.2"
    
    chunk_size = 900000
	......

chunk_size, num_iteration 이 _get_data 함수 안에 있었음 -> class object 밑에 넣음

 

2. 긴 루프를 피해라. 함수로 만들어서 짧게 만들어라 (avoid long while loop -> convert into function)

# original
def _get_data(self, dataset_id, version_id, access_token):
        print('  ',(colors.green & colors.bold)["+"], 'Getting the latest dataset')
        total_num_row = self.total_num_row
        chunk_size = self.chunk_size
        num_iteration = int(total_num_row/chunk_size)+1
        total_records = []
        
        for i in range(0, num_iteration):
            ......
        return total_records
        

# revised
def _get_data(self, iteration, dataset_id, version_id, access_token):
        print('  ',(colors.green & colors.bold)["+"], 'Getting the latest dataset')
		offset = iteration*self.chunk_size
        records = self._get_data_by_chunk(offset, self.chunk_size, dataset_id, version_id, access_token)
        records = records if records else []
        num_records = len(records)
        print('\tFound {} records'.format(str(num_records)))
        if num_records == self.chunk_size:
            records.extend(
                self._get_data(self, iteration+1, dataset_id, version_id, access_token)
            )
        return records
        
def _get_data_by_chunk(self, offset, chunk_size, dataset_id, version_id, access_token):
        properties_query = ', '.join(['\'{}\' as \'{}\''.format(prop, prop) for prop in self.properties])
        query = """
            q = load \"{}/{}\";
            \nq = order q by 'LastModifiedAt' asc;
            \nq = foreach q generate {};
            \nq = offset q {};
            \nq = limit q {};
        """.format(dataset_id, version_id, properties_query, offset, chunk_size)

        body = {
            'query': query
        }
        response = requests.post(
            'https://platform.gobyinc.com/services/data/v54.0/wave/query',
            headers={'Authorization': 'Bearer {}'.format(access_token)},
            json=body,
            verify=False
        ).json()

원래 함수 1개 그리고 안에 루프 -> 함수 2개 만들고 루프를 없앰

 

3. 루프안에 루프는 피해라 (Avoid nested loop) (피곤함. 리뷰할 때 프로그래머가 루프 돌아가는 과정을 계속해서 생각해야 되기 때문에)

# original
def _get_data(self, dataset_id, version_id, access_token):
        print('  ',(colors.green & colors.bold)["+"], 'Getting the latest dataset')
        total_num_row = self.total_num_row
        chunk_size = self.chunk_size
        num_iteration = int(total_num_row/chunk_size)+1
        total_records = []

        for i in range(0, num_iteration):
			...
            
                for record in records:
                    total_records.append(record)
                if num_records < chunk_size:
                    break
            else:
                raise Exception('Could not find any records in response')
			...
                
        return total_records
        
 # revised
 def _get_data(self, iteration, dataset_id, version_id, access_token):
        print('  ',(colors.green & colors.bold)["+"], 'Getting the latest dataset')
        offset = iteration*self.chunk_size
        records = self._get_data_by_chunk(offset, self.chunk_size, dataset_id, version_id, access_token)
        records = records if records else []
 		num_records = len(records)
        print('\tFound {} records'.format(str(num_records)))
        if num_records == self.chunk_size:
            records.extend(
                self._get_data(self, iteration+1, dataset_id, version_id, access_token)
            )
        return records

recursive function으로 for loop 대신함

 

4. def _foo

-> 앞에  underscore (_) 들어가는 함수는 private function. 공용으로 사용되는 함수 아니고 이 프로젝트에서만 사용되는 한정적인 사적인 함수