[리팩토링]GliderWiki – 공간저장 리팩토링

이제 출품과 심사가 모두 끝나고 시간날때 하나씩 내가 만든 코드를 리팩토링 하기로 했다.

참고로 글라이더 위키의 사이트와 소스는 아래를 참고하면 된다.

기존에는 서비스단의 저장메서드에 여러가지 기능들이 절차지향적으로 혼재되어 있었다면

이번 리팩토링의 가장 큰 목적은 각 기능별로 메서드를 분리하는 것이었다.

공간쪽 프로세스는 크게 3가지로 나눌 수 있다.

  • 공간데이터 저장
  •  권한 관련 데이터처리
  • 공간이미지관련 파일copy, 데이터처리

우선 리팩토링 전 소스를 보면..

WeSpace addSpaceInfo = new WeSpace(weSpace.getWe_space_name(), weSpace.getWe_space_desc(),
				weSpace.getWe_space_exposed(), weSpace.getWe_view_privacy(), weSpace.getWe_edit_privacy(), weUserIdx,
				weUserId);
		entityService.insertEntity(addSpaceInfo);

		// 생성, 수정권한에 대한 DB처리
		WeSpace afterSpaceInfo = (WeSpace) entityService.getRowEntity(addSpaceInfo);

		if (!weSpace.getWe_view_privacy().equals(AuthorityType.ALLGROUP)) {
			authorityTypesProcess(VIEW_AUTHORITY, weSpace.getWe_view_data(), afterSpaceInfo.getWe_space_idx(),
					afterSpaceInfo.getWe_view_privacy());
		}

		if (!weSpace.getWe_edit_privacy().equals(AuthorityType.ALLGROUP)) {
			authorityTypesProcess(EIDT_AUTHORITY, weSpace.getWe_edit_data(), afterSpaceInfo.getWe_space_idx(),
					afterSpaceInfo.getWe_edit_privacy());
		}

		// 프로필관련 DB처리
		if (!StringUtils.isEmpty(weSpace.getWe_upload_imgName())) {
			ImageInfo imgInfo = processImage(imageInfo, weSpace.getWe_upload_imgName());
			String imgPath = weSpace.getWe_upload_imgName().substring(0,
					weSpace.getWe_upload_imgName().lastIndexOf('/'));
			String imgName = weSpace.getWe_upload_imgName().substring(
					weSpace.getWe_upload_imgName().lastIndexOf('/') + 1, weSpace.getWe_upload_imgName().length());
			WeSpaceImage weSpaceImage = new WeSpaceImage(afterSpaceInfo.getWe_space_idx(), imgPath, imgName,
					imgInfo.getWidthSize(), imgInfo.getHeightSize());
			entityService.insertEntity(weSpaceImage);
		}

각 기능들이 하나의 메소드안에 모두 들어가있고, 절차지향적으로 만들어져 있어서

가독성이 매우 떨어짐을 알 수 있다.

리팩토링 후의 소스를 다시 보면..

public void create(WeSpace weSpace, Integer userIdx, ImageInfo imageInfo) throws Throwable {
		WeSpace addSpaceInfo = new WeSpace(weSpace.getWe_space_name(), weSpace.getWe_space_desc(),
				weSpace.getWe_space_exposed(), weSpace.getWe_view_privacy(), weSpace.getWe_edit_privacy(), userIdx,
				userIdx);
		entityService.insertEntity(addSpaceInfo);

		// 생성, 수정권한에 대한 DB처리
		WeSpace savedSpaceInfo = (WeSpace) entityService.getRowEntity(addSpaceInfo);
		savedSpaceInfo.setWe_view_data(weSpace.getWe_view_data());
		savedSpaceInfo.setWe_edit_data(weSpace.getWe_edit_data());
		savedSpaceInfo.setWe_upload_imgName(weSpace.getWe_upload_imgName());
		processAuthorityTypes(savedSpaceInfo);

		// 프로필관련 DB처리
		processSpaceImage(savedSpaceInfo, imageInfo);
	}

