.NET 코드 리뷰 팁

이 글은 다음의 내용에 대한 제 맘대로 번역 글입니다. ^^

My tips for .NET code-review
; https://levelup.gitconnected.com/my-tips-for-net-code-review-f1a47feece43

개인적으로는, 이 글이 review에 관한 내용이라기보다는 그냥 Tip 정도로 보면 좋을 듯합니다.

---

• Collection은 선언과 동시에 초기화

Collection 개체는 선언한 경우 그냥 new 할당까지 같이 하는 것을 권장합니다. (원글의 덧글에도 나오지만, 이에 대해서는 호불호가 있습니다.)

List<Person> persons;

==> List<Person> persons = new List<Persons>();

• Collection을 반환하는 메서드의 경우 null 반환을 하지 않는다.

반환 타입이 Collection 관련 개체라면, 내부에서 비록 반환할 것이 없다고 해도 null이 아닌, 빈 collection 개체를 반환합니다.

public List<Person> GetPersons()
{
    return new List<Person>();
}

• FirstOrDefault() 또는 First()

First()는 조건을 만족하는 요소가 없으면 InvalidOperationException 예외를 발생시킵니다. 이 오류를 피하려면 2가지 방법이 있는데, 만약 기본값을 설정해야 한다면 DefaultIfEmpty를 이용하던가,

List<int> numbers = new List<int> { };
int firstNumber = numbers.DefaultIfEmpty(1).First();

FirstOrDefault() 확장 메서드를 사용할 수 있습니다. FirstOrDefault는 예외를 발생시키지는 않지만 참조 형식의 경우 default가 null이므로 이후 연산에서 null 예외가 발생할 수 있음을 인지해야 합니다. 따라서 FirstOrDefault의 반환 값에 대해서는 언제나 null 체크를 하는 것이 권장됩니다.

var firstPerson = persons.FirstOrDefault(x => x.Age > 18);
if (firstPerson != null)
{
    // ...
}

유사하게, Single() / SingleOrDefault() 메서드도 같은 문제가 발생합니다.

• Dictionary의 경우 키가 있는지 체크

NullReferenceException은 아니지만, Dictionary의 경우 키에 해당하는 자료가 없을 때 indexer를 사용하면 KeyNotFoundException이 발생하므로 대신 ContainsKey로 먼저 체크하거나 TryGetValue 등의 메서드를 이용하는 것이 권장됩니다.

var persons = new Dictionary<string, Person> = new Dictionary<string, Person>();
if (persons.ContainsKey("Jack"))
{
    var person = persons["Jack"];
    // ...
}

if (persons.TryGetValue("Jack", out Person person))
{
    // ...
}

• as 또는 is

형변환 연산자는 변환에 실패한 경우 예외가 발생하지만 as는 null을 반환하므로 좀 더 안전한 연산입니다.

protected override void SetValueImpl(object target, SomeStatus value)
{
    var ctrl = target as ControlA;
    ctrl.Status = newValue;
}

하지만, null 반환이 가능하다는 점에서 반드시 null 체크를 해야 합니다. 혹은 is + as 조합으로 대체하거나,

if (target is ControlA)
{
    var ctrl = target as ControlA;
    ctrl.Status = newValue;
}

C# 7.0부터의 is 패턴 연산으로 대체할 수 있습니다.

if (target is ControlA ctrl)
{
    ctrl.Status = newValue;
}

