리팩터링 제안

[Tarakyu]

여기서 제안되는 모든 사항은 당연히 틀린 내용이 있을 수 있으니 더블 체크가 필요하다.. ㅎㅎ

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/SessionConst.java

Lines 1 to 9 in 5aed484

	package com.github.twentiethcenturygangsta.adminboard;
	public class SessionConst {
	public static final String LOGIN_MEMBER = "loginMemberId";
	public static final String LOGIN_MEMBER_CREATE_AUTHORITY = "loginMemberCreateAuthority";
	public static final String LOGIN_MEMBER_CREATE_ADMIN_AUTHORITY = "loginMemberCreateAuthority";
	public static final String LOGIN_MEMBER_UPDATE_AUTHORITY = "loginMemberUpdateAuthority";
	public static final String LOGIN_MEMBER_DELETE_AUTHORITY = "loginMemberDeleteAuthority";
	}

public 유틸 클래스는 유저가 인스턴스화 하는 것을 막기 위해 프라이빗 생성자를 추가하는 것이 좋음. 혹은 enum으로 만드는 것도 가능하다. (이펙티브 자바 아이템 22, p140)
AdminBoardStringConvertUtil에는 적용한 걸 보니 아마 깜빡한 듯하다.

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/entity/EntityInfo.java

Lines 57 to 66 in 5aed484

	private AdminBoardEntity getAdminBoardEntityAnnotation(Class<?> object) {
	List<Annotation> annotations = Arrays.asList(object.getAnnotations());
	List<AdminBoardEntity> annotationsWithAdminBoardEntity = annotations.stream().filter(ob -> ob instanceof AdminBoardEntity)
	.map(ob -> (AdminBoardEntity) ob)
	.collect(Collectors.toList());
	if (annotationsWithAdminBoardEntity.size() == 1) {
	return annotationsWithAdminBoardEntity.get(0);
	}
	throw new RuntimeException("Not AdminBoardEntity");
	}

리스트를 스트림 돌면서 특정 조건을 만족하는 친구를 리턴하고, 존재하지 않는 경우에는 에러를 반환하고 싶은 경우라면

    private AdminBoardEntity getAdminBoardEntityAnnotation(Class<?> object) {
        List<Annotation> annotations = Arrays.asList(object.getAnnotations());
        return annotations.stream().filter(AdminBoardEntity.class::isInstance)
                .map(ob -> (AdminBoardEntity) ob).findAny().orElseThrow(() -> new RuntimeException("Not AdminBoardEntity"));
    }

와 같이 findAny().orElseThrow()를 활용 가능하다. 그런데 특정 어노테이션을 찾는 것이 목적이라면 다음 코드를 더욱 권장한다.

    private AdminBoardEntity getAdminBoardEntityAnnotation(Class<?> clazz) {
        AdminBoardEntity annotation = clazz.getAnnotation(AdminBoardEntity.class);
        if (annotation == null) {
            throw new RuntimeException("Not AdminBoardEntity");
        }
        return annotation;
    }

Class<?> 타입의 변수 이름으로 object를 사용하면 헷갈릴 여지가 있어 clazz로 변경하였다. (나도 첨에 코드 보고 Object에 getAnnotations 메서드가 있었나? 라고 생각함)
그리고 덧붙여 RuntimeException을 그대로 리턴하는 것보다는, 직접 구현한 어플리케이션 예외를 만들어서 이를 던지는 것이 바람직하다. (이펙티브 자바 아이템 72, p397)

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/AdminBoardFactory.java

Lines 59 to 70 in 2e46808

	public Map<String, List<EntityInfo>> getGroupEntities() {
	Map<String, List<EntityInfo>> entitiesByGroup = new HashMap<>();
	List<EntityInfo> entityInfos = entityClient.getEntities();
	for (EntityInfo entityInfo : entityInfos) {
	String group = entityInfo.getGroup();
	List<EntityInfo> groupEntities = entitiesByGroup.getOrDefault(group, new ArrayList<>());
	groupEntities.add(entityInfo);
	entitiesByGroup.put(group, groupEntities);
	}
	return entitiesByGroup;
	}

스트림을 활용해서 간단히 할 수 있다. 스트림을 잘 활용하면 코드 가독성을 높일 수 있다. (동욱이 형도 좋아함)

    public Map<String, List<EntityInfo>> getGroupEntities() {
        List<EntityInfo> entityInfos = entityClient.getEntities();
        return entityInfos.stream()
                          .collect(Collectors.groupingBy(EntityInfo::getGroup));
    }

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/entity/EntityInfo.java

Lines 46 to 55 in 8fa840c

	private List<ColumnInfo> getObjectColumns(Class<?> object) {
	List<ColumnInfo> columns = new ArrayList<>();
	for (Field field : object.getDeclaredFields()) {
	if (!isStaticField(field)) {
	ColumnInfo column = ColumnInfo.builder().field(field).build();
	columns.add(column);
	}
	}
	return columns;
	}

