PMD. AvoidInstantiatingObjectsInLoops에 대해서 어떻게 생각하시는지요?

[임성현]

PMD(http://pmd.sf.net)에서 권고하는 룰셋을 보면 정말 이런 것도 고려해야 하는지..? 라는 쪼잔한 룰 부터
설마 이런 것도 실수할까 싶은 내용도 있습니다.(자세한 사항은 추후에.)

그 중에 다양한 환경과 상황에 따라서 의견이 분분한 룰이 AvoidInstantiatingObjectsInLoops 입니다.

자세한 설명을 살펴보면,..

** AvoidInstantiatingObjectsInLoops
------------------------------------------------------------------------------------------------------------
Since: PMD 2.2
Detects when a new object is created inside a loop
This rule is defined by the following Java
class:net.sourceforge.pmd.rules.optimization.AvoidInstantiatingObjectsInLoops
Example:

public class Something {
public static void main( String as[] ) {
for (int i = 0; i < 10; i++) {
Foo f = new Foo(); //Avoid this whenever you can it's really
expensive
}
}
}
------------------------------------------------------------------------------------------------------------

이라고 나오고, Optimization Rule에 해당하는 사항입니다.

간단하게 말씀드려서 loop안에서 반복적으로 사용하는 객체의 경우 루프 밖에서 생성하고 재활용하라는 의미인데.

아래 메소드를 한번 보시죠.(Service에서 사용하는 메소드 입니다.)

public List sampleAbcList(List inList){//DB에서 DVO에 정보를 넣어서 List로 전달
List returnList = new ArrayList();
Iterator itr = inList.iterator();
while(itr.hasNext()){
//리스트에서 하나씩 꺼내서 가공함
AbcDVO dvo = new AbcDVO(); //--------(1)
dvo = (AbcDVO) itr.next();
dvo.setLoginEpno(session.getLognEpno());
dvo.setScnId(session.getScnId());
dvo.setLastChgPsEpno(session.getEpno());
returnList.add(dvo);
}
return returnList;
}

위의 경우, inList는 DB에서 정보를 가져온 내용이고, 이를 하나씩 꺼내서 가공해서 다시 넣는 경우인데..
이 경우 처음 말씀드린 Rule을 활용해서 객체생성(1)을 while 밖에서 생성해서 재활용해야 한다 라고 가이드하는 것이
1) 문제점이 없는지
2) 이득을 얻을 수 있는지
라는 측면이 이슈 입니다.

rule 위반을 해결하려면 아래와 같이 작성하는데,

public List sampleAbcList(List inList){//DB에서 DVO에 정보를 넣어서 List로 전달
List returnList = new ArrayList();
Iterator itr = inList.iterator();
AbcDVO dvo = new AbcDVO(); //--------(1)
while(itr.hasNext()){
//리스트에서 하나씩 꺼내서 가공함
dvo = (AbcDVO) itr.next();
dvo.setLoginEpno(session.getLognEpno());
dvo.setScnId(session.getScnId());
dvo.setLastChgPsEpno(session.getEpno());
returnList.add(dvo);
}
return returnList;
}

이 부분에 대해서 제 의견은 이렇습니다.
(1) 문제점은 없는지...
- abcDVO는 이미 DAO에서 만들어서 inList로 담아서 보내고 있고
- 이를 (1)에서 생성된 객체는 단지 받아서 전달만 하므로 returnList에 담기는 객체는 inList의 DVO임
- 따라서, (1)의 dvo는 원본 객체의 레퍼런스만 담고 있는 객체가 됨
- 재활용에 문제 없음
(2) 이득을 얻을 수 있는지
- 개발자의 습관과 경험을 바꿀 정도로 의미가 있을까...의 사항인데,
위 내용은 보통 조회성 화면의 게시판 테이블에 담기는 경우가 대부분입니다.
- 즉, List의 사이즈가 많아도 20~40개 정도 만들어지는데.... 이를 큰 오류라고 생각하고 해결하기 위한
노력을 하는 것이 중요한가. 라는 부분 입니다.
- 여기에서는 다소간의 견해 차이가 존재할 수 있습니다만, 제 생각에는
확실한 사항에 대해서만 개발자의 습관을 바꾸도록 한다
(예: obj.equals("String") --> "String".equals(obj) 으로 null point
exception 방지 등)
입니다.

따라서, 내부적으로는 위 사항을 감안하여 결과를 확인하고, 조치는 강제사항이 아닌
권고사항으로 접근하는것이 좋겠다는 생각입니다.