참고: Type-testing operators and cast expression (C# reference)

• Check the instance which comes from the DI container for deep-sleep mode on mobile

[원글 참고]

• if 문을 동반한 null 체크를 대신할 수 있는 방법

C# 6.0부터 추가된 null 조건 연산자, '?'를 사용하면 됩니다.

A?.B?.Do(C);

==>
if (A != null)
{
    if (B != null)
    {
        B.Do(C);
    }
}

또한, indexer를 사용할 때도 null 체크 if 문 대신 사용할 수 있습니다.

string firstElement = (lines != null) ? lines[0] : null;

==>
string firstElement = lines?[0];

null 조건 연산자를 사용하는 대표적인 사례가 delegate 등의 인스턴스에 대해 호출을 하는 경우입니다.

class Counter
{
    public event EventHandler ThresholdReached;
    protected virtual void OnThresholdReached(EventArgs e)
    {
        /*
        if (ThresholdReached != null)
        {
            ThresholdReached(this, e);
        }
        */

        ThresholdReached?.Invoke(this, e);
    }
}

그 외에, C# 2.0에 추가된 null 병합 연산자, '??'를 사용하면 기존의 조건 연산자로 사용한 코드를 간단하게 축약할 수 있습니다.

int[] elems = null;

int [] GetElements()
{
 // return (elems != null) ? elems : Array.Empty<int>();
    return elems ?? Array.Empty<int>();
}

Dictionary dict = null;

// foreach (var item in (dict != null) ? dict : new Dictionary())
   foreach (var item in dict ?? new Dictionary())
{
}

게다가 C# 8.0부터 추가된 null 병합 할당 연산자, '??='도 있습니다.

/*
if (variable is null)
{
    variable = expression;
}
*/

variable ??= expression;

• Parse 메서드보다 TryParse 사용

이젠 두말할 필요 없을 듯!

---

• event handler를 제외하고, async 메서드의 경우 void 반환보다 Task 반환

자세한 사항은 다음의 글을 참조하세요.

async 메서드의 void 반환 타입 사용에 대하여
; https://www.sysnet.pe.kr/2/0/11414

• async 메서드를 사용 시 Task.Wait(), Task.Result 등의 blocking 코드 호출은 사용하지 말 것!

async 메서드를 await으로 호출하지 않고 Task로 반환받아 처리하는 경우 나타나는 부작용을 전에도 여러 글에서 설명한 적이 있습니다.

WebClient 타입의 ...Async 메서드 호출은 왜 await + 동기 호출 시 hang 현상이 발생할까요?
; https://www.sysnet.pe.kr/2/0/11419

C# - Task.Yield 사용법 (2)
; https://www.sysnet.pe.kr/2/0/12245

따라서, 일단 async/await으로 흐르기 시작한 코드는 중간에 절대 (상황을 100% 이해하지 않은 상태라면) Task를 이용한 동기 코드를 섞지 않는 것이 권장됩니다.

• async 메서드 내에서 다시 await 호출과 함께 반환하는 경우라면 직접 Task를 반환

예를 들어, 다음과 같은 코드의 경우,

public async Task<string> GetDataFromApiAsync()
{
    //Do some async jobs
}

/*? 이렇게 처리하는 것도 가능하지만,
public async Task<string> GetDataAsync()
{
    return await GetDataFromApiAsync();
}
*/
?
// Async 메서드의 Task를 직접 반환하는 것을 더 권장
public Task<string> GetDataAsync()
{
    // Just return the task
    return GetDataFromApiAsync();
}

---

• 다중 Task의 처리였다면 Task.WaitAll로 호출

예를 들어, 2개의 작업을 처리한 후에 실행해야 하는 코드가 있다고 가정했을 때 다음과 같은 식으로 코딩하게 되면,

// C# 9.0 - (15) 최상위 문(Top-level statements)

using System;
using System.Threading;
using System.Threading.Tasks;

Console.WriteLine(DateTime.Now);
await TaskA();
await TaskB();
Console.WriteLine(DateTime.Now);

Task TaskA()
{
    return Task.Run(() =>
   {
       Console.WriteLine("A work Started");
       Thread.Sleep(2000);
       Console.WriteLine("A work Completed");
   });
}

Task TaskB()
{
    return Task.Run(() =>
    {
        Console.WriteLine("B work Started");
        Thread.Sleep(2000);
        Console.WriteLine("B work Completed");
    });
}

/* 출력 결과
2020-12-17 오전 11:47:24
A work Started
A work Completed
B work Started
B work Completed
2020-12-17 오전 11:47:28
*/

총 4초의 시간이 걸린 후에 다음 작업에 들어가게 됩니다. 반면, Task를 직접 사용하면 2초 정도 만에 끝낼 수도 있는데요,

Console.WriteLine(DateTime.Now);

Task taskA = TaskA();
Task taskB = TaskB();

taskA.Wait();
taskB.Wait();

Console.WriteLine(DateTime.Now);

/* 출력 결과
2020-12-17 오전 11:46:48
B work Started
A work Started
A work Completed
B work Completed
2020-12-17 오전 11:46:50
*/

그렇다고 해도 Task.Wait을 호출하기보다는 Task.WaitAll로 사용하라고 합니다.

Task.WaitAll(taskA, taskB);

/* 출력 결과
2020-12-17 오전 11:49:16
A work Started
B work Started
B work Completed
A work Completed
2020-12-17 오전 11:49:18
*/

그런데, 저건 틀린 조언입니다. 왜냐하면 개별적으로 Wait을 호출하는 것과 다를 바 없는 동기 코드이고 결국 async + Task.Wait을 섞었을 때 dead-lock이 발생하는 경우라면 Task.WaitAll을 사용해도 마찬가지로 발생할 수 있기 때문입니다.

따라서, 이런 경우에는 Task.WaitAll이 아닌, Task.WhenAll로 바꿔 await 호출을 하는 것이 더 좋습니다.

await Task.WhenAll(taskA, taskB);

/* 출력 결과
2020-12-17 오전 11:51:58
A work Started
B work Started
A work Completed
B work Completed
2020-12-17 오전 11:52:00
*/

• 기어코 사용해야 한다면, dead-lock을 피하기 위해 ConfigureAwait(false) 사용

어쨌든 그것이 최선의 방법이겠지만, 그래도 dead-lock이 발생하는 경우도 있으므로,

WebClient 타입의 ...Async 메서드 호출은 왜 await + 동기 호출 시 hang 현상이 발생할까요?
; https://www.sysnet.pe.kr/2/0/11419

100% 확신해서는 안 됩니다.

---

• 에러 코드를 반환하기보다는 예외로 처리

사실, 성능 이슈라는 점을 감안한다면 오류가 발생했을 때의 문제 상황을 처리할 수밖에 없도록 만든다는 점에서 예외가 더 낫긴 합니다. 단지, 결과적으로 나중에 원인 분석을 위해 어떤 예외인지 로그를 남기거나 할 때 장황한 오류 메시지를 쓸 수도 있겠지만 관리적인 측면에서 오류 코드 정보를 담은 예외가 더 나을 수 있습니다. (아니면, 오류 코드에 따른 상황별 예외 타입을 개별적으로 만들거나!)

• 가능한 BCL의 기본 예외 타입을 사용

굳이 새로 만들지 말고, .NET의 BCL에 포함된 여러 예외 타입(ArgumentException, ArgumentNullException, InvalidOperationException, 등...)을 사용하는 것을 권장합니다.

• 사용자 정의 예외를 만드는 경우 부가 정보를 위한 속성 제공

원 글에서는, 디버깅 측면에서 봤을 때 예외 타입에 좀 더 부가 정보를 담는 속성/필드가 있다면 좋겠다고 하지만, 개인적으로는 로그로 남기는 것을 대비해 Message 문자열을 잘 정의하는 것도 그에 못지 않게 중요하다고 봅니다.

원 글에서 예를 든 FileNotFoundException의 FileName 속성도 사실 여러분들이 디버깅하면서 FileName 속성까지 찾아보는 경우는 거의 없었을 것입니다. 예를 들어, 다음과 같은 코드에서 예외가 발생하면,

File.Open("test.txt", FileMode.Open);

FileName 속성을 조사하기보다는 화면에 출력된 (또는 로그 파일에 출력한) Exception.Message가 더 도움이 됩니다.

Unhandled exception. System.IO.FileNotFoundException: Could not find file 'C:\ConsoleApp1\bin\Debug\net5.0\test.txt'.
File name: 'C:\ConsoleApp1\bin\Debug\net5.0\test.txt'
   at System.IO.FileStream.ValidateFileHandle(SafeFileHandle fileHandle)
   ...[생략]...

• 예외를 try-catch로 처리하는 경우 "throw ex"보다는 "throw"를 권장

예를 들어 볼까요?

using System;
using System.IO;

namespace ConsoleApp1
{
    static class Program
    {
        static void Main()
        {
            try
            {
                ProcessFile();
            }
            catch (Exception)
            {
                throw;
            }
        }

        private static void ProcessFile()
        {
            File.Open("test.txt", FileMode.Open);
        }
    }
}

위와 같은 경우 "throw"를 사용했으므로 기존 예외 문맥을 그대로 전달하게 되므로 화면에서 다음과 같이 ProcessFile 메서드 내부에서 잘못되었다는 것을 인지할 수 있습니다.

Unhandled exception. System.IO.FileNotFoundException: Could not find file 'C:\ConsoleApp1\bin\Debug\net5.0\test.txt'.
File name: 'C:\ConsoleApp1\bin\Debug\net5.0\test.txt'
   at System.IO.FileStream.ValidateFileHandle(SafeFileHandle fileHandle)
   at System.IO.FileStream.CreateFileOpenHandle(FileMode mode, FileShare share, FileOptions options)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share)
   at System.IO.File.Open(String path, FileMode mode)
   at ConsoleApp1.Program.ProcessFile() in C:\ConsoleApp1\Program.cs:line 23
   at ConsoleApp1.Program.Main() in C:\ConsoleApp1\Program.cs:line 13