우선 공간데이터 저장이외 나머지 부분을 개별메서드로 분리함을 확인할 수 있으며,

그에 따라 코드 가독성이 훨씬 좋아졌음을 알 수 있다.

이 중에서 권한을 처리하는 processAuthorityTypes()메서드는 디자인패턴 중

전략패턴“을 사용하여 리팩토링을 진행했다.


public interface AuthorityProcessor {
	Map<String, Object> getPrivacyData();
}

public void processAuthorityTypes(final WeSpace weSpace) throws Throwable {

	if (weSpace.isAllGroupViewPrivacy()) {
		AuthorityProcessor processor = new AuthorityProcessor() {

			@Override
			public Map<String, Object> getPrivacyData() {
				Map<String, Object> result = Maps.newHashMap();

				result.put("authorityType", weSpace.getWe_view_privacy());
				result.put("authorityData", weSpace.getWe_view_data());
				result.put("authorityPrivacy", VIEW_AUTHORITY);

				return result;
			}
		};

		divideFromAuthrityData(weSpace.getWe_space_idx(), processor);
	}

	if (weSpace.isAllGroupEditPrivacy()) {
		AuthorityProcessor processor = new AuthorityProcessor() {

			@Override
			public Map<String, Object> getPrivacyData() {
				Map<String, Object> result = Maps.newHashMap();

				result.put("authorityType", weSpace.getWe_edit_privacy());
				result.put("authorityData", weSpace.getWe_edit_data());
				result.put("authorityPrivacy", EDIT_AUTHORITY);

				return result;
			}
		};

		divideFromAuthrityData(weSpace.getWe_space_idx(), processor);
	}

}

리팩토링을 하면서 아래 그림에서 보다 싶이 가장 윗단의 조회/수정 정책에서 정해서

내려오면 될 것을,  그룹이나 구성원으로 내려갔다가 다시 조회/수정 정책으로 분기하는

코드를 떼어낼 수 있었다.

우선 추상화를 통해 권한처리 중에 다르게 동작하는 부분을 인터페이스로 분리하고,

다르고 동작하는 부분을 익명클래스의 메서드안에 구현함으로써 리팩토링을 진행했다.

그리고, 공통으로 처리해야할 부분은 템플릿 메서드로 빼내서 다르게 동작하는 부분의

객체를 템플릿 메서드로 전달해서 공통으로 수행해야할 동작을 처리했다.

구현하면서 오버엔지니어링이라는 생각이 들었고, 실제로도 리팩토링을 다 하고 난 뒤의

결과를 보니, 이 정도로 리팩토링을 할 사항은 아닌것 같다는 생각이 들었다.

하지만 의미없게 넘기던 메서드의 파라미터는 최소한 제거할 수 있었음에 소기의 목적을

이룰 수 있었다.

한 가지 아쉬운 점은 리팩토링을 하면서 추상화를 통해, 이게 진정 맞는 추상화인가라는

의문이 계속 들었다는 점이다. 내가 해당 디자인패턴에 끼워맞추기 위해 추상화를

한 것이 아닐까라는 의구심이 생긴것이다.

그렇다면 앞서 말한 오버엔지니어링을 깊게 의심해 볼 수 있을 것이다.

리팩토링을 하는 것도 중요하지만, 오버엔지니어링이 되는 시점을 아는 것도

참 중요한 일이 아닐까 생각한다.

그리고 아래는 이번에 리팩토링한 저장쪽의 테스트코드이다.

@Mock
private EntityService entityService;

@Mock
private EntityDao entityDao;

@Mock
private FileService fileService;

@Mock
SpaceDao spaceDao;

@Spy
private SpaceService spaceService = new SpaceService();

@Captor
private ArgumentCaptor<WeSpace> weSpaceCaptor;

private WeSpace weSpace;

