-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat : 학교 홈페이지 리뉴얼로인한 교직원 스크랩 및 스크랩간 직위 추가 #223
Conversation
기존 Disabled 되어있는 StaffScraperTest를 기존 코드 수정하다가 일부만 수정한채 커밋해서 생긴 문제였습니닷!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EmailConverter를 조금 수정하면 좋을것 같은데,
말로 설명하기가 어려워 수정을 조금 해봤습니다~
확인후 본인이 의도한 대로 동작 하는지 점검해줴요~
} | ||
|
||
// Konkuk 도메인 우선 선택 | ||
private static String selectEmail(String[] emails) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인자로 받는 emails가 0개인 일은 없나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재는 빈 공백이라도 입력되도록 구현되어있어서 괜찮다고 넘긴 부분인데, 이 메서드의 입장에서 봤을 때 불확실한 믿을을 가져야 하는거 같아서 검증할 수 있도록 추가하겠습니다!
|
||
public static String convertValidEmail(String email) { | ||
if (email == null || email.isBlank()) { | ||
return ""; // 빈 입력 처리 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수 이름이 convertValidEmail 인 상황인데 "" 이 나오면 이건 ValidEmail로 간주하는 것 일까요?
방어 로직이 있는거 자체는 좋아요!, 다만 호출하는쪽에서 null과 blank가 아닌 경우에 converting을 시도하는것도 좋을수도??
의견입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동의합니다~! 조금 확장해서 EmailSupporter로 만드는게 더 좋을 수 있겠다는 생각이 드네용!
외부에서 null과 blank를 체크하는 메서드를 호출(isNullOrBlank)한 후 결과에 따라 convertEmail 메서드를 통해 변환하도록 구현하는게 조금 더 명확할거 같다고 생각합니다!
이런 방향성이면 PhoneNumberConverter도 수정이 필요할거 같네욤
//여러 이메일인 경우 있으니 분리. | ||
String[] emailGroups = email.split("[/,]"); | ||
//정상 구조가 아닌 경우 구조 정상화 | ||
for (int i = 0; i < emailGroups.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드를 조금 다시 재구성 해봤는데 확인 부탁해요~
public class EmailConverter {
private static final Pattern AT_PATTERN = Pattern.compile("\\s+at\\s+");
private static final Pattern DOT_PATTERN = Pattern.compile("\\s+dot\\s+");
private static final Pattern EMAIL_PATTERN = Pattern.compile("^[a-zA-Z0-9_!#$%&'\\*+/=?{|}~^.-]+@[a-zA-Z0-9.-]+$");
private static final String KONKUK_DOMAIN = "@konkuk.ac.kr";
private static final String EMPTY_EMAIL = "";
public static String convertValidEmail(String email) {
if (isNullOrEmpty(email)) {
return EMPTY_EMAIL;
}
String[] emailGroups = splitEmails(email);
String[] normalizedEmails = normalizeEmails(emailGroups);
return selectPreferredEmail(normalizedEmails);
}
private static boolean isNullOrEmpty(String str) {
return str == null || str.isBlank();
}
private static String[] splitEmails(String email) {
return email.split("[/,]");
}
private static String[] normalizeEmails(String[] emailGroups) {
return Arrays.stream(emailGroups)
.map(EmailConverter::normalizeEmail)
.toArray(String[]::new);
}
private static String normalizeEmail(String email) {
if (isNullOrEmpty(email)) {
return EMPTY_EMAIL;
}
if (EMAIL_PATTERN.matcher(email).matches()) {
return email;
}
if (containsSubstitutePatterns(email)) {
return replaceSubstitutePatterns(email);
}
return EMPTY_EMAIL;
}
private static boolean containsSubstitutePatterns(String email) {
return DOT_PATTERN.matcher(email).find() && AT_PATTERN.matcher(email).find();
}
private static String replaceSubstitutePatterns(String email) {
return email.replaceAll(DOT_PATTERN.pattern(), ".")
.replaceAll(AT_PATTERN.pattern(), "@");
}
private static String selectPreferredEmail(String[] emails) {
return Arrays.stream(emails)
.filter(email -> !email.isBlank())
.filter(email -> email.endsWith(KONKUK_DOMAIN))
.findFirst()
.orElseGet(() -> emails.length > 0 ? emails[0] : EMPTY_EMAIL);
}
}
@zbqmgldjfh 확인 부탁드립니당 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다~
1.생성자 이슈는 무시하고 넘어가시쥬~
2.굳굳!
3.직위는 스크랩은 하되, API 반영은 클라이언트 팀과 직접 논의해보시쥬~
테스트에서 출력문만 제거해주시구~
develop으로 Squash Merge 해주세요~
Approve남겨둘게요~
|
||
//then | ||
for (int i = 0; i < numbers.length; i++) { | ||
System.out.println(numbers[i] + " " +convertedNumbers.get(i) + " " + answer[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 테스트 용도였을 것 같은데? 검증에서 출력을 제외해주세요!~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다~
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
📝작업 내용
1. 스크랩할 원본페이지 선택
스크랩은 모든 학과 페이지의 교수진 기준으로 했습니다.
예시) 컴퓨터공학부 교수진 홈페이지
각 학과/교수에 따라 다를 수 있는 점은 다음과 같습니다.
1. 교수 직위에 따른 차이
2. 수집한 정보 및 제공된 정보에 따른 차이
추가로 기존과 url구조가 변경되었으며 필요 정보가 바뀌었습니다.
siteName
)과 페이지 정보(siteId)를 추가했으며, 학과의 구분없이 사용 가능한baseurl
이라 판단되어 구분해놨던 url을 통합했습니다.(staff.each-dept-url
)www.
사용 가능하나, 이 경우 Jsoup으로 접근 시도시 redirect 횟수 초과 발생(정확한 원인은 찾지 못했으나 관리자 모드 확인 해보니 합니다)2.구현
DeptInfo
,ParserTemplate
,ApiClient
을 변경하여 사용했습니다.DeptInfo
의 학과별 정보를siteName
,siteId
로 변경ParsertTemplate
은 부동산 학과와 그 외 학과를 분리했습니다.(html구조 차이 존재)ApiClient
는 하나로 통합하였습니다. 다만, 사회봉사교육센터의 경우siteId
가 Empty List로 이루어져 있어 에러가 발생하였고, 이를 해결하기 위해 학과 홈페이지에 교수진 정보를 제공하는 지 유무를 체크할 검증 메서드를 하나 추가했습니다.distinct()
처리 하였습니다.3. Test
3. 이후 TODO 제안
1. �공지사항 스크랩과 교직원 스크랩 분리
VeterinaryMedicineDept
의 생성자 내용중 일부입니다.
StaffScrapInfo의
siteId정보를 List로 구현한 이유입니다. 수의과대학의 경우 수의학과, 수의예과의 교수진 정보를 다른 페이지에서 관리하고 있습니다. 이로인해 prefix는 동일하지만
siteId`는 두개입니다.이에, 학과 구분에 따라 수의학과, 수의예과를 각각 구분하여
DepartmentName
,DeptInfo
를 생성하려 했으나, 공지사항 스크랩에서 같은 객체를 공유하고 있다는 것을 확인했습니다. 객체 추가로 인해 교직원 중복은 해결될 수 있지만, 반대로 공지사항이 중복 스크랩되는 등 부수효과가 발생할 수 있다고 예상됩니다.결론적으로, 현재 학과 공지사항과 교직원 정보 제공 유무에 따라 각 정보의 스크랩 유무가 달라지며 따로 예외처리해야합니다. 이를 구분할 수 있는 방법이 필요하다고 생각합니다.
간단히 생각한 방법은 스크랩 시
NoticeScrapInfo
,StaffScrapInfo
을 바탕으로 지원 유무를 체크할 수 있는 로직을 추가하여, 스크랩간 스크랩 대상인지 체크할 수 있을거 같습니다.(isSupportStaffScrap()
와 같은 역할을 하는)혹은, 객체 자체를 분리할 수도 있을거 같으나 작업공수가...🥲
좋은 방법� 생각나시면 공유 부탁드립니닷!
2. 스크랩 정보(
NoticeScrapInfo
,StaffScrapInfo
)를DepartmentName
을 활용하여 한 파일 내에서 관리이번 작업에서 모든 학과별
DeptInfo
객체에서siteId
와siteName
을 변경하는게 상당히 귀찮으면서 실수할 여지가 컸다고 생각합니다.스크랩 특성상 학교 홈페이지에 전적으로 의존하고 있는만큼 학교 홈페이지 리뉴얼에 의해 스크랩 방식이 변경될 가능성이 크다고 생각합니다. 이에, 스크랩을 위한 정보들을(hostPrefix, siteId, siteName)은 모두 한 Enum에서 관리하는게 낫지 않을까 생각합니다.
현재
DeptInfo
에DepartmentName
을 필드로 갖고 있으므로 이를 변경해서 적용하면(DepartmentScrapInfo 이런 느낌의 네이밍..?), 스크랩 정보(NoticeScrapInfo
,StaffScrapInfo
)가 없어도 Enum을 통해 바로 스크랩을 위한 사이트 정보를 얻을 수 있지 않을까 생각하빈다.3. Parsing 후 반환하는
String[]
형식을 DTO형식으로 변경하면 변경에 용이할거 같습니다.매번 배열 순서를 체크해야하고, 모든 값이 String이라 헷갈릴 여지가 충분한거 같습니다. 이에 DTO 추가를 하면 어떨지 생각합니다.
테스트랑 스크랩 리다이렉션때문에 살짝 헤맸습니다 ㅜㅜ