협업이 중요해오
코딩하는 개발자간의 협업은 중요하다. 개발하지 못한 기능 추가, 혼자서는 발견하지 못한 버그를 찾거나, 더 나은 코드를 생각해내서 알려주고 적용할 수 있는 그런 서로가 윈윈할 수 있는것이 협업이라 생각한다.
나도 몇번의 내부 프로젝트를 경험하면서 일부 null-check생략, annotaion missing 부터 잘못된 api 사용으로 크래시가 나는 문제를 협업을 통해 원인을 찾고 해결할 수 있었다.
나 또한 다른 개발자의 코드를 보면서 특정 조건에서 발생할 수 있는 오류를 예방할 수 있는 코드를 제시했고 잘 적용했었던 기억이 있다.
계기
예전에 이 글을 오픈소스 관련 특강을 들으면서 우연히 보게 되었다.
그전까지 생각했던 기여는 일명 "개발 고인물", "눈 감고 코딩해도 내가 짠 코드를 능가하는 무언가"들이 문제점을 발견하고, 이를 효율적으로 개선하는 일련의 과정으로 느끼고 있었다.
그래서 issue정도는 날리면서 문제점 리포팅만 해봤던? 그런 단계였지만, 위 글과 특강을 들으며 "협업"에 있어선 더 나은 부분을 생각하고, 개선할 방안을 제공할 수 있다면 사소한거라도 괜찮다는것을 알게되었다.
나도.. 나도할거야
그래서 그전까지 백엔드 프레임워크로 스프링 부트를 사용해봤던 경험이 있었기에
생각난김에 스프링이라는 거대한 오픈소스에 기여하기로 했다.
솔직히 70%는 기여자에 이름을 올리는 영광을 갖고싶었던것도 있었다 :)
과정
우선 PR을 하기전에 고려했던게 3가지 있었다.
- 코드 컨벤션 부수지 말기
- 이상한 커밋 이력 만들지 말기
- 확신을 가지기
협업에서 중요한것중 하나는 코드 컨벤션이라고 생각한다.
나는 else를 다음 라인에 넣고 싶은데 너도 함 무봐라 하면서 모든 코드를 자기 입맛대로 바꾸고 style: restyling else branch 커밋을 날리는 순간 수많은 마찰이 발생할것이다.
코드 스타일은 개인마다 다르기 때문에 보통은 하나의 규율로 정해놓기 때문에 지키는것이 중요하다
이상한 커밋 이력은.. 아 맞다 이 커밋 빠트렸지 하면서 feat: add blahblah.java 커밋을 수십개씩 쌓아놓는다면 PR 당사자 입장에서 매우 난해하고 읽기도 싫어질것이다. 그래서 보통 PR시에 rebase를 이용해서 깔끔하게 하나의 커밋으로 정리하고 올린다고 한다.
마지막으로 내가 수정한 코드의 수정사유를 확신있게 말할 수 있어야함이였다. 나도 이해못하고, PR당사자도 이해할 수 없는 코드는 쓸모없는 코드라고 생각한다. 따라서 간결하고, 확신있는 코드여야 한다고 생각했다.
찾기..
스프링 코드는 너무 막대했다. 기능에 따라 cloud, starter등으로 나뉘어진 project만 7개가 넘어갔고, 거기 세부적으로 나뉘어지는게 너무 많았다.
일단은 core부분을 위주로 확인하고자 했다.
진짜 너무 많았다. 확실히 이런 대형 프로젝트는 기여자의 도움이 있으면 훨씬 유지보수하는데 도움이 될것이라는 생각이 들었다.
뭐.. 코드 찾아보는건 좋아하니까 스프링 내부 구현 보는겸해서 하나하나 클래스를 구경했다.
확실히 코드가 간결하고 상속 구조를 잘 활용해서 진짜 필요한 객체에 위임을 하고있다는 느낌을 받았다. 나도 저렇게 짜고싶다
위 게시글처럼 String literal로 hard-coad된 부분을 상수로 합쳐서 유지보수의 용이성과 읽기 유용하게 변경하는 부분을 찾을까? 하다가 나 처럼 생각하는 사람이 많다는걸 알게되었다.
이미 관련된 부분은 다 수정되어 커밋이력이 남아 있었고, 거의 없다고 봐야 했다.
그래도.. 이왕 하는김에 좀더 나가서 기존 코드를 효율적으로 재구성해보고 싶었다.
Enhancement
그러던중 하나의 클래스를 발견했다. 서버와 관련된 파일을 쓰는 클래스로 보였다.
- WebServerPortFileWriter.java
protected File getPortFile(ApplicationContext applicationContext) {
String namespace = getServerNamespace(applicationContext);
if (!StringUtils.hasLength(namespace)) {
return this.file;
}
String name = this.file.getName();
String extension = StringUtils.getFilenameExtension(this.file.getName());
name = name.substring(0, name.length() - extension.length() - 1);
if (isUpperCase(name)) {
name = name + "-" + namespace.toUpperCase(Locale.ENGLISH);
}
else {
name = name + "-" + namespace.toLowerCase(Locale.ENGLISH);
}
if (StringUtils.hasLength(extension)) {
name = name + "." + extension;
}
return new File(this.file.getParentFile(), name);
}위 코드에서 문제점이 무엇일까?
바로 extension을 가져올때 this.file 객체의 getName()메소드를 중복호출한 부분이였다.
이미 name 변수에 name을 할당했는데, getName으로 또 한번더 가져온다.
File 클래스의 getName메소드의 구현체는 다음과 같다.
- File.java
public String getName() {
int index = path.lastIndexOf(separatorChar);
if (index < prefixLength) return path.substring(prefixLength);
return path.substring(index + 1);
}
물론 jdk마다 구현이 다를수 있기 때문에, 이 부분은 정확하게 장담할 수 없지만, 현재 사용하는 OpenJDK 17버전에서는 파일의 경로(문자열)를 slice하고, 파일 이름부분만 가져오는 작업을 한다.
근데 문제는 '매번' 메소드를 호출할때마다 이 작업을 수행한다는점이다.
문자열의 '마지막 인덱스'를 찾는 작업은 시간복잡도 O(n)의 작업이므로 문자열 탐색에 시간이 어느정도 할애된다.
물론 path가 항상 긴건아니라 찾는데 오래 걸리지는 않지만, 약간의 불필요한 작업을 두번 하는 느낌이라 getName을 중복호출하지 말고 위의 name 지역변수를 이용하도록 수정한다.
String extension = StringUtils.getFilenameExtension(name);이렇게 좀더 더 나은 방안을 생각하고 적용했다.
기존에 단순히 어떤 클래스의 메소드만 쓰면 돌아간다~ 보단, IJ의 디컴파일 기능을 이용해서 실제 구현부를 확인하고 어떻게 돌아가는지 확인해보는게 재밌었는데 여기서 빛을 발했던거 같다
하지만, 밑의 코드도 뭔가 더 나은 방안이 있을것 같았다.
현재 코드의 작동방식은 다음과 같다.
작동 기작
- 파일 이름을 가져온다.
- 파일 확장자를 1에서 가져온다.
- 파일명(name)에서 확장자를 잘라낸다.
3.1. 파일명이 isUpperCase를 통과하면 파일명 뒤에 '-' 와 namespace를 대문자로 붙인다.
3.2. 파일명이 isUpperCase를 통과하지 못한다면 뒤에 '-' 와 namespace를 소문자로 붙인다. - 확장자가 있다면, 뒤에 확장자를 붙인다.
여기서 중복된 부분이 좀 보였다.
4-1과 4-2에 작대기('-')를 붙이는 과정은 둘다 공통되므로 하나로 합칠 수 있을것 같았고, 확장자를 굳이 마지막에 붙일거라면 3. 에서 확장자를 제외할 필요가 있을까? 라는 생각이 들었다.
그래서 코드를 개선했다.
개선
protected File getPortFile(ApplicationContext applicationContext) {
String namespace = getServerNamespace(applicationContext);
if (!StringUtils.hasLength(namespace)) {
return this.file;
}
String name = this.file.getName();
String extension = StringUtils.getFilenameExtension(name);
StringBuilder builder = new StringBuilder(name);
String suffix = "-" + (isUpperCase(name) ? namespace.toUpperCase(Locale.ENGLISH) : namespace.toLowerCase(Locale.ENGLISH));
if (StringUtils.hasLength(extension))
builder.insert(name.lastIndexOf(extension) - 1, suffix);
else
builder.append(suffix);
return new File(this.file.getParentFile(), builder.toString());
}extension을 잘라내 파일명만 가져오는 방법 대신, StringBuilder를 이용해 insert를 이용하는 방법을 택했다.
뒤에 붙일 문자열을 가져오는 방법은 삼항연산자를 이용, "-"는 공통적으로 붙이되, upperCase인경우 upper된값, 아닌경우 lower된값을 갖는 suffix라는 지역변수를 하나 만들었다.
이후, 확장자가 있는경우 확장자 앞의 .위치인 lastIndex - 1부분에 suffix를 삽입하고, 아닌경우 마지막에 append해서 이어붙이는 builder를 이용했다.
이 부분은 개발자마다 더 '간결하고', '이해하기' 쉬운 코드에 대한 생각이 다르기 때문에, ignore될 수 있다는 점은 염두에 두었다.
Test
요시! 수정 완료~ 하고 바로 PR 날리는 순간 난리가 날 수 있다.
내가 수정한 코드가 정상 작동 되는지를 보증할 수 없기 때문이다.
따라서 Test코드를 생성해야 하나.. 생각했는데, 기존에 있던 테스트 클래스가 있었다. 이를 이용해 기존 메소드의 리팩토링이 잘 진행되었는지 보증할 수 있었다.
Mistake..
위 테스트 수행과정에서 2개의 테스트를 통과하지 못했었다. 왜 안되지 하면서 보던중, 수정한 메소드에서 문제가 발생함을 알 수 있었다.
breakPoint를 걸어본 결과.. application-PORTFILE.file 이렇게 결과값이 나와야 했는데 application.-PORTFILEfile 이런값을 가지고 있었다..
알고보니 내가 extension의 index를 가져오는 부분에서 lastIndexOf(extension)은 확장자의 점(.) 바로 뒷 부분의 인덱스를 찾으므로 문제가 생긴것이였다.
그래서 위 코드처럼 lastIndexOf에 -1 한 값을 넣어 해결하였다.
PR
이후 수정한 부분을 deepl 갓갓과 papago 갓을 이용해 번역한 뒤, 개인적으로 너무 과하게 표현된 부분 (아예 삭제했다~ 이런식으로 번역)은 좀 다듬고 PR을 요청했다.
대충 getName 메소드는 캐싱(기존 실행결과를 전역변수든 지역변수든 저장해두고 변수만 그대로 반환)되지 않고 항상 slice하기 때문에 좀 효율적이지 않다. 그래서 수정했다. + StringBuilder써서 좀더 효율적으로 개선해봤다. 가 메인 내용이였다.
그래서 기다리던중..
!!!!!
감사하다고 코멘트가 달렸다.
getName이 캐싱되지 않는건 알지 못했고, StringBuilder를 쓰는건 좀 읽기가 더 어렵다, 다시 String쓰는걸로는 변경하는데 변수 이름을 좀 개선했다라는 내용이였다.
그래서 보니 getName 부분이 수정되었고, 밑에 if 부분은 개선이 이루어졌다.
authored로 커밋 이력에 들어갔다.. 너무 좋았다.
후기
깃허브 PR : https://github.com/spring-projects/spring-boot/pull/42411#issuecomment-2367399470
이런 사소한 개선사항이라도 잘 답변해주고 적용해준다는것이 좋았고, 협업은 더 나은 코드가 어떤건지, 상대방의 의견은 어떤지 확인하고, 더 좋게 개선할 수 있는 좋은 기회라는것을 한번더 깨닫게 되었다.
좀 아쉬운점이라면, 아까전에 내가 수정한 부분 (StringBuilder)에 대해서 '이 부분은 수정되어도 괜찮습니다? 무시되어도 괜찮습니다라는 느낌으로 가볍게 수정한 부분을 좀 알려드렸으면 어땠을까 라는 생각이 들었다.
분명 한글로 작성할때는 있었는데, 영어로 옮기다보니 사라졌던거 같기도 하구
끗