@Before
public void setup() {
	weSpace = new WeSpace("테스트공간", "공간설명", "Y", AuthorityType.ALLGROUP, AuthorityType.USER, 1, 1);
	injector(spaceService).set(entityService).set(spaceDao).set(fileService);
}

@Test
public void create() throws Throwable {
	// 공간정보를 DB에 저장한다
	ImageInfo imageInfo = new ImageInfo("/temp", "/real");
	WeSpace savedSpace = new WeSpace();
	savedSpace.setWe_space_idx(1);
	weSpace.setWe_view_data("");
	weSpace.setWe_edit_data("1,2");
	weSpace.setWe_upload_imgName("/upload/test.jpg");

	when(entityService.getRowEntity(weSpaceCaptor.capture())).thenReturn(savedSpace);

	spaceService.create(weSpace, 1, imageInfo);

	verify(entityService).insertEntity(weSpaceCaptor.getValue());

	// 조회와 수정정책에 따라 관련된 데이터를 DB에 저장한다
	verify(spaceService).processAuthorityTypes(savedSpace);

	// 공간이미지파일을 지정된 위치에 복사하고, 이미지 정보를 DB에 저장한다
	verify(spaceService).processSpaceImage(savedSpace, imageInfo);

	WeSpace ws = weSpaceCaptor.getValue();

	assertThat(ws.getWe_space_name(), is("테스트공간"));
	assertThat(ws.getWe_space_desc(), is("공간설명"));
	assertThat(savedSpace.getWe_edit_data(), is("1,2"));
	assertThat(savedSpace.getWe_upload_imgName(), is("/upload/test.jpg"));
}

기존에 있던 테스트코드를 모두 지우고, 다시 만들면서 느낀 점은

  • 테스트코드가 없는 코드를 리팩토링하는건 정말 힘들다.(테스트하기가 너무~ 힘듦)
  • 테스트코드는 개발자가 꼭 해야할 항목!

이다. 특히나 ArgumentCaptor에 대한 개념을 잡는것도 큰 도움이 되었다.

원본코드에서 새로 생성한 객체가 메서드의 인자로 넘어갈 때, 테스트코드와의

객체생성시점이 달라서 메서드의 인자에 대한 테스트를 어떻게 할까 고민했는데,

Mockito에 저 놈이 딱 있지 않은가?

===================================================

이제 하나씩 리팩토링하면서, 그 코드를 테스트코드로 단단히 묶어놓으면

코드품질이 확 올라가는 서비스를 만들어낼 수 있을것이라 확신하며..

리팩토링과 추상화… 그리고 테스트코드는 계속 된다!

Advertisements

