이번에도 단원을 시작하기 앞서 예시 코드를 살펴보자
public static List<int> FindPrimes(int start, int end) {
//Create new list of integers
List<int> primesList = new List<int>();
//Perform a loop from start to end
for (int num = start; num <= end; num++) {
//Declare boolean variable, initially true
bool prime = true;
//Perform loop from 2 to sqrt(num)
for (int div = 2; div <= Math.Sqrt(num); div++) {
//Check if div divides num with no remainder
if (num % div == 0) {
// We found a divider -> the number is not prime
prime = false;
//Exit from the loop
break;
}
위 주석들이 도움이 된다고 생각되는가?
만약 코드를 전혀 이해하지 못하는 수준이라면 당연히 도움이 되겠지만,
프로그래머가 독자라고 가정을 한다면 주석들은 오히려 방해가 될 것이다.
왜냐? 코드만 읽어도 이해할 수 있는 수준인데, 굳이 주석을 적어 읽을 것을 더 늘려놨기 때문이다
만약 코드가 의도를 제대로 전달하고 있지 못한다면, 코드를 수정해야한다! 주석이 필요한 경우도 물론 있는데 이는 아래에서 더 설명하겠다
Introduction
- well-placed 주석보다 도움되는 것은 없다
- frivolous한 주석보다 도움이 안되는 것이 없다
- 틀린 주석만큼 위험한 것이 없다
- 주석은 "순수하게 좋다"라고 할 수 없다
- 주석은 가장 최선이라도 필요악이다
- 코드에서 표현을 다 하고 있다면 주석은 필요 없을 것이다!
- 따라서 주석을 작성할 때마다 표현력의 부족함을 느껴야 한다
- 왜 주석에 대해 부정적인가?
- 주석은 종종 거짓말을 하기 때문이다
- 코드가 바뀌거나 발전할 때마다 현실적으로 주석을 유지시킬 수 없기 때문이다 (매번 바꿔야 하기 때문)
그렇다면 왜 주석을 작성할까?
바로 코드가 좋지 않기 때문이다... (팩폭 멈춰)
우리가 모듈을 작성해놓고 이것이 혼란스럽고 정리가 잘 안되었다는 것을 알 때 주석을 작성해야겠다라고 생각한다. 하지만 이 생각은 clean하게 코드를 수정해야겠다라는 생각으로 바뀌어야 한다
코드 그 자체가 좋은 설명이기 때문이다!
예시를 함께 살펴보자
//Check to see if the employee is eligible for full benefits
if ((employee.flags & HOURLY_FLAG ) &&
(employee.age > 65)) {if (employee.isEligibleForFullBenefits()) {어떤 코드를 더 선호하는가?
의도를 바로 파악할 수 있는 두 번째 코드일 것이다!
많은 경우, 함수를 좋은 이름으로 만드는 것처럼 쉬운 일일 때가 많다!
그럼 이제 좋은 주석에 대해 살펴보자~!
✒️ 좋은 주석
그러나, 진정으로 좋은 주석은 당신이 쓰지 않아도 되는 방법을 찾은 주석임을 잊지 말자.
📌 Legal Comments
- 저작권이나 저자 표시 문구와 같이 법적인 이유로 주석을 써야하는 경우가 있다
// Copyright (C) 2003,2004,2005 by Object Mentor, Inc. All rights reserved.
// Released under the terms of the GNU General Public License version 2 or later.가능한 경우, 모든 조건과 조항을 주석에 넣기보다는 표준 라이선스나 다른 외부 문서를 참조하는 것이 좋다
📌 Informative Comments
- 주석을 통해 기본적인 정보를 제공하는 것이 유용할 때가 있다
🔹 이 주석은 추상 메서드의 return 값에 대해 설명한다
// Returns an instance of the Responder being tested
protected abstract Responder responderInstance();- 함수의 interface : 함수의 이름, 매개변수, return value라고 할 수 있으
- 추상 메소드는 함수의 껍데기만 정의하고 본체가 없는 형태임
하지만 이 경우, 함수명을 다시 작성함으로써 주석을 제거할 수 있다
protected abstract Responder responderBeingTested();함수명은 동사형태가 좋음을 잊지 말자! get과 같은 동사를 사용하면 더 좋음!
🔹 이 주석은 정규식 표현이 시간과 날짜를 매칭하기 위함임을 알려주고 있다
// format matched hh:mm:ss EEE, MMM dd, yyyy
Pattern timeMatcher = Pattern.compile(
"\\d*:\\d*:\\d* \\w*, \\w* \\d*, \\d*");ex. 13:47:08 EST, Jul 10, 2024
📌 Explanation of intent
- 주석은 의도를 드러내주기도 한다
🔹 두 대상을 비교할 때, 저자는 그의 클래스의 대상이 다른 대상보다 더 크다고 결정 지었다.
public intcompareTo(Object o) {
if(o instanceofWikiPagePath) {
WikiPagePathp = (WikiPagePath) o;
String compressedName= StringUtil.join(names, "");
String compressedArgumentName= StringUtil.join(p.names, "");
return compressedName.compareTo(compressedArgumentName);
}
return 1; // we are greater because we are the right type.
}이 코드의 경우 받은게 WikiPagePath가 아니면 무조건 1을 리턴하는데, 왜 이렇게 하는지 알 수 없기에 comment를 사용해도 좋은 케이스이다
🔹 프로그래머의 솔루션에 동의하지 않을 수 있지만, 적어도 그가 무엇을 하고자 했는지 이해할 수는 있다
public void testConcurrentAddWidgets() throws Exception {
WidgetBuilder widgetBuilder = new WidgetBuilder(...);
String text = ...;
ParentWidget parent = new BoldWidget(...);
AtomicBoolean failFlag = new AtomicBoolean();
failFlag.set(false);
// This is our best attempt to get a race condition
// by creating large number of threads.
for (int i = 0; i < 25000; i++) {
WidgetBuilderThread widgetBuilderThread =
new WidgetBuilderThread(widgetBuilder, text, parent, failFlag);
Thread thread = new Thread(widgetBuilderThread);
thread.start();
}
assertEquals(false, failFlag.get());
}📌 Clarification
- 때로는 모호한 매개변수나 return 값을 읽을 수 있는 것으로 번역하는 것이 도움이 될 수 있다
- 하지만, 주석이 정확하지 않을 수 있는 리스크가 있다 → 각오하고 붙여야 한다!
public void testCompareTo() throws Exception {
WikiPagePath a = PathParser.parse("PageA");
WikiPagePath ab = PathParser.parse("PageA.PageB");
WikiPagePath b = PathParser.parse("PageB");
WikiPagePath aa = PathParser.parse("PageA.PageA");
WikiPagePath bb = PathParser.parse("PageB.PageB");
WikiPagePath ba = PathParser.parse("PageB.PageA");
assertTrue(a.compareTo(a) == 0); // a == a
assertTrue(a.compareTo(b) != 0); // a != b
assertTrue(ab.compareTo(ab) == 0); // ab == ab
assertTrue(a.compareTo(b) == -1); // a < b
assertTrue(aa.compareTo(ab) == -1); // aa < ab
assertTrue(ba.compareTo(bb) == -1); // ba < bb
assertTrue(b.compareTo(a) == 1); // b > a
assertTrue(ab.compareTo(aa) == 1); // ab > aa
assertTrue(bb.compareTo(ba) == 1); // bb > ba
}📌 Warning of consequences
- 코드만 봐서는 결과를 예상하기 어려운 경우, 다른 프로그래머들에게 특정한 결과에 대해 주의를 주는 것이 도움이 될 수 있다
🔹 예시 코드는 수행 시간이 엄청 오래 걸릴 것임을 경고하고 있다
// Don't run unless you
// have some time to kill.
public void _testWithReallyBigFile() {
writeLinesToFile(10000000);
response.setBody(testFile);
response.readyToSend(this);
String responseString= output.toString();
assertSubString("Content-Length: 1000000000", responseString);
assertTrue(bytesSent> 1000000000);
}👉 @ignore("Takes too long to run")을 사용하는게 더 나음!!
🔹 more poignant(가슴 아픈) example : 해당 주석은 각 호출마다 별도의 SimpleDateFormat 인스턴스가 생성되는 이유를 설명한다
public static SimpleDateFormat makeStandardHttpDateFormat() {
//SimpleDateFormat is not thread safe,
// so we need to create each instance independently.
SimpleDateForamt df = new SimpleDateFormat("dd MMM yyyy HH:mm:ss z");
df.setTimezone(TimeZone.getTimeZone("GMT"));
return df;
}우선 코드를 보면, 함수를 호출할 때마다 매번 새로운 객체를 만들고 있는데 이는 메모리 낭비가 될 수 있어 독자 입장에서 이유가 궁금할 수 있다.
주석은 동시에 여러개 접속 되었을 때 문제가 될 수 있기에 독립적으로 객체를 만들어 주는 것이 필요하다고 설명하고 있으므로, 이렇게 why에 대한 주석은 필요한 주석이다!
📌 TODO comments
- 'To Do' 노트를 주석으로 작성하는 것이 필요할 수는 있으나 결국 지워야 하는 커멘트임을 잊지말자!
//TODO : these are not needed
// We expect this to go away when we do the checkout model
protected VersionInfo makeVersion() throws Exception {
return null;
}📌 Amplification 강조
- 주석은 중요하지 않아 보일 수 있는 것의 중요성을 강조하는 데 사용될 수 있다
String listItemContent = match.group(3).trim();
// the trim is real important. It removes the starting
// spaces that could cause the item to be recognized
// as another list
new ListItemWidget(this, listItemContent, this.level + 1);
return buildList(text.substring(match.end()));trim을 안하면 다른 list의 아이템으로 오해될 수 있어 trim이 필요함을 보여주는 comment로 필요한 주석이다!
왜냐면 trim을 그냥 누군가가 지워버릴 수도 있기 때문에... (남의 코드 안 건드릴거 같쥬? 그렇지 않더라고요^^)
✒️ 나쁜 주석
📌 Mumbling
- 주저리주저리 작성하지 말고 만약 주석을 달리고 결정을 했으면 가장 좋은 주석을 달기 위해 노력하라
public void loadProperties() {
try {
String propertiesPath= propertiesLocation+ "/" + PROPERTIES_FILE;
FileInputStreampropertiesStream= new FileInputStream(propertiesPath);
loadedProperties.load(propertiesStream);
}
catch(IOExceptione) {
// No properties files means something happened
}
}분명 독자에게 무엇인가를 전달하려고 한것 같지만, 제대로 전달이 되고 있지 않으므로 좋은 주석이 아니다
📌 Redundant comments
- 중복된 주석은 피하라
//Utility method that returns when this.closed is true
// Throws an exception if the timeout is reached
public synchronized void waitForClose(final long timeoutMillis) throws Exception {
if(!closed) {
wait(timeoutMillis);
if(!closed)
throw new Exception("MockResponseSender could not be closed");
}
}
위 코드는 두 가지 이유에서 잘못 되었다
- 코드 내용을 그대로 repeat하고 있으므로 적합하지 않다
- "when"이라고 하면서 뉘앙스가 조금 달라졌다. 주석이 코드의 의도를 살짝 다르게 이야기하고 있는 경우로, 주석이 코드를 이해하는데 도움을 주지 못하고 있음!
이처럼 주석이 틀린 경우가 발생하기 쉽기 때문에 주석이 좋지 않다고 하는 것!
다른 예시들도 보자!
🔹 BAD
function hashIt(data) {
//The hash
let has = 0;
//Length of string
const length = data.length;
//Loop through every character in data
for (let i = 0; i < length; i++) {
//Get character code
const char = data.charCodeAt(i);
// Make the hash
hash = (hash << 5) - hash + char;
//Convert to 32-bit integer
hash &= hash;
}
}🔹 GOOD
function hashIt(data) {
let hash = 0;
const length = data.length;
for (let i = 0; i < length; i++) {
const char = data.charCodeAt(i);
//Make the hash
hash = (hash << 5) - hash + char;
//convert to 32-bit integer
hash &= hash;
}
}- code를 그저 repeat하고 있는 주석은 모두 제거
- Make the hash라는 주석이 유일하게 의미있는 comment다. why? 의도를 설명하고 있기 때문!
- convert to 32-bit integer도 나쁘진 않다
- 하지만, refactoring을 한다면 makeHash라는 함수를 만들어서 해당 코드를 넣는다.
- comment를 다 날릴 수 있는 방법!
다음 예시를 보자.
다음 예시에서는 쓸모없고 중복된 javadoc 주석들은 코드만 어지럽히고 가리는 역할을 하고 있기에 없애는 것이 좋다
public abstract class ContainerBase
implements Container, Lifecycle, Pipeline,
MBeanRegistration, Serializable{
/**
* The processor delay for this component.
*/
protected intbackgroundProcessorDelay= -1;
/**
* The lifecycle event support for this component.
*/
protected LifecycleSupportlifecycle = new LifecycleSupport(this);
/**
* The container event listeners for this Container.
*/
protected ArrayListlisteners = new ArrayList();
/**
* The Loader implementation with which this Container is
* associated.
*/
protected Loader loader= null;
/**
* The Logger implementation with which this Container is
* associated.
*/
protected Log logger = null;
/**
* Associated logger name.
*/
protected String logName= null;
/**
* The Manager implementation with which this Container is
* associated.
*/
protected Manager manager= null;
/**
* The cluster with which this Container is associated.
*/
protected Cluster cluster= null;
📌 Mandated comments 의무화된 주석
- 모든 함수와 변수가 반드시 주석을 가져야한다는 말은 정말 바보같은 말이다
- 이런 주석은 코드만 어지럽히고 혼란과 무질서를 초래한다
/**
*
* @param title The title of the CD
* @param author The author of the CD
* @param tracks The number of tracks on the CD
* @param durationInMinutes The duration of the CD in minutes
*/
public void addCD(String title, String author, int tracks, int durationInMinutes) {
CD cd = new CD();
cd.title = title;
cd.author = author;
cd.tracks = tracks;
cd.duration = duration;
cdList.add(cd);
}📌 Journal comments
- 누가, 언제 작성을 했는지 일기처럼 다 적는 주석을 말한다.
- 이러한 긴 journal은 모듈을 더 혼란스럽게만 만든다
- 그러니 삭제하고 version control system을 대신 사용할것! (ex. Github처럼~)
📌 Noise comments
- 아무 의미 없이 노이즈에 가까운 주석들을 본 적이 있을 것이다. 당연하고 어떠한 새로운 정보도 전달하고 있지 않는 주석을 의미한다
예시를 통해 보자
🔹 디폴트 생성자임을 굳이 명시한 경우 -딱 봐도 생성자이기 때문
/**
* Default constructor
*/
protected AnnualDateRule() {
}🔹 코드 내용 반복
/** The day of the month */
private int dayOfMonth;
/**
*Returns the day of the month
*/
public int getDayOfMonth() {
return dayOfMonth;
}이제 나쁜 코드와 좋은 코드를 함께 보자
🔹 BAD
private void startSending() {
try {
doSending();
} catch(SocketException e) {
//normal. someone stopped the request
} catch(Exception e) {
try {
repsonse.add(ErrorResponder.makeExceptionString(e));
response.closeAll();
} catch(Exception e1) {
//Give me a break!
}
}
}두 번째 주석은 진짜 그냥 노이즈다.. 응...
🔹 GOOD
private void startSending() {
try {
doSending();
} catch(SocketException e) {
//normal. someone stopped the request
} catch(Exception e) {
addExceptionAndCloseResponse(e);
}
}
private void addExceptionAndCloseResponse(Exception e) {
try {
repsonse.add(ErrorResponder.makeExceptionString(e));
response.closeAll();
}
catch(Exception e1) {
}
}그 외에도 scary comment, funny comment 모두 noise에 해당한다!
📌 Position Markers
-
프로그래머는 하단처럼 소스파일에서 특정 위치를 마크하길 바랄 때가 있다
// Actions ////////////////////// -
일반적으로 이런 주석은 어수선하기 때문에 제거되어야 한다
-
position marker를 남용하면 노이즈가 되어버리고 무시될 것이다
📌 Closing Brace Comments
- 괄호가 너무 많으면 무엇이 무엇의 괄호인지 헷갈리기 때문에 하단 예시처럼 괄호 끝에 주석을 달 때가 있다
(flutter는 자동으로 해줘서 편하긴함...헿)
public class wc {
public static void main(String[] args) {
BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
String line;
int lineCount = 0, charCount = 0, wordCount = 0;
try {
while ((line = in.readLine()) != null) {
lineCount++;
charCount += line.length();
String word[] = line.split("\\W");
wordCount += words.length;
} //while
System.out.println("wordCount = " + wordCount);
System.out.println("lineCount = " + lineCount);
System.out.println("cahrCount = " + charCount);
} // try
catch (IOException e) {
System.err.println("Error:" + e.getMessage());
} //catch
} //main
}- 많이 중첩 구조로 긴 코드로 되어있을 경우 이런 주석은 의미가 있을 수 있다
- 우리가 추구해야할 작은 함수에서 이런 주석은 어수선하게 만드는 요소!
- 따라서 닫는 괄호에 주석을 달고 싶을 경우에는 함수를 더 짧게 만들기 위해 노력해보도록 하자!
📌 Attributions and Bylines
- byline은 필자를 적은 행을 의미한다
- 아래 예시처럼 byline로 코드를 더럽힐 이유가 없다!
/ Added by Rick /
-
이런 정보를 위해서는 version control system을 사용할 것! 누가 언제 무엇을 업데이트 하였는지 다 기록되니까,,
-
bylines는 수년동안 남아 점점 더 부정확하고 관련성이 없어진다.. 다른 누군가가 edit을 반드시 할 것이기에~
📌 Commented-Out Code
InputStreamResponse response = new InputStreamResponse();
response.setBody(formatter.getResultStream(), formatter.getByteCount());
//InputStream resultsStream = formatter.getResultStream();
// StreamReader reader = new StreamReader(resultsStream);
// response.setContent(reader.read(formatter.getByteCount()));- 주석 처리된 코드를 보는 사람들은 이 주석들을 삭제하지 않을 것이다.
- 주석이 어떤 이유로 존재할 것이라고 생각할 것이고, 삭제하기에는 중요할 수도 있겠다고 생각하기 때문!
- 그래서 이건 나쁜 와인 병의 찌꺼기처럼 쌓이게 되므로 삭제할것!
Apache Commons에서 가져온 다른 예시를 보자!
this.bytePos = writeBytes(pngIdBytes, 0);
//hdrPos = bytePos;
writeHeader();
writeResolution();
//dataPos = bytePos;
if (writeImageData()) {
writeEnd();
this.pngBytes = resizeByteArray(this.pngBytes, this.maxPos);
} else {
this.pngBytes = null;
}
return this.pngBytes;
주석 처리된 코드를 보면 hdrPos = bytePos;와 dataPos = bytePos; 두 줄이 있다. 이부분을 주석 처리한 이유에 대해 명확한 설명이 없기 때문에 다음 질문들이 발생한다
- 왜 이 두줄의 코드가 주석처리 되었을까?
- 이 코드들이 중요한가?
- 향후 변경을 위한 알림으로 남겨진 것인가?
- 단순히 몇년 전에 주석 처리된 불필요한 코드일까?
이런 주석 코드가 많아지면 코드의 가독성, 유지보수성이 떨어지게 된다. 그리고 위 질문들을 팀원들이 던지게 되고 혼란에 빠질 수 있다.
따라서 주석 처리된 코드를 가능한 한 빨리 제거하거나, 필요하다면 명확한 설명을 붙여야 한다!
📌 Nonlocal information / Too much information
- 주석은 그 주석이 나타나는 코드 근처를 설명해야 한다
- 지역 주석의 맥락에서 시스템 전체 정보를 제공하지 말것!!
/**
* Port on which fitnesse would run. Defaults to 8082
*
* @param fitnessePort
*/
public void setFitnessePort(int fitnessePort) {
this.fitnessePort = fitnessePort;
}- 위 코드에서 함수는 기본값에 대해 전혀 제어하고 있지 않다
- 즉, 이 주석은 함수를 설명하고 있지 않은 것!
- 더 나쁜 것은, 기본값이 변경될 때 이 주석이 변경되지 않을 수 있다는 점이다..
- 주석에 역사적인 논의나 관련없는 세부 설명은 넣지 말 것!
- 대부분의 사람들은 그 정보가 필요하지 않다...
/*
RFC 2045 -Multipurpose Internet Mail Extensions (MIME)
Part One: Format of Internet Message Bodies
section 6.8. Base64 Content-Transfer-Encoding
The encoding process represents 24-bit groups of input bits as output
strings of 4 encoded characters. Proceeding from left to right, a
24-bit input group is formed by concatenating 3 8-bit input groups.
These 24 bits are then treated as 4 concatenated 6-bit groups, each
of which is translated into a single digit in the base64 alphabet.
When encoding a bit stream via the base64 encoding, the bit stream
must be presumed to be ordered with the most-significant-bit first.
That is, the first bit in the stream will be the high-order bit in
the first 8-bit byte, and the eighth bit will be the low-order bit in
the first 8-bit byte, and so on.
*/
📌 Inobvious Connection
- 주석과 코드 간의 연결은 명백해야 한다!
/*
* start with an array that is big enough to hold all the pixels
* (plus filter bytes), and an extra 200 bytes for header info
*/
this.pngBytes = new byte[((this.width + 1) * this.height * 3) +200];이 코드를 읽으면 다음과 같은 질문이 생길 수 있다
- filter byte는 무엇인가? +1과 연관이 있는 것인지 * 3과 연관이 있는 것인지 혹은 둘 다인지 알 수 없음
- pixel이 byte인가? 왜 200인가?
- 주석 자체에 설명이 필요한 경우는 매우 유감인 것
📌 Function Headers
- 짧은 함수는 많은 설명이 필요하지 않다
- 한 가지 일을 수행하는 작은 함수가 좋은 이름을 가지고 있다면, comment header보다 더 실용적일 것
👉 함수나 변수를 사용할 수 있을 때 주석을 사용하지 말것!
- 많은 경우, 코드는 주석없이 다시 바꾸어 작성될 수 있다 (rephrased)
//Does the modeul depend on the subsystem we are part of?
if (module.getDependSubsystems().contains(subSysMod.getSubSystem()))⏬⏬⏬⏬⏬⏬⏬
ArrayList moduleDependees = module.getDpendSubsystems();
String ourSubSystem = subSysMod.getSubSystem();
if (moduleDependess.contains(ourSubSystem))👉 Use it to explain why
- 코드 자체에 대해서가 아니라, 코드의 의도나 목적에 관해 주석을 달아라!!
- Focus on the why rather than the how
/*check each character in *inputString* unitl a dollar sign is found or all characters have been checked
*/
done = false;
maxLen = inputString.length();
i = 0;
while (!done && (i < maxLen)) {
if (inputString[i] == '$')
done = true;
else
i++;
}- even better :
//find the command-word terminator ($)
🔻 Final Example
마지막 예시를 보며 Refactoring을 해보자!
🔹 BAD
- [n] 숫자들은 설명을 하기 위해 주석을 추가한 것으로 코드의 퀄리티를 볼 때는 무시해주시길 바랍니다..
/**
* This class Generates prime numbers up to a user specified
* maximum. The algorithm used is the Sieve of Eratosthenes.
* <p>
* Eratosthenes of Cyrene, b. c. 276 BC, Cyrene, Libya -
* d. c. 194, Alexandria. The first man to calculate the
* circumference of the Earth. Also known for working on
* calendars with leap years and ran the library at Alexandria.
* <p>
* The algorithm is quite simple. Given an array of integers
* starting at 2. Cross out all multiples of 2. Find the next
* uncrossed integer, and cross out all of its multiples.
* Repeat until you have passed the square root of the maximum
* value.
*
* @author Alphonse
* @version 13 Feb 2002 atp
*/
50
import java.util.*;
public class GeneratePrimes {
/**
* @param maxValue is the generation limit
*/
public static int[] generatePrimes(int maxValue) {
if (maxValue >= 2) { //the only valid case [0]
//declarations [1]
int s = maxValue + 1; //size of array [2]
boolean[] f = new boolean[s]; [2]
int i;
//initialize array to true
for (i = 0; i < s; i++) //[3]
f[i] = true;
//get rid of known non-primes
f[0] = f[1] = false;
//sieve
int j;
for (i = 2; i < Math.sqrt(s) + 1; i++) { //[4]
// [5]
if (f[i]) { //if i is uncrossed, cross its multiples
for (j = 2 * i; j < s; j += i) //[6]
f[j] = false; //multiple is not prime
}
// how many primes are there? [7]
int count = 0;
for (i = 0 ; i < s; i++) {
if (fi[i])
count++; //bump count
}
int[] primes = new int[count];
//move the primes into the result [8]
for (i = 0, j = 0; i < s; i++) {
if (fi[i]) //if prime
primes[j++] = i;
}
return primes; //return the primes [9]
}
else //maxValue <2
return new int[0]; //return null array if bad input [10]
}
주석 및 코드를 톺아보자!
- maxValue에 대한 설명이 되므로 의미 있는 주석
- 변수 선언을 할 때 굳이 '선언'이라는 주석이 필요하지 않음
- 변수명을 통해 얻을 수 있는 정보가 없다. s,a,b와 같은 알파벳 변수는 절대 좋지 않음!
- for문 안에서 i가 튀어나오는 것은 bad. i 대신 다른 변수명을 사용해서 무엇을 수행하는 것인지 드러나도록 수정하는 것이 좋다!
- 왜 Math.sqrt(s)까지만 loop를 도는지에 대한 comment가 필요하다
- if절을 isCrossed()라는 함수로 빼고, "if i is uncrossed,..~" 주석을 삭제하는 것이 좋다
- if문 안의 for문 역시 CrossIsMultiples()와 같은 함수로 빼는 것이 좋다. 그래야 의도가 드러나기 때문
- 이 곳 역시 countPrimes()라는 함수가 필요
- 이 곳 역시 movePrimesIntoResult()와 같은 함수가 필요
- return primes를 반복하고 있는 주석이므로 의미 없음
- int[0]이 무엇인지 알기 어렵기 때문에 "return null array if bad input" 주석은 의미가 있는 주석!
🔹 Refactored
/**
* This class Generates prime numbers up to a user specified
* maximum. The algorithm used is the Sieve of Eratosthenes.
* Given an array of integers starting at 2:
* Find the first uncrossed integer, and cross out all its
* multiples. Repeat until there are no more multiples
* in the array.
*/
public class PrimeGenerator { //[1]
private static boolean[] crossedOut;
private static int[] result;
public static int[] generatePrimes(int maxValue) {
if (maxValue < 2) //the only vaild case
return new int[0];
else { // [2]
uncrossInteggersUpTo(maxValue);
crossOutMulitples();
putUncrossedIntegersIntoResult();
return result;
}
}
private static void uncrossIntegersUpTo(int maxValue) {
crossedOut = new boolean[maxValue + 1];
for (int i = 2; i < crossedOut.length; i++)
crossedOut[i] = false;
}
private static void corssOutMultiples() { //[3]
int limit = determineIterationLimit();
for (int i = 2; i <= limit; i++)
if (notCrossed(i))
crossOutMultiplesOf(i);
}
private static int determineIterationLimit() {
//Every multiple in the array has a prime factor that
// is less than or equal to the root of the array size
// so we don't have to cross out multiples of numbers
// larger than root
double iterationLimit = Math.sqrt(crossedOut.length);
return (int) iterationLimit;
}
private static void crossOutMultiplesof(int i) {
for (int multiple = 2*i; multiple < crossedOut.length; multiple += i)
crossedOut[multiple] = true;
}
private static boolean notCorssed(int i) { //[4]
return crossedOut[i] == false;
}
private static void putUncrossedIntegersIntoResult() {
result = new int[numberOfUncrossedIntegers()]; //[5]
for (int j = 0; i = 2; i < crossedOut.length; i++)
if (notCrossed(i))
result[j++] = i;
}
private static int numberOfUncrossedIntegers() {
int count = 0;
for (int = 2; i < crossedOut.length; i++)
if (notCrossed(i))
count++;
return count;
}
}- class명은 동사가 아닌 명사여야 하므로 GeneratePrimes는 좋지 않은 이름이다
- else문을 하나의 함수로 빼서 전체 의도가 나타나도록 하면 좋았을 것이다! ex. IsInteger()처럼
- 아주 좋은 코드! sqrt를 사용하는 것이 의문이 들기 때문에
determineIterationLimit()함수로 뽑아낸 것! - 한줄짜리 코드를 함수로 빼는게 비효율적이라고 생각될 수 있지만 가시성이 높아지고, 여러군데에서 쓰일 수 있으므로 함수로 만드는게 좋다
- 메소드는 명사로 쓰지 말것!! 동사로 써야 하므로
numerOfUncrossedIntegers()대신CountUncrossedIntegers()가 더 좋은 이름이다
욕심 많은 공대생