현재 구현 중이던 쇼핑몰 코드의 ItemImgService 의 구현 로직을 살펴보던 중 중복 코드와 하나의 함수에 두 가지의 책임을 가진 것을 보고 리팩터링의 필요성이 느껴져서 수정하였습니다.
기존 코드
public class ItemImgService {
private final ItemImgRepository itemImgRepository;
private final FileProperties fileProperties;
private final FileService fileService;
public void itemImgRegister(Item item, List<MultipartFile> itemImgFiles) throws IOException {
ItemImg itemImg = new ItemImg(item);
for(int i=0; i<itemImgFiles.size(); i++){
if( i == 0)
itemImg.RepImg();
else
itemImg.SubImg();
saveItemImg(itemImg, itemImgFiles.get(i));
}
}
public List<ItemImg> itemImgUpdate(Item item, List<MultipartFile> itemImgFiles) throws IOException {
List<ItemImg> itemImgs = itemImgRepository.findAllByItemId(item.getId());
List<ItemImg> updateItemImgs = new ArrayList<>();
for(int i=0; i<itemImgFiles.size(); i++) {
updateItemImgs.add(updateItemImg(item, itemImgs.get(i), itemImgFiles.get(i)));
}
return updateItemImgs;
}
private ItemImg updateItemImg(Item item, ItemImg itemImg, MultipartFile itemImgFile) throws IOException {
validateItemImgFile(itemImgFile);
clearPreviousItemImgFile(itemImg);
itemImg = validateItemImg(item, itemImg);
String oriImgName = itemImgFile.getOriginalFilename();
String imgName = createImgName(itemImgFile, oriImgName);
String imgUrl = "/images/item/" + imgName;
itemImg.updateItemImg(oriImgName, imgName, imgUrl);
return itemImg;
}
private ItemImg validateItemImg(Item item, ItemImg itemImg) {
if(itemImg == null){
itemImg = new ItemImg(item);
itemImgRepository.save(itemImg);
}
return itemImg;
}
private void validateItemImgFile(MultipartFile itemImgFile) {
if(itemImgFile.isEmpty())
throw new ItemImgFileNotFoundException();
}
private void clearPreviousItemImgFile(ItemImg itemImg) {
if(!StringUtils.isEmpty(itemImg.getImgName())){
fileService.deleteFile(fileProperties.getItemImgLocation()
+ "/" + itemImg.getImgName());
}
}
private void saveItemImg(ItemImg itemImg, MultipartFile itemImgFile) throws IOException {
String oriImgName = itemImgFile.getOriginalFilename();
String imgName = "";
String imgUrl = "";
if(!StringUtils.isEmpty(oriImgName)){
imgName = createImgName(itemImgFile, oriImgName);
imgUrl = "/images/item/" + imgName;
}
itemImg.updateItemImg(oriImgName, imgName, imgUrl);
itemImgRepository.save(itemImg);
}
private String createImgName(MultipartFile itemImgFile, String oriImgName) throws IOException {
return fileService.uploadFile(fileProperties.getItemImgLocation()
, oriImgName, itemImgFile.getBytes());
}
public List<ItemImg> getItemImgs(Long itemId) {
return itemImgRepository.findAllByItemId(itemId);
}
}
이 코드들 중에서 먼저 두 함수 updateItemImg, saveItemImg 함수에서 코드 중복이 보였습니다.
updateItemImg
saveItemImg
두 함수 모두 이미지 파일에서 이름을 추출하고 itemImg 인스턴스의 정보를 수정하는 로직입니다. 차이는 중간에 검증이 있고 없고 인데. 검증을 하는 함수를 이미 만들어 놨기 때문에 이를 saveItemImg 함수의 제일 앞에 사용하도록 했습니다.
나머지는 동일한 코드로 보여서 하나의 함수(nameExtractionInformationRegistration)로 추출했습니다.
따로 변수로 빼지 말고 한 줄로 깔끔하게 작성하는게 좋을까 싶지만 가독성이 과연 좋은가? 하는 의문이 들어서 변수를 유지했습니다.
또 itemImg 인스턴스의 유효성을 검증하는 로직인 validateItemImg 함수를 봤을 때, 하나의 함수에 검증과 객체 생성 후, 저장이라는 여러가지의 책임을 가진 것이 문제로 보였습니다.
private ItemImg validateItemImg(Item item, ItemImg itemImg) {
if(itemImg == null){
itemImg = new ItemImg(item);
itemImgRepository.save(itemImg);
}
return itemImg;
}
재사용성 측면에서 봤을 때, itemImg 객체가 item의 정보를 받아서 인스턴스 생성 후 저장하는 함수를 만들 경우 자주 사용할 수 있을 것으로 보아. null 값을 확인하는 것은 그대로 두고 저장하는 함수만 따로 구현 했습니다.
저장 함수
수정 코드
public class ItemImgService {
private final ItemImgRepository itemImgRepository;
private final FileProperties fileProperties;
private final FileService fileService;
public void itemImgRegister(Item item, List<MultipartFile> itemImgFiles) throws IOException {
ItemImg itemImg = new ItemImg(item);
for(int i=0; i<itemImgFiles.size(); i++){
if( i == 0)
itemImg.RepImg();
else
itemImg.SubImg();
saveItemImg(itemImg, itemImgFiles.get(i));
}
}
public List<ItemImg> itemImgUpdate(Item item, List<MultipartFile> itemImgFiles) throws IOException {
List<ItemImg> itemImgs = itemImgRepository.findAllByItemId(item.getId());
List<ItemImg> updateItemImgs = new ArrayList<>();
for(int i=0; i<itemImgFiles.size(); i++) {
updateItemImgs.add(updateItemImg(item, itemImgs.get(i), itemImgFiles.get(i)));
}
return updateItemImgs;
}
@Transactional(readOnly = true)
public List<ItemImg> getItemImgs(Long itemId) {
return itemImgRepository.findAllByItemId(itemId);
}
private ItemImg updateItemImg(Item item, ItemImg itemImg, MultipartFile itemImgFile) throws IOException {
validateItemImgFile(itemImgFile);
clearPreviousItemImgFile(itemImg);
if(Objects.isNull(itemImg)){
itemImg = saveItemImg(item);
}
nameExtractionInformationRegistration(itemImg, itemImgFile);
return itemImg;
}
private ItemImg saveItemImg(Item item) {
return itemImgRepository.save(new ItemImg(item));
}
private void saveItemImg(ItemImg itemImg, MultipartFile itemImgFile) throws IOException {
validateItemImgFile(itemImgFile);
nameExtractionInformationRegistration(itemImg, itemImgFile);
itemImgRepository.save(itemImg);
}
private void validateItemImgFile(MultipartFile itemImgFile) {
if(itemImgFile.isEmpty())
throw new ItemImgFileNotFoundException();
}
private void clearPreviousItemImgFile(ItemImg itemImg) {
if(!StringUtils.isEmpty(itemImg.getImgName())){
fileService.deleteFile(fileProperties.getItemImgLocation()
+ "/" + itemImg.getImgName());
}
}
private String uploadItemImgFile(MultipartFile itemImgFile, String oriImgName) throws IOException {
return fileService.uploadFile(fileProperties.getItemImgLocation()
, oriImgName, itemImgFile.getBytes());
}
private void nameExtractionInformationRegistration(ItemImg itemImg, MultipartFile itemImgFile) throws IOException {
String oriImgName = itemImgFile.getOriginalFilename();
String imgName = uploadItemImgFile(itemImgFile, oriImgName);
String imgUrl = "/images/item/" + imgName;
itemImg.updateItemImg(oriImgName, imgName, imgUrl);
}
}중간 중간 만족스럽지 못한 코드들이 보여서 수정하고 싶은 부분들이 남아있습니다. 이걸 수정하고 따로 함수를 만드는게 좋은지에 대한 의문도 있었고 변수명 이름 짓는 것에 대한 어려움이 있었습니다. 또 다른 사람들과의 피드백이 필요하게구나 느끼게 되는 시간이었습니다.
장생농씬가?