위 코드도 스트림으로 간단히 할 수 있다.

    private List<ColumnInfo> getObjectColumns(Class<?> object) {
        return Arrays.stream(object.getDeclaredFields())
                     .filter(field -> !isStaticField(field))
                     .map(field -> ColumnInfo.builder().field(field).build())
                     .collect(Collectors.toList());
    }

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/view/AdminBoardViewController.java

Lines 115 to 118 in 5aed484

	@GetMapping("/login")
	public String LoginView(@ModelAttribute("login") LoginRequestDto loginRequestDto) {
	return "login";
	}

컨트롤러 코드에 메서드 이름이 대문자로 시작하는 경우가 많았는데, 의도한 건가..? 의도하지 않았다면 소문자로 시작하도록 고치는 것이 좋겠다. (만약 어떤 이유에 따라 의도한 거라면, 소문자로 시작하는 애들도 있던데 통일하는 것이 좋아보인다.)

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/view/AdminBoardViewController.java

Lines 102 to 107 in 5aed484

	Object returnObject = null;
	Optional<Object> object = adminBoardFactory.getObject(entityName, id);
	if (object.isPresent()) {
	returnObject = object.get();
	}
	model.addAttribute("object", returnObject);

다음과 같이 간단히 할 수 있다.

        Object returnObject = adminBoardFactory.getObject(entityName, id).orElse(null);
        model.addAttribute("object", returnObject);

근데 이렇게 값이 존재하지 않는 경우 null을 할당할 것이라면 adminBoardFactory.getObject()가 Optional을 반환하는 의미가 없어진다. null을 반환하면 안 되는 상황이면 예외처리를 하고, 그렇지 않다면 뭔가 디폴트값을 넣어주는 것이 좋겠다.
그리고 이 코드랑 별개로 null을 리턴하는 메서드가 많이 보였는데, 될 수 있으면 최대한 지양하는 것이 좋다.
null이 나오면 안 되는 상황이면 예외를 던지고, 그렇지 않다면 적절한 디폴트값을 반환하거나, Optional을 반환해 그 처리의 책임을 호출하는 쪽에 넘길 수 있다. null은 만악의 근원!

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/view/AdminBoardViewController.java

Lines 136 to 139 in 5aed484

	Boolean createAuthority = (Boolean) session.getAttribute(SessionConst.LOGIN_MEMBER_CREATE_AUTHORITY);
	if(!createAuthority) {
	return "redirect:/admin-board/" + entityName;
	}

if문 안에 Boolean이 들어가면 NPE 가능성이 있다. boolean이 아니어야 할 이유가 없다면 boolean을 사용하고, 꼭 Boolean을 사용해야한다면 다음과 같이 if문을 작성하자. (이펙티브 자바 아이템 61)

        Boolean createAuthority = (Boolean) session.getAttribute(SessionConst.LOGIN_MEMBER_CREATE_AUTHORITY);
        if(Boolean.FALSE.equals(createAuthority)) {
            return "redirect:/admin-board/" + entityName;
        }

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/repository/RepositoryClient.java

Lines 29 to 44 in ac687ae

	public void registerRepositories() {
	List<Class<?>> repositories = new ArrayList<>();
	List<String> basePackages = AdminBoardRegistrar.getBasePackages();
	for(String basePackage : basePackages) {
	repositories.addAll(new Reflections(basePackage).getTypesAnnotatedWith(Repository.class));
	}
	log.info("repositories = {}", repositories);
	for (Class<?> object : repositories) {
	DefaultRepositoryMetadata metadata = new DefaultRepositoryMetadata(object);
	RepositoryInfo repositoryInfo = setRepository(metadata);
	this.repositories.put(repositoryInfo.getDomainName(), repositoryInfo);
	}
	}

1. 객체 생성할 때만 필드를 초기화할 때 쓰는 메서드인 것 같은데, 그렇다면 private 메서드로 두고 생성자에서 호출하는 게 나을 수도 있다. (아님 ㅈㅅ)
   EntityClient의 경우에도 같다.
2. 클래스가 repositories라는 필드를 이미 가지고있는데 메서드 안에서 같은 변수명으로 로컬변수를 만들어서 코드를 읽는 중간에 헷갈릴 수 있다.
3. 스트림으로 간단히 할 수 있다. (이건 좀 과할지도?? 근데 어느정도는 활용해주는 게 가독성을 높여준다.)

    public void registerRepositories() {
        AdminBoardRegistrar.getBasePackages().stream()
                           .flatMap(basePackage -> new Reflections(basePackage).getTypesAnnotatedWith(Repository.class).stream())
                           .map(DefaultRepositoryMetadata::new)
                           .map(this::setRepository)
                           .forEach(repositoryInfo -> this.repositories.put(repositoryInfo.getDomainName(), repositoryInfo));

    }

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/repository/RepositoryClient.java