[리팩토링]GliderWiki – 공간저장 리팩토링”에 대한 3개의 생각

  1. 헬로우. 용훈님. 잘 지내죠? 이 작업에 대한 피드백입니다.
    만나서 하면 더 좋겠지만, 글로 최대한 남겨봅니다.

    1. 메서드 이름 더 정확하게
    process***() 류의 이름. 의미가 약합니다. 메서드가 뭘 하는 지 더 구체적으로 이름을 정해주세요.
    AuthorityProcessor도 동일하구요. 뭔가 데이터를 생성해주는 애 같은데,
    이름은 역할과 다른 듯 합니다.

    2. create() 메서드의 코드 추상화 레벨
    각 코드의 추상화 레벨이 다름. 아래와 같이 동일시키면 좋을 듯.

    WeSpace newSpaceInfo = createSpaceInfo(….);
    WeSpace savedSpaceInfo = saveSpaceInfo(newSpaceInfo);
    processAuthorityTypes(savedSpaceInfo);
    processSpaceImage(….);

    3. 불필요한 주석

    예를 들어, create() 메서드의 아래 코드에서 주석 불필요. 주석보단 메서드 이름을 더 구체적으로 표시하는 게 좋을 듯 합니다.

    // 프로필관련 DB처리
    processSpaceImage(savedSpaceInfo, imageInfo);

    4. AuthorityProcessor의 정체성

    weSpace의 내부 상태에 따라서 뭔가 다른 데이터를 생성하고,
    그 데이터를 divideFromAuthorityData() 메서드에서 사용하는 것 같은데요,
    divideFromAuthorityData() 메서드의 이름이 다소 느낌이 안 옵니다.
    (AuthorityData로부터 분리/분할/나눈다?)

    하고 싶은 말은 processAuthorityTypes() 메서드의 코드 구성이 아래와 같은 데요

    if (weSpace.조건) {
    divideFromAuthorityData(weSpace.id, 데이터제공);
    }
    if (weSpace.다른조건) {
    divideFromAuthorityData(weSpace.id, 다른데이터제공);
    }

    위와 같다면, 그냥 weSpace.divdeFromAuthorityData(); 로 끝내는 게 더 좋을 것 같습니다.
    기반 기술의 제약 때문에 weSpace에 해당 기능을 넣을 수 없다면,
    weSpace.getViewPrivacyData(), weSpace.getEditPrivacyData()를 만들면 될 것 같습니다.

    이 부분은 divideFromAuthorityData()의 정체를 알면 좀더 알맞은 피드백을 드릴 수 있을 것 같아요.

    • 범균님~ 일단 피드백 감사드리구요.

      1~3번은 이해했고, 4번에서 divideFromAuthorityData의 기능은, 예를 들면

      “1,2,3”과 같이 조회정책에서 그룹선택을 한 데이터가 들어왔다면

      “,”로 데이터를 분리해서 1,2,3 각각을 공간의 그룹정보 DB에 인서트하는 작업을

      하게 됩니다.

      그리고 조회정책과 수정정책 각각 그룹과 구성원정보를 선택적으로 DB에

      저장하는 기능을 하게 됩니다.

      변수나 메서드, 클래스 이름 정하는게 참 중요한것 같네요.

      옆에 사전 끼고 되도록 명확한 이름으로 코드 가독성을 높일수 있도록 노력하겠습니다.

      • 아하, divideFromAuthorityData가 두 개의 역할을 하는 군요.
        – weSpace에 있는 어떤 데이터를 콤마로 분리하고
        – 분리된 데이터를 DB에 넣는 것

        이렇다면, 데이터를 갖고 있는 게 weSpace니까,
        weSpace에서 콤마로 구분된 결과물을 제공하는 기능을 구현하구요,

        그 데이터를 divideFromAuthorityData(음, 이제 이름이 바뀌겠죠?)에는 weSpace를 전달받아, weSpace가 제공하는 기능을 사용하면 어떨까요?

        public void 바뀐이름(weSpace) {
        List viewXList = weSpace.getViewXXXXXXX(); // 콤마로 구분해서 List로 제공
        …. 어떤 처리
        List editXList = weSpace.getEditXXXXXXXX();
        … 어떤 처리
        }

        어떤 처리 부분은 동일한 코드로 보이니, 다시 별도 메서드로 분리하면 될 것 같구요.
        AuthorityProcessor 가 실제로 제공하는 데이터는 모두 weSpace로부터 나오는 것들이니,
        AuthorityProcessor를 제거하고, 관련 데이터를 weSpcae에서 직접 가져오도록 변경하도록 수정하면 더 단순해질 것 같습니다.

답글 남기기

아래 항목을 채우거나 오른쪽 아이콘 중 하나를 클릭하여 로그 인 하세요:

WordPress.com 로고

WordPress.com의 계정을 사용하여 댓글을 남깁니다. 로그아웃 /  변경 )

Google+ photo

Google+의 계정을 사용하여 댓글을 남깁니다. 로그아웃 /  변경 )

Twitter 사진

Twitter의 계정을 사용하여 댓글을 남깁니다. 로그아웃 /  변경 )

Facebook 사진

Facebook의 계정을 사용하여 댓글을 남깁니다. 로그아웃 /  변경 )

%s에 연결하는 중