반면, "throw ex"를 하게 되면,

try
{
    ProcessFile();
}
catch (Exception ex)
{
    throw ex;
}

외부의 catch 문맥에서 새롭게 예외를 throw 한 것이 되므로 다음과 같이 실제 예외가 발생한 호출 스택 정보를 잃어버리게 됩니다.

Unhandled exception. System.IO.FileNotFoundException: Could not find file 'C:\ConsoleApp1\bin\Debug\net5.0\test.txt'.
File name: 'C:\ConsoleApp1\bin\Debug\net5.0\test.txt'
   at ConsoleApp1.Program.Main() in C:\ConsoleApp1\Program.cs:line 17

만약 별도 예외 인스턴스를 만들어 처리하는 경우라면, 반드시 inner exception 매개변수에 이전 예외 인스턴스를 전달하는 것이 좋습니다.

catch (Exception ex)
{
    throw new MyException("error occurs", ex); // 이전 예외를 inner exception으로 연결
    
    // throw new MyException("error occurs"); // 이전 예외 정보를 날리므로 좋지 않은 사례
}

참고로, 비주얼 스튜디오에서 "throw ex"와 같은 식으로 코딩을 하게 되면 아예 정적 코드 분석기에 의해 빨간색 밑줄이 그어지면서 "CA2200: Re-throwing caught exception changes stack information" 경고가 뜹니다.