다른 분 의견은 어떠하신지요?....

[심상호]

초큼(?) 당황스러운 Source Code인데요...

당연히 원래 Source Code는 문제가 있어 보입니다.

사용하지도 않을 AbcDVO를 Loop 내부에서 생성하고 있기 때문이죠. 바로 다음줄에서 List에서 꺼내서 생성한 객체를 assign한 변수에 reassign하고 있네요.

 

당연히 변경된 방식으로 수정하셔야 하며

굳이 사용하지도 않을 객체를 new 하지 않으셔도 됩니다.

 

       public List sampleAbcList(List inList){//DB에서 DVO에 정보를 넣어서 List로 전달
               List returnList = new ArrayList();
               Iterator itr = inList.iterator();

               AbcDVO dvo = null;    //--------(1)

[Sanghyuk Jung]

       public List sampleAbcList(List inList){//DB에서 DVO에 정보를 넣어서 List로 전달
               List returnList = new ArrayList();
               Iterator itr = inList.iterator();
               while(itr.hasNext()){
                       //리스트에서 하나씩 꺼내서 가공함
                       AbcDVO dvo = new AbcDVO();    //--------(1)
                       dvo = (AbcDVO) itr.next();
                       dvo.setLoginEpno(session.getLognEpno());
                       dvo.setScnId(session.getScnId());
                       dvo.setLastChgPsEpno(session.getEpno());
                       returnList.add(dvo);
               }
               return returnList;
       }

위의 소스는 (1) 에서 new 객체 생성이 아예 필요가 없지 않나요?

어짜피 다음 줄에서 list 안에 있는 객체를 가지고 와서 참조를 바뀌기 때문에 바로 new 객체는 바로 GC대상이 됩니다. 새로 new 한 객체가 쓰일 일도 없고, 중간에 Exception이 날일도 없으므로 try catch에 대비한 기본값으로의 의미도 없습니다. 

 

 

                       AbcDVO  dvo = (AbcDVO) itr.next();

바로 이렇게 쓰면 필요없는 객체가 생성이 되지 않습니다.

 

그리고 아래와 같이 loop 밖으로 빼는 경우도

 

       public List sampleAbcList(List inList){//DB에서 DVO에 정보를 넣어서 List로 전달
               List returnList = new ArrayList();
               Iterator itr = inList.iterator();
               AbcDVO dvo = new AbcDVO();    //--------(1)
               while(itr.hasNext()){
                       //리스트에서 하나씩 꺼내서 가공함
                       dvo = (AbcDVO) itr.next();
                       dvo.setLoginEpno(session.getLognEpno());
                       dvo.setScnId(session.getScnId());
                       dvo.setLastChgPsEpno(session.getEpno());
                       returnList.add(dvo);
               }
               return returnList;
       }

(1)에 있는 코드에서도 new AbcDVO();    도 필요없는 초기값입니다.

 

굳이 loop의 마지막 값을 참조해야한다던지하는 이유로 밖으로 뺄 필요가 있다면

 

AbcDVO dvo = null;

이라면 충분합니다.

 

그리고 대부분의 경우는 변수의 범위를 최소화해서, Loop안으로 선언을 넣는 것이 바람직한 코딩방식입니다.

 

Loop 안에서의 변수의 범위에 대한 자세한 내용은 아래 링크에 설명되어 있습니다.

 

http://benelog.egloos.com/1382604

 

 결국 List안에 있는 객체를 하나씩 꺼내와서 필드의 값을 변경한다던지 하는 코딩에서는 new로 객체를 생성할 일이 없고, 초기값 의미인 객체를 new로 생성을 해봤자, 그  초기 값 객체가 쓰이지 않으므로 (loop안에서 다른 객체의 레퍼런스로 바뀌게 되죠) 재활용되지도 않습니다. 그리고 Stack 메모리영역은 알아서 loop 안에서는 재활용됩니다.

 

 

2010년 10월 28일 오후 5:04, 임성현 <sunghy...@gmail.com>님의 말:

--
Google 그룹스 'Korea Spring User Group' 그룹에 가입했으므로 본 메일이 전송되었습니다.
이 그룹에 게시하려면 ks...@googlegroups.com(으)로 이메일을 보내세요.
그룹에서 탈퇴하려면 ksug+uns...@googlegroups.com로 이메일을 보내주세요.
더 많은 옵션을 보려면 http://groups.google.com/group/ksug?hl=ko에서 그룹을 방문하세요.

[Sungchul Park]

이 경우, 성능에는 좋지만 위험한 부분이 있습니다.
매개변수로 넘어 온 리스트에서 꺼낸 객체를 가공하고 새 개체에 넘기는 사이
에 원본이 변조되는데요.
sampleAbcList()를 호출한 쪽에서 내가 매개변수로 넘긴 리스트가 변조될 수
있다고 가정하는 일은 거의 없는 듯 합니다. 일종의 최소 놀람 원칙 위반이랄
까요? 불변객체를 사용해 파급 효과를 최소화 하는 관례가 요즘 강조되고 있
습니다.

[Sanghyuk Jung]

원본을 변조하지 않으려면 new나 clone 같은 것이 당연히 필요하겠죠.

그렇게 하려면 아래와 같이 코드가 바뀌어야할 것 같습니다.

              while(itr.hasNext()){
                      //리스트에서 하나씩 꺼내서 가공함

                       AbcDVO  source = (AbcDVO) itr.next();

                       AbcDVO dvo = new  AbcDVO (source.getId()..... 등등 초기화 값);

                      dvo.setLoginEpno(session.getLognEpno());
                      dvo.setScnId(session.getScnId());
                      dvo.setLastChgPsEpno(session.getEpno());
                      returnList.add(dvo);
              }
              return returnList;

그리고 이 경우에도 new 생성말고 dvo 선언 자체를 밖으로 빼는 것은 아무 성능에 이득이 없습니다. 변수범위를 최소화하는 것이 좋겠죠.

 

 

 

2010년 10월 28일 오후 5:38, Sungchul Park <gyu...@gmail.com>님의 말:

[Sungchul Park]

아마 첫번째 소스에 의도하지 않은 실수가 있는 듯 합니다.
원래 제시하려고 했던 소스는 이런 게 아닐가요?

public List sampleAbcList(List inList){//DB에서 DVO에 정보를 넣어서 List로 전달
List returnList = new ArrayList();
Iterator itr = inList.iterator();
while(itr.hasNext()){
//리스트에서 하나씩 꺼내서 가공함

AbcDVO dvo = new AbcDVO((AbcDVO) itr.next());

dvo.setLoginEpno(session.getLognEpno());
dvo.setScnId(session.getScnId());
dvo.setLastChgPsEpno(session.getEpno());
returnList.add(dvo);
}
return returnList;
}

inList에서 하나씩 받아서 이것을 복사한 다음에 (이 과정에 새 객체가 생성
되죠) 이를 반환할 리스트(returnList)에 넣고 반환하는...

> 위의 경우, inList는 DB에서 정보를 가져온 내용이고, 이를 하나씩 꺼내서 가공해서 다시 넣는 경우인데..
> 이 경우 처음 말씀드린 Rule을 활용해서 객체생성(1)을 while 밖에서 생성해서 재활용해야 한다 라고 가이드하는 것이
> 1) 문제점이 없는지