Lines 50 to 56 in 5aed484

	public RepositoryInfo getRepository(String domainName) {
	try {
	return repositories.get(domainName);
	} catch (NullPointerException ex) {
	throw new RuntimeException("Not Exist Repository");
	}
	}

repositories를 항상 초기화해주므로 NPE는 domainName이 null일 때만 발생한다. 근데 "Not Exist Repository"라고 예외 메시지를 작성한 것을 보면 domainName에 해당하는 매핑값이 없는 경우를 처리하려는 의도같은데, 그렇다면 그 경우엔 NPE가 발생하는 것이 아니라 null이 반환되기 때문에 이를 처리해주면 된다.

    public RepositoryInfo getRepository(@NonNull String domainName) {
        RepositoryInfo repositoryInfo = repositories.get(domainName);
        if (repositoryInfo == null) {
            throw new IllegalArgumentException("Not Exist Repository");
        }
        return repositoryInfo;
    }

lombok의 @NonNull 어노테이션을 추가하여 호출하는 쪽에서 null을 보내면 안 된다는 것을 더 쉽게 알 수 있도록 하였고, RuntimeException 대신 IllegalArgumentException을 발생시켰다. Assert.notNull()을 사용해도 된다. 쓰고 보니 이쪽이 나을 것 같기도..

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/entity/ColumnInfo.java

Lines 56 to 61 in c3d86fe

	private String getFieldType(Field field) {
	if (field.isAnnotationPresent(OneToMany.class) || field.isAnnotationPresent(ManyToMany.class)) {
	return getListItemType(field).getSimpleName();
	}
	return field.getType().getSimpleName();
	}

getListItemType(field)가 null을 반환할 수 있으니 방어 로직을 짤 필요가 있어 보인다. 혹은 getListItemType(field)가 null을 반환하지 않도록 수정할 수도 있다.

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/AdminBoardFactory.java

Line 126 in 5aed484

public HashMap<String, String> getAdminBoardInfo() {

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/AdminBoardFactory.java

Line 138 in 2e46808

public void createObject(String entityName, HashMap<String, Object> data) {

유연성을 위해 파라미터와 반환타입으로서 HashMap보다 Map을 사용하는 것이 적절하다.

Program to an interface, not an implementation

그리고 createObject() 메서드가 꽤 긴데, 두 부분으로 나눌 수 있을 것 같다.

---

사실 웹서버 코드가 아니라서 구조에 대한 코멘트는 못 했는데,, 이건 면접관도 비슷할 것 같다. 아마 코드레벨에서 많이 보게되지 않을까?
코드레벨에서는 Stream과 Optional, null 방어를 조금 더 고려하면 개선될 수 있겠다는 생각이 들었다.
제너릭도 조금 개선할 수 있을 것 같은데 잘 몰라서 코멘트를 못했다.. ㅎㅎ 나도 공부해야겠다.
코드를 보니 생각보다 더 복잡하고 어려운 구현이었겠다. 대단쓰~~

[oereo]

---

admin-board/src/main/java/com/github/twentiethcenturygangsta/adminboard/SessionConst.java

Lines 1 to 9 in 5aed484

	package com.github.twentiethcenturygangsta.adminboard;
	public class SessionConst {
	public static final String LOGIN_MEMBER = "loginMemberId";
	public static final String LOGIN_MEMBER_CREATE_AUTHORITY = "loginMemberCreateAuthority";
	public static final String LOGIN_MEMBER_CREATE_ADMIN_AUTHORITY = "loginMemberCreateAuthority";
	public static final String LOGIN_MEMBER_UPDATE_AUTHORITY = "loginMemberUpdateAuthority";
	public static final String LOGIN_MEMBER_DELETE_AUTHORITY = "loginMemberDeleteAuthority";
	}

public 유틸 클래스는 유저가 인스턴스화 하는 것을 막기 위해 프라이빗 생성자를 추가하는 것이 좋음. 혹은 enum으로 만드는 것도 가능하다. (이펙티브 자바 아이템 22, p140)
AdminBoardStringConvertUtil에는 적용한 걸 보니 아마 깜빡한 듯하다.

---

혹시 이 부분에 대해서 Class를 abstract로 만들면 괜찮을까?? 인스턴스화 막는 것에 대해서 동의합니다!

[Tarakyu]

@oereo
가능한 선택지이긴 한데 이펙티브 자바 저자는 상속의 우려가 있어 권장하지는 않나봐

(아이템19: 상속을 고려해 설계하고 문서화하라. 그러지 않았다면 상속을 금지하라.)