---

• static HttpClient를 사용

제 글에서도 관련 내용을 다룬 적이 있습니다.

HttpClient와 HttpClientHandler의 관계
; https://www.sysnet.pe.kr/2/0/12024

C# - HttpClient에서의 ephemeral port 재사용
; https://www.sysnet.pe.kr/2/0/12449

따라서, HttpClient는 static으로 사용되도록 충분한 설계가 되어 있기 때문에 굳이 인스턴스 생성 단위로 사용할 필요는 없습니다. 게다가 소켓의 TIME_WAIT 문제까지 겹치면 더더욱 static 유형의 사용이 권장됩니다.

• DNS 변화를 모르는 HttpClient

.NET Core의 최신 버전에서는 개선되었다고 하는데, .NET Framework 4.x에 포함된 HttpClient는 DNS 변화를 인지하지 않는다고 합니다.

• 헤더 변경 시 HttpClient의 HttpHeaders에서 접근할 것인지, HttpRequestMessage에서 접근할 것인지?

2가지 모두 HTTP Header 설정이 가능한데, HttpClient.DefaultRequestHeaders.Add() 메서드는 (HttpClient를 static 유형으로 사용하므로) 요청 전체에 걸쳐서 헤더를 바꾸는 반면, HttpRequestMessage.Headers.Add 메서드는 개별 요청 시에만 헤더를 바꿉니다.

또한 주의할 점이 있다면, HttpClient는 thread-safe하지만 그것의 DefaultRequestHeaders 속성의 타입은 thread-safe하지 않아 외부로의 요청이 발생하는 동안에는 DefaultRequestHeaders로 변경하는 경우 InvalidOperationException 예외가 발생할 수 있다고 합니다.

Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.

• 재요청

Polly에 의해, 실패한 요청에 대한 재요청을 하는 경우에도 InvalidOperation 예외가 발생한다고 합니다.

InvalidOperation exception: The request message was already sent. Cannot send the same request message multiple times.

왜냐하면 Poly가 요청을 그대로 재시도하기 때문인데, HttpClient의 경우 이전 요청과 동일한 경우 위의 InvalidOperation 예외를 발생시킨다고 합니다. 따라서, 이를 우회하기 위해 Clone을 하면 된다고!

public static HttpRequestMessage Clone(this HttpRequestMessage req)
{
    HttpRequestMessage clone = new HttpRequestMessage(req.Method, req.RequestUri);

    clone.Content = req.Content;
    clone.Version = req.Version;

    foreach (KeyValuePair<string, object> prop in req.Properties)
    {
        clone.Properties.Add(prop);
    }

    foreach (KeyValuePair<string, IEnumerable<string>> header in req.Headers)
    {
        clone.Headers.TryAddWithoutValidation(header.Key, header.Value);
    }

    return clone;
}