이 경우는 PMD의 룰이 잘못입니다. PMD가 더 똑똑했다면 지적하지 않았을 것
같네요.

> 2) 이득을 얻을 수 있는지

객체 생성을 밖으로 뺄 수 없습니다.

> rule 위반을 해결하려면 아래와 같이 작성하는데,
>
> public List sampleAbcList(List inList){//DB에서 DVO에 정보를 넣어서 List로 전달
> List returnList = new ArrayList();
> Iterator itr = inList.iterator();
> AbcDVO dvo = new AbcDVO(); //--------(1)
> while(itr.hasNext()){
> //리스트에서 하나씩 꺼내서 가공함
> dvo = (AbcDVO) itr.next();
> dvo.setLoginEpno(session.getLognEpno());
> dvo.setScnId(session.getScnId());
> dvo.setLastChgPsEpno(session.getEpno());
> returnList.add(dvo);
> }
> return returnList;
> }
>
> 이 부분에 대해서 제 의견은 이렇습니다.
> (1) 문제점은 없는지...

루프 밖에서 생성한 객체는 전혀 사용되지 않습니다.

> (2) 이득을 얻을 수 있는지

1번 소스의 원본에서 new한 객체도 이번에 밖으로 뺀 객체도 전혀 사용되지
않습니다. 오류이니 이득을 따질 필요가 없네요.

> - 즉, List의 사이즈가 많아도 20~40개 정도 만들어지는데.... 이를 큰 오류라고 생각하고 해결하기 위한 노력을 하는 것이 중요한가. 라는 부분 입니다.