참고로, .NET Core 2.1에서 이 문제를 해결했는데, 아쉽게도 Microsoft의 DI 체계에 의존적이라는 단점이 있다고 합니다.

---

• IEnumerable보다 ToList를 사용해야 할 때

foreach 내에서 await을 호출하는 경우,

foreach (var item in ...)
{
    await HandleItemAsync(item);
}

문제가 되는 경우가 있는데, 예를 들어 열거 작업에 원격지 호출을 포함하는 Azure Blob을 사용하는 다음의 코드는,

var files = container.ListBlobs(null, true, BlobListingDetails.None);// List all the files in the blob. It will return an IEnumerable object.
foreach (var file in files)
{
    await file.DeleteIfExistsAsync();
}

files 개체가 IEnumerable 지연 처리를 하므로, foreach 루프 내부의 await 비동기 처리로 인해 전체 소요 시간이 늘어나면 files의 열거 작업에 time-out이 발생할 수 있습니다. 따라서, 그런 상황을 고려한다면 지연 처리를 하는 IEnumerable보다는, 결과를 완료한 후에 처리가 진행되도록 ToList 확장 메서드를 거치도록 하는 것이 권장됩니다.

var files = container.ListBlobs(null, true, BlobListingDetails.None);// List all the files in the blob. It will return an IEnumerable object.
foreach (var file in files.ToList())
{
    await file.DeleteIfExistsAsync();
}

• 동일 인스턴스에 대한 다중 열거 작업

Resharper의 정적 코드 분석은 다음의 코드에 대해 경고를 발생시킵니다.

IEnumerable<string> names = GetNames();
foreach (var name in names)
    Console.WriteLine("Found " + name);

var allNames = new StringBuilder();
foreach (var name in names)
    allNames.Append(name + " ");

IEnumerable로 인한 지연 처리가 DB 또는 API 호출에 의해 발생한다고 가정하면, 첫 번째 열거 작업 이후 두 번째 열거 작업 사이에 대상 데이터의 변화가 발생했을 수 있고, 따라서 2번의 열거 작업은 동일한 데이터를 반환하지 않을 수 있습니다. 따라서 이런 경우에도 ToList를 활용해 결과를 모두 받아온 후 그것을 재사용하는 것이 권장됩니다.

List<string> names = GetNames().ToList();
foreach (var name in names)
    Console.WriteLine("Found " + name);

var allNames = new StringBuilder();
foreach (var name in names)
    allNames.Append(name + " ");

• Concurrent 컬렉션

List나 Dictionary는 thread-safe하지 않습니다. 따라서, 다중 스레드에서 사용해야 한다면 (물론 thread-safe한 컬렉션을 만들어도 되지만) System.Collections.Concurrent 네임스페이스 아래에서 제공하는 thread-safe 버전의 컬렉션 사용을 권장합니다.

---

• == 또는 Equals

이것은 다음의 내용을 참조하시고.

== 연산자보다는 Equals 메서드의 호출이 더 권장됩니다.
; https://www.sysnet.pe.kr/2/0/2878

• 추상 클래스에는 protected 생성자를 사용

이유 불문!

• 이벤트 패턴

이벤트와 연관된 상태 정보가 있는 경우, 종종 이벤트를 먼저 발생시키고 상태 정보를 업데이트하는 코드를 보게 되는데 이에 대한 각별한 주의가 필요!

• 사용하지 않는 코드는 제거

사용하지 않는 코드가 있다면, 현재 버전에서는 Obsolete 처리를 하고 다음 버전에서는 (주석 처리를 하는 경우를 종종 보게 되는데) 그냥 삭제합니다.

• 메서드에 너무 많은 매개변수를 사용하지 않는다.

매개변수가 많아지면 별도의 타입을 만들어 매개변수를 담아 전달하도록 코드 변경

---

[이 글에 대해서 여러분들과 의견을 공유하고 싶습니다. 틀리거나 미흡한 부분 또는 의문 사항이 있으시면 언제든 댓글 남겨주십시오.]

[최초 등록일: 12/21/2020]
[최종 수정일: 6/23/2022]


이 저작물은 크리에이티브 커먼즈 코리아 저작자표시-비영리-변경금지 2.0 대한민국 라이센스에 따라 이용하실 수 있습니다.

by SeongTae Jeong, mailto:techsharer at outlook.com