20-40 정도되는 리스트를 만드는 게 VM 입장에서 큰 부하는 아니라고 생각합
니다. 필요하면 만들어야겠죠. 하지만 애초에 원본 소스에 오류가 있어 성능
을 논할 단계는 아니라고 생각합니다.

[JUNG-LAE JO]

처음 질문을 보면.. 서비스 단인데 굳이 원본을 복사할 필요가 있을까요?

해당 서비스에서 필요한 포맷이기 때문에 서비스단에서 재가공 해서 넘기겠다는 건데요..

서비스 단에서도 원본 데이터를 보호해야 하는 이유가 뭐가 있나요?

2010년 10월 28일 오후 5:44, Sanghyuk Jung <ben...@gmail.com>님의 말:

[임성현]

의견주신 분들 감사합니다.

결국 보안에도 문제있고(참조하는 원본이 변경될 수 있고),
Loop에 들어가자마자 바로 다른 객체를 참조하는데 내부에서든 외부에서든 생성할
가치가 없다 라는 측면으로 이해 됩니다.

성철님의 말씀(내가 처리를 요청한 원본이 의도하지 않게 변해버린다)라는 측면은
좀 더 진지하게 고민해볼께요. 어떻게 사용되는지.. 좀 더 살펴봐야겠습니다.

원래의도인 룰 위반을 하는지 여부 보다는
어떤 이유로 이런 형식의 코딩을 하는지 지켜보는 용도로 써야겠군요.

한시간도 안되어서 이렇게 뜨거운. 호응을 받으니.
감사 합니다.

> > 그룹에서 탈퇴하려면 ksug+uns...@googlegroups.com<ksug%2Bunsu...@googlegroups.com>로

[Sungchul Park]

> 원본을 변조하지 않으려면 new나 clone 같은 것이 당연히 필요하겠죠.
> 그렇게 하려면 아래와 같이 코드가 바뀌어야할 것 같습니다.
> while(itr.hasNext()){
> //리스트에서 하나씩 꺼내서 가공함
> AbcDVO source = (AbcDVO) itr.next();
> AbcDVO dvo = new AbcDVO (source.getId().....
> 등등 초기화 값);
> dvo.setLoginEpno(session.getLognEpno());
> dvo.setScnId(session.getScnId());
> dvo.setLastChgPsEpno(session.getEpno());
> returnList.add(dvo);
> }
> return returnList;
>
> 그리고 이 경우에도 new 생성말고 dvo 선언 자체를 밖으로 빼는 것은 아무
> 성능에 이득이 없습니다. 변수범위를 최소화하는 것이 좋겠죠.

네. 동의합니다. 두번째 글에 그렇게 썼습니다. 전 clone을 사용했습니다.

애초에 PMD의 경고가 반복문 안에서 객체를 생성하는 게 옳은지 여부인데 사
실 올리신 소스에서는 애초에 객체 생성이 무의미 하기 때문에 객체 생성이
반복문 안에서 의미있게 사용되는 사례로 좀 바꾸어 본 것입니다. 논점이 소
스의 문제로 바뀐 것 같아서요.
임성현님이 토론하기 원하시는 것은 루프안에서 객체를 생성함으로써 PMD의
경고를 받았는데 PMD의 경고를 무시해도 좋은지, 아니면 PMD의 지침을 따라
변경해야 하는지 같습니다.

[Sungchul Park]

> 처음 질문을 보면.. 서비스 단인데 굳이 원본을 복사할 필요가 있을까요?

원본이 변할 수 있다는 전제가 있다면 복사할 필요가 없습니다. 호출하는 쪽
에서 그것을 미리 고려하고 있을테니까요. 시스템이 복잡해지면 이런 선택이
문제를 일으킬 수 있지만 웹은 단순하고 객체의 생애가 짧기 때문에 대부분
문제가 생길 일이 없겠죠.

하지만 단순한 DTO가 아닌 ORM의 영속화된 객체라면 고민해야 할 부분입니다.

그리고 제가 원본을 복사하는 버전으로 바꾼 이유는 임성현님의 질문 의도를
더 부각하려고 한 것 뿐입니다.

[JUNG-LAE JO]

제가 너무 질문에 요지만 보려고 단순하게 생각했나 봅니다 ㅋ;

하긴 서비스가 다른 서비스를 가져다 쓰고 하면, 원본 데이터에 대한 확신을 가지고 싶겠네요. 그럼 말씀하신대로 복사해서 사용해야 겠지요. 

지적 감사드립니다 :)

2010년 10월 28일 오후 6:09, Sungchul Park <gyu...@gmail.com>님의 말: