현재 진행 중인 프로젝트의 객체지향적 리팩터링 과정을 기록해보고자 한다.
구체적인 리팩터링 과정에 앞서 리팩터링이란 무엇인지 짧게 설명하자면
Refactoring이란 외부 동작을 바꾸지 않으면서 내부 구조를 개선하는 방법이다. 즉, 결과는 바뀌지 않지만 내부적으로 코드의 구조 혹은 코드의 디자인을 개선시켜서 코드의 가독성과 유연성, 유지보수성등을 향상하는데 목적이 있다.
지금 쓰는 글이 단순히 리팩터링에 대한 글이라고 생각하지는 않았으면 한다.
내가 한 리팩터링은 사실 리팩터링보다는 구조 개선에 가까운 코드 수정이며, 진행하면서 많은 것들이 바뀐다.
다만 객체지향설계와 객체지향프로그래밍을 공부하면서 최대한 프로젝트를 객체지향적으로 고치고자 하기에,
코드를 고쳐가며, 어떤 문제점들이 있었는지 자세한 코드와 함께 해결과정을 기록해놓았다.
코드를 수정하면서 여러 문제점들이 발생하기에, 다소 글이 중구난방일 수 있겠지만, 코딩 초보의 객체지향적 해결과정을 보며, 함께 객체지향에 대한 고민을 해보았으면 하는 바램이다.
현재 내가 진행중인 프로젝트는 주식 시뮬레이션 프로젝트이다. 가상의 돈으로 주식을 사고팔고 할 수 있으며, 실제 주식 가격을 바탕으로 실시간으로 변화하는 주식 정보를 볼 수 있다.
코드 리팩터링은 매도, 매수 기능에서 이루어지며, 총 2단계로 진행되었다.
먼저 초기의 코드다.
매수 매도 기능에 있어서 주로 협력에 참여하는 객체는 Account, Trade다.
Account는 고객의 주식계좌의 역할을 맡은 객체이며, Trade 객체는 거래를 진행함에 있어 어떤 주식을 몇 개 팔 것인지, 거래 종류는 매도인지 매수인지를 나타내는 객체이다.
일단 코드를 보자.
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
@Entity
public class Account {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private long id;
private long money;
@OneToMany
private Map<Stock, Integer> stocks;
@OneToOne(mappedBy = "account")
private Member member;
@OneToMany(mappedBy = "account")
private List<Trade> trades;
@OneToMany(mappedBy = "account")
private List<Trade> trades;
public void buy(Trade trade) {
long priceToPayment = trade.getQuantity() * trade.getStock().getPrice();
if (this.money >= priceToPayment) {
this.money -= priceToPayment;
this.trades.add(trade);
} else {
throw new IllegalArgumentException("not enough money");
}
}
public void sell(Trade sellTrade) {
List<Trade> sells = trades.stream()
.filter(trade -> trade.getStock().getCode().equals(sellTrade.getStock().getCode()) &&
trade.getTradeType().equals(TradeType.BUY))
.sorted(Comparator.comparingInt(Trade::getQuantity).reversed())
.toList();
int quantity = sellTrade.getQuantity();
for (Trade trade : sells) {
if (quantity <= 0) {
break;
}
int tradeQuantity = trade.getQuantity();
if (tradeQuantity >= quantity) {
trade.sell(quantity);
this.money += quantity * sellTrade.getStock().getPrice();
quantity = 0;
} else {
this.money += tradeQuantity * sellTrade.getStock().getPrice();
quantity -= tradeQuantity;
}
}
}
}@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
@Entity
public class Trade {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private long id;
@ManyToOne
@JoinColumn(name = "account_id")
private Account account;
@ManyToOne
@JoinColumn(name = "stock_id")
private Stock stock;
private int quantity;
@Enumerated(EnumType.STRING)
private TradeType tradeType;
@Builder
public Trade(Account account, Stock stock, int quantity, TradeType tradeType) {
this.account = account;
this.stock = stock;
this.quantity = quantity;
this.tradeType = tradeType;
}
public void sell(int quantity) {
this.quantity -= quantity;
}
}
초기에는 이렇게 entity가 정의되어 있고, 아래와 같은 TradeService에서 trade 메서드를 통해 거래 종류에 맞게 switch문으로 매도와 매수 기능을 진행하는 방식이었다.
@RequiredArgsConstructor
@Service
public class TradeService {
private final TradeRepository tradeRepository;
private final MemberRepository memberRepository;
private final StockRepository stockRepository;
@Transactional
public boolean trade(String memberEmail, TradeRequestDto dto, TradeType tradeType) {
Member member = memberRepository.getMemberByEmail(memberEmail);
Stock stock = stockRepository.findByCode(dto.stockCode())
.orElseThrow(NoSuchElementException::new);
Account account = member.getAccount();
Trade trade;
switch (tradeType) {
case BUY :
trade = Trade.builder()
.account(account)
.stock(stock)
.quantity(dto.quantity())
.tradeType(TradeType.BUY)
.build();
account.buy(trade);
tradeRepository.save(trade);
break;
case SELL :
trade = Trade.builder()
.account(account)
.stock(stock)
.quantity(dto.quantity())
.tradeType(TradeType.BUY)
.build();
account.sell(trade);
tradeRepository.save(trade);
break;
}
return true;
}
}
하지만 현재 코드에서는 여러 문제점들이 보였다
첫째, Account 객체에서의 불필요한 양방향 매핑이 존재한다.
둘째, 의존관계가 올바르지 않다.
셋째, 확장성이 떨어진다.
따라서 천천히 문제점들을 하나하나 고쳐가며 첫번째 리팩터링을 진행해 보겠다.
먼저, 첫 번째 문제 Account 객체에서의 불필요한 양방향 매핑이 존재한다.
Account에서는 현재 Trade List와 TradeTrace List들을 양방향으로 매핑하여 참조하고 있다.
( 여기서 TradeTrace는 거래를 목적으로 한 Trade와는 다르게 단순 거래 기록을 위한 객체이다. )
하지만 양방향 매핑의 경우 반대방향으로 그래프 탐색이 가능하도록 하여, 개발에 편리함을 주는 장점이 있는 반면, 무한루프참조 문제를 발생시키는 주요 원인이 될 수 있다.
양방향 매핑에 관해 많은 글들을 찾아본 결과 그래프 탐색을 사용하지 않을 때, 코드의 복잡성이나 구현 자체가 어려울 경우에만 사용하고, 그 외에는 무한 참조를 방지하기 위해 지양하고 있다는 것을 알게 되었다.
따라서 나는 Account 객체에 있는 불필요한 양방향 매핑 관계를 없애보고자 한다.
먼저 Trade에 관한 양방향 매핑이다.
Account의 코드를 보면 sell이라는 매도 메서드에서 양방향 매핑을 통한 그래프 탐색을 진행하고 있는 것을 볼 수 있다.
( Account의 메서드 sell )
public void sell(Trade sellTrade) {
List<Trade> sells = trades.stream()
.filter(trade -> trade.getStock().getCode().equals(sellTrade.getStock().getCode()) &&
trade.getTradeType().equals(TradeType.BUY))
.sorted(Comparator.comparingInt(Trade::getQuantity).reversed())
.toList();
int quantity = sellTrade.getQuantity();
for (Trade trade : sells) {
if (quantity <= 0) {
break;
}
int tradeQuantity = trade.getQuantity();
if (tradeQuantity >= quantity) {
trade.sell(quantity);
this.money += quantity * sellTrade.getStock().getPrice();
quantity = 0;
} else {
this.money += tradeQuantity * sellTrade.getStock().getPrice();
quantity -= tradeQuantity;
}
}
이것은 단순히 "sell에서 그래프 탐색을 사용하니 이 양방향은 필요해!"라고 생각하면 안 된다.
이렇게 양방향 매핑이 존재하고, 양방향 매핑을 통한 탐색을 사용하고 있을 때는 과연 양방향 매핑을 통한 탐색을 사용하는 메서드가 이 객체의 책임이 맞는지부터 판단을 해야 한다.
그럼 먼저 왜 sell에서 양방향 매핑을 필요로 하는지부터 알아보자, 내가 구성한 주식 매도 매수 기능은 매수를 할 경우 사용자의 Account와 주식 정보를 담은 Trade 객체가 생성되어서 저장되며, 매도를 할 경우 사용자의 Account에 있는 매수 Trade들 중에서 매도하고자 하는 주식 정보를 가지고 있는 Trade들을 하나씩 제거해 나가며 매도가 진행되는 방식이다.
이때, Account가 가진 Trade들 중 원하는 주식 정보를 가지고 있는 Trade들을 Account에서 탐색하기 위해서 양방향 매핑이 필요한 것이다.
그렇다면 고려해 볼 것은 두 가지이다.
첫째, 매도 매수 기능 로직을 Account 객체에서 처리하는 것이 맞는가?
둘째, 매도 매수 기능에서 Trade 탐색이 없이 처리할 수는 없는가?
첫 번째 경우먼저 생각해 보자, Account의 역할은 사용자의 자산을 관리해 주는 역할이다. 그렇다면 자신의 보유 잔액을 빼고 더하는 기능만 가져야지만 이러한 역할에 충실할 수 있다고 볼 수 있다. 매도와 매수의 구체적인 로직의 경우에는 전체적인 협력에 있어서 Account의 역할이라고 보기보단, Service레이어에서 Trade 객체와 Account 객체를 사용하여서 처리하는 것이 더 객체지향적이라고 판단되었다.
이미 첫 번째의 경우에서 매도와 매수의 구체적인 로직이 Account 객체의 책임이 아니라는 것을 알았지만, 두 번째의 경우도 생각해 보자.
사실 매도와 매수 기능이라고 두고 100명의 개발자들에게 이 기능을 구현하라고 시킨다면, 비슷할 수는 있겠지만, 다들 자신만의 로직을 구상하고, 그에 맞게 구현을 할 것이다. 현재 내가 매도, 매수를 이렇게 구현했다고 하여서 이 방식이 틀렸다고는 할 수 없다고 생각한다. 또한 이 방식이 틀렸다고 가정하고 Trade 참조 없이 매도와 매수를 하도록 바꾸는 것도 생각해 봤지만, 그러면 배보다 배꼽이 더 커지게 된다.
이는 매도, 매수 기능에 대한 리팩터링 과정이지, 프로젝트의 전체적인 구조까지 갈아엎을 필요는 없다. 만약 추후에 프로젝트를 더 진행하면서 문제가 발생한다면, 그때 가서 고민해야 할 문제라고 생각한다.
결론적으로는 매도, 매수에 대한 구체적인 로직 구현은 Account 객체의 책임이 아니므로, Trade에 대한 양방향 매핑을 삭제하고, 이를 Service레이어에서 처리하도록 바꾸는 것으로 결정되었다.
두 번째 양방향 매핑은 TradeTrace에 대한 양방향 매핑이다.
이번 양방향 매핑에 관련된 리팩터링은 굉장히 쉬웠다. 왜냐하면 Account 객체에서 TradeTrace에 대한 탐색을 사용하고 있지 않기 때문이다.
이처럼 그저 습관적으로 양방향 매핑을 두는 것을 방지하기 위해서는 먼저 단방향으로 만들어 놓고 필요시에만 양방향으로 매핑하는 것을 연습해야 한다.
TradeTrace에 대한 양방향 매핑은 아직은 필요가 없으므로 삭제시킨다.
두 번째 문제는 의존관계가 올바르지 않다는 것이다.
Account 객체와 Trade 객체는 엄연히 다른 어그리거트에 속하는 객체이다. 즉, 각자가 해결하고 처리해야 할 도메인이 다르다는 말이다.
하지만 현재의 Account 객체에서는
public void buy(Trade trade) {
long priceToPayment = trade.getQuantity() * trade.getStock().getPrice();
if (this.money >= priceToPayment) {
this.money -= priceToPayment;
this.trades.add(trade);
} else {
throw new IllegalArgumentException("not enough money");
}
}
public void sell(Trade sellTrade) {
List<Trade> sells = trades.stream()
.filter(trade -> trade.getStock().getCode().equals(sellTrade.getStock().getCode()) &&
trade.getTradeType().equals(TradeType.BUY))
.sorted(Comparator.comparingInt(Trade::getQuantity).reversed())
.toList();
int quantity = sellTrade.getQuantity();
for (Trade trade : sells) {
if (quantity <= 0) {
break;
}
int tradeQuantity = trade.getQuantity();
if (tradeQuantity >= quantity) {
trade.sell(quantity);
this.money += quantity * sellTrade.getStock().getPrice();
quantity = 0;
} else {
this.money += tradeQuantity * sellTrade.getStock().getPrice();
quantity -= tradeQuantity;
}
}
}
이와 같이 메서드에서 Trade 객체를 받아오고 있다. 이는 다른 어그리거트로의 의존관계라고 볼 수 있으며, 이것은 변경에 취약한 코드가 될 수밖에 없다.
따라서 나는 Account 객체에서 Trade 객체로의 의존을 없애주어야 한다.
앞서 첫 번째 문제인 양방향 매핑에서 Trade에 대한 양방향 매핑을 삭제시키고자 하였고, 매도, 매수의 구체적인 로직 구현을 Service레이어로 빼도록 결정하였으니 Account 객체의 buy와 sell 메서드에도 변화가 필요하다.
어떻게 변화해야 하는가 한다면, Trade에 대한 의존성을 제거하고, Account 객체의 역할에 맞게 자신의 책임인 자산 관리에만 신경 쓸 수 있도록 바꾸어야 한다.
따라서
public void buy(int quantity, long price) {
long priceToPayment = quantity * price;
if (this.money >= priceToPayment) {
this.money -= priceToPayment;
} else {
throw new IllegalArgumentException("not enough money");
}
}
public void sell(int quantity, long price) {
this.money += quantity * price;
}
이와 같이 자신의 자산에 대한 처리만 하도록 바꿔 준 후 나머지는 Service레이어에서 처리하도록 바꿔보겠다.
전체적인 설계는 객체의 설계, 즉 협력, 역할, 책임을 적절하게 나누는 것으로부터 시작하기에 일단은 객체들에 대한 문제를 해결한 뒤, Service 레이어에서의 변화는 다음 문제인 확장성이 떨어진다. 에서 함께 다뤄보도록 하겠다.
세 번째 문제, 확장성이 떨어진다.
현재는 일반 매도, 매수에 대한 요구사항만이 존재하기에 문제가 없지만, 만약 추후에 예약 매도, 매수나 지정가 매도, 매수가 추가적으로 요구된다면, 아래에 보이는 TradeService에 존재하는 switch문이 계속해서 추가되어야 한다.
switch (tradeType) {
case BUY :
trade = Trade.builder()
.account(account)
.stock(stock)
.quantity(dto.quantity())
.tradeType(TradeType.BUY)
.build();
account.buy(trade);
tradeRepository.save(trade);
break;
case SELL :
trade = Trade.builder()
.account(account)
.stock(stock)
.quantity(dto.quantity())
.tradeType(TradeType.BUY)
.build();
account.sell(trade);
tradeRepository.save(trade);
break;
}
물론 이렇게 처리를 해도 문제 되는 것은 없다. switch문 같은 경우에는 hashtable을 사용하기에, 성능면에서도 문제가 없을뿐더러, 코드의 가독성면에서도 크게 문제가 되지는 않는다.
하지만 나는 지금 객체지향적 프로그래밍에 대해 학습하며, 이 프로젝트를 진행하고 있다.
객체지향적 관점에서 보았을 때, 이러한 확장성에 대한 문제는 다형성으로 풀어나가기 마련이고, 되도록이면 if-else문이나 switch문등 코드의 depth가 늘어나지 않도록 유의하며 코드를 작성해야 한다.
따라서 이 switch문을 없애고 다형성으로 풀어나가도록 해보자.
다형성으로 풀어나가기 위해서는 추상화를 시켜야 한다.
나에겐 2가지 선택지가 있었다.
1. Trade를 추상화시켜서 요청이 들어올 때 알맞은 구현체를 생성해서 로직 처리하기
2. 따로 trade라는 협력을 하는 추상화 객체를 생성해서 trade 책임을 넘기기
나는 기존의 객체를 추상화시켜 처리하는 것이 더 바람직한 리팩터링이라고 생각했기에, 먼저 첫 번째 방법을 생각해 보았다.
Trade를 추상화시킨 후 BuyTrade, SellTrade라는 구현체들을 생성해 주고, 각자 자신에게 맞는 trade() 메서드를 오버라이딩하도록 구현해 보았다.
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
@Entity
public abstract class Trade {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private long id;
@ManyToOne
@JoinColumn(name = "account_id")
private Account account;
@ManyToOne
@JoinColumn(name = "stock_id")
private Stock stock;
private int quantity;
@Enumerated(EnumType.STRING)
private TradeType tradeType;
@Builder
public Trade(Account account, Stock stock, int quantity, TradeType tradeType) {
this.account = account;
this.stock = stock;
this.quantity = quantity;
this.tradeType = tradeType;
}
public abstract trade();
}
public class BuyTrade extends Trade {
@Override
public trade() {
}
}
이런 방식으로 다형성을 사용하여 여러 가지 Trade를 처리할 수 있도록 하려고 시도를 해보았으나, 이는 결국 Service레이어에서 똑같이 어떤 거래 타입인지를 판별해서 그에 맞는 객체를 생성해주어야 했다.
즉 리팩터링 전과 같이 switch문이 사용되는 것이었다.
이러한 switch문을 벗어나기 위해서 나는 enum 타입을 사용했다.
enum타입은 키-값으로 이루어진 하나의 자료구조라고 볼 수 있지만, 값의 안에 특정 메서드를 넣을 수 있다.
나는 enum타입을 사용해 switch문 없이 알맞은 구현 객체를 생성할 수 있도록 구현해 냈지만, 또 다른 문제가 발생했다.
현재 나의 매수, 매도 기능은 매수타입의 Trade만을 저장시켜 놓았다가 매도 주문이 들어올 경우 매수타입의 Trade의 quantity를 빼거나 Trade 자체를 삭제시키는 방식으로 진행되야 하기에, SellTrade라는 구현객체가 생성되어서 trade() 메서가 호출이 될 경우 SellTrade에서 BuyTrade들을 찾아야 하는 문제도 있을뿐더러, 매도 혹은 매수가 호출되었을 때, Account의 잔액도 업데이트가 되어야 한다.
만약 이걸 Service레이어에서 Account의 잔액을 업데이트하기 위해서는 또다시 거래타입이 무엇인지에 따라 로직이 바뀌어야 하기에 switch문이나 if-else문이 필요하게 되며, 이를 사용하지 않기 위해서는 Trade 구현체에서 Account를 의존해야 하는 상황이었다.
이렇게 여러 문제가 발생했고, 이 문제들을 해결하기 위해서는 매수, 매도 기능의 로직을 바꿔야만 했다.
나는 차분하게 다시 협력관계에 필요한 요소가 무엇인지를 생각해 보았다.
현재 내가 구상한 매수와 매도 기능을 위해 필요한 로직은
1. Account의 잔액 업데이트
2. Trade의 거래 주식 수 업데이트 혹은 새로운 Trade 생성
3. TradeTrace(거래 기록) 생성 및 저장
크게 이렇게 3가지로 볼 수 있다.
따라서 나는 AccountRepository와 TradeRepository, TradeTraceRepositoy 총 3개의 저장소가 필요하며, 여기서 TradeTrace에 관한 로직은 모든 거래타입에서 공통적으로 사용되니 배재하고 생각해도 되었다.
하지만 거래타입에 따라 Account의 잔액 업데이트 방식도 다를뿐더러, Trade의 거래 주식 수 업데이트 방식도 다르고 Trade를 새로 생성해야 할 수 도 있는 매수와는 다르게 매도는 Trade를 따로 생성하지 않는다.
이러한 문제들로 보았을 때, 나는 매수, 매도 로직 자체를 갈아엎지 않는 이상 entity 객체들의 다형성으로는 해결할 수 없다는 결론에 도달했고, 나는 두 번째 방법인 따로 trade라는 협력을 하는 추상화 객체를 생성해서 trade 책임을 넘기기를 구현하기로 했다.
하나의 trade를 처리하는 Service레이어를 만들어서 추상화시킨 다음, 이를 통해 다형성을 구현하면 해결되지 않을까 싶었고, 나는 바로 새로운 Service 객체를 만들었다.
@RequiredArgsConstructor
@Service
public abstract class Trader {
final TradeRepository tradeRepository;
@Transactional
public abstract void trade(Account account, Trade trade);
}
이를 통해 BuyTrader와 SellTrader 구현체를 만들어주었고, 똑같이 enum타입을 사용해 알맞은 구현체를 생성해 내도록 하였다.
public class BuyTrader extends Trader {
public BuyTrader(TradeRepository tradeRepository) {
super(tradeRepository);
}
@Override
public void trade(Account account, Trade trade) {
if (account.hasTrade(trade.getStockCode())) {
account.buy(trade.getQuantity(), trade.getStockPrice());
tradeRepository.findAllByAccount(account).stream()
.filter(oldTrade -> oldTrade.getStockCode().equals(trade.getStockCode()))
.findFirst()
.get()
.buy(trade.getQuantity());
} else {
account.buy(trade.getQuantity(), trade.getStockPrice());
tradeRepository.save(trade);
account.getTrades().add(trade);
}
}
}
public class SellTrader extends Trader {
public SellTrader(TradeRepository tradeRepository) {
super(tradeRepository);
}
@Override
public void trade(Account account, Trade trade) {
if (account.canSell(trade.getStockCode(), trade.getQuantity())) {
account.sell(trade.getQuantity(), trade.getStockPrice());
trade.sell(trade.getQuantity());
}
}
}
public enum TraderConstructor {
BUY {
@Override
public Trader createTrader(TradeRepository tradeRepository) {
return new BuyTrader(tradeRepository);
}
},
SELL {
@Override
public Trader createTrader(TradeRepository tradeRepository) {
return new SellTrader(tradeRepository);
}
};
public abstract Trader createTrader(TradeRepository tradeRepository);
}
이렇게 아래와 같이 Trader라는 거래를 담당하는 Service레이어를 따로 만들어내니, 다형성을 통한 거래가 가능해졌다.
@RequiredArgsConstructor
@Service
public class TradeService {
private final TradeTraceService traceService;
private final TradeRepository tradeRepository;
private final MemberRepository memberRepository;
private final StockRepository stockRepository;
@Transactional
public boolean trade(String memberEmail, TradeRequestDto dto, TraderConstructor traderConstructor, TradeType tradeType) {
Member member = memberRepository.getMemberByEmail(memberEmail);
Stock stock = stockRepository.findByCode(dto.stockCode())
.orElseThrow(NoSuchElementException::new);
Account account = member.getAccount();
Trade trade = Trade.builder()
.account(account)
.stock(stock)
.quantity(dto.quantity())
.tradeType(tradeType)
.build();
Trader trader = traderConstructor.createTrader(tradeRepository);
trader.trade(account, trade);
traceService.recordTrace(trade);
return true;
}
이처럼 TradeService에는 switch문이 삭제되었지만, 이렇게 구현하면서 문제가 발생했다.
Account에서의 Trade 양방향 매핑이 필요해진 것이다.
매수와 매도의 경우 사용자의 Account가 해당 주식에 대한 Trade를 가지고 있는지, 혹은 매도하려는 수만큼 주식을 보유하고 있는지에 대해 알려줄 책임이 있었고, Account는 다시 필요에 의해 Trade양방향 매핑을 가지게 되었다.
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
@Entity
public class Account {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private long id;
private long money = 0;
@OneToOne(mappedBy = "account")
private Member member;
@OneToMany(mappedBy = "account")
private List<Trade> trades = new ArrayList<>();
@Builder
public Account(Member member) {
this.member = member;
}
public void buy(int quantity, long price) {
long priceToPayment = quantity * price;
if (this.money >= priceToPayment) {
this.money -= priceToPayment;
} else {
throw new IllegalArgumentException("not enough money");
}
}
public void sell(int quantity, long price) {
this.money += quantity * price;
}
public boolean canSell(String stockCode, int quantity) {
return trades.stream()
.anyMatch(trade -> trade.getStockCode().equals(stockCode)) &&
trades.stream()
.filter(trade -> trade.getStockCode().equals(stockCode))
.findFirst().get().getQuantity() >= quantity;
}
public boolean hasTrade(String stockCode) {
return trades.stream().anyMatch(trade -> trade.getStockCode().equals(stockCode));
}
}
위에 보이는 canSell() 메서드와 hasTrade() 메서드가 양방향 매핑의 필요성이다.
여기까지가 내가 진행한 1차 리팩터링이다.
2차 리팩터링
여기까지 진행한 뒤 PR을 올렸고 멘토님에게 PR 리뷰를 받았다.
사실 난 자신 있었다. switch문을 다형성으로 풀어내고 객체 간의 의존성을 없애는데 자그마치 3일이라는 시간을 모두 쏟아부어야 했기에, 나는 나의 노력의 산물을 빠르게 평가받고 싶었다.
하지만 리뷰는 처참했다..
멘토님과 같이 코드를 보며 리뷰를 진행했는데, 확실히 나는 갈길이 멀다.
내가 객체지향이라고 생각하며 작성한 코드는 객체지향적으로 굉장히 문제가 많았다.
그래서 리뷰를 바탕으로 다시 2차 리팩터링을 진행해 보도록 하겠다.
문제점들은 많았다.
네이밍 문제, 코드의 복잡성 문제, 코드의 직관성 문제 등등.... 여러 가지 문제들이 있었고, 멘토님은 이걸 하나하나 다 고치지 말고 일단 먼저 하나의 Service레이어에서 모두 다 처리하도록 다시 합쳐보고, 그다음 천천히 다시 나눠보라고 하셨다.
Service레이어 하나에서 다 처리했을 때, 오히려 더 직관성 있고, 좋은 코드가 될 수 도 있다고 말이다.
그래서 나는 일단 추상화로 나눠놨던 Service레이어를 다 하나로 합쳐볼 것이다.
@RequiredArgsConstructor
@Service
public class TradeService {
private final TradeTraceService traceService;
private final TradeRepository tradeRepository;
private final MemberRepository memberRepository;
private final StockRepository stockRepository;
@Transactional
public boolean buy(String memberEmail, TradeRequestDto dto) {
Member member = memberRepository.getMemberByEmail(memberEmail);
Stock stock = stockRepository.findByCode(dto.stockCode())
.orElseThrow(NoSuchElementException::new);
Account account = member.getAccount();
if (account.hasTrade(stock.getCode())) {
account.buy(dto.quantity(), stock.getPrice());
tradeRepository.findAllByAccount(account).stream()
.filter(oldTrade -> oldTrade.getStockCode().equals(stock.getCode()))
.findFirst()
.get()
.buy(dto.quantity());
} else {
Trade trade = Trade.builder()
.account(account)
.stock(stock)
.tradeType(TradeType.BUY)
.quantity(dto.quantity())
.build();
account.buy(dto.quantity(), stock.getPrice());
tradeRepository.save(trade);
account.getTrades().add(trade);
}
traceService.recordTrace(account, stock, dto.quantity(), TradeType.BUY);
return true;
}
@Transactional
public boolean sell(String memberEmail, TradeRequestDto dto) {
Member member = memberRepository.getMemberByEmail(memberEmail);
Stock stock = stockRepository.findByCode(dto.stockCode())
.orElseThrow(NoSuchElementException::new);
Account account = member.getAccount();
if (account.canSell(stock.getCode(), dto.quantity())) {
account.sell(dto.quantity(), stock.getPrice());
Trade trade = tradeRepository.findAllByAccount(account).stream()
.filter(hasTrade -> hasTrade.getStockCode().equals(stock.getCode()))
.findFirst().get();
trade.sell(dto.quantity());
traceService.recordTrace(account, stock, dto.quantity(), TradeType.SELL);
}
return true;
}
하나의 Service에 기능을 다 넣어버린 모습이다.
다형성도 존재하지 않고, 새로운 타입의 거래가 생기면 새로 코드를 추가시키면 된다.
과연 이전의 다형성을 사용한 코드와 지금 합친 이 코드는 어떤 차이가 있을까 한번 생각해 보자.
사실 합치기 전의 코드를 작성하면서도 극단적인 객체지향을 추구하는 것이니 다형성을 사용하면서 코드의 복잡성이 올라가도 그러려니 했다. 객체지향의 특성을 이용해 switch문을 대체했고, 동적으로 추상화된 객체의 구현체를 알맞게 생성하는 코드를 보고 참 잘 짰다고 생각했지만,
코드를 다시 하나로 합쳐보니 이게 훨씬 더 복잡도가 낮고, 한눈에 보기가 쉬운 느낌이다.
물론 요구사항이 늘어나면 늘어날수록 한 Service 클래스의 코드 라인수가 계속 늘어날 테니 복잡도는 합치기 전보다 더 높아지겠지만, 현재까지는 합친 게 훨씬 더 보기가 좋다.
그렇다면 내가 코드를 합치기 전에 놓친 부분이 무엇일까?
내가 생각하는 아쉬운 부분들이다.
1. 네이밍
2. 도메인이 아닌 부분에서의 상속
이 두 가지가 떠오른다.
먼저 네이밍 같은 경우에는 Service레이어에서 가질만한 네이밍이 아니었다.
Trader라는 네이밍은 Service 클래스보단 도메인계층의 entity 객체에 더 어울리는 네이밍이며, 이외에도 Trader에 포함되어 있는 trade()라는 추상메서드 때문에 TradeService에서 trade메서드를 실행하고, 그 안에서 Trader를 만들고 Trader가 trade메서드를 실행하는 보기만 해도 복잡한 구조가 되어 버렸다.
두 번째로 도메인이 아닌 부분에서의 상속이다.
객체지향은 객체를 지향하는 프로그래밍 기법이다. 하지만 Service클래스를 객체라고 할 수 있을까?
물론 객체라고는 할 수 있기야 하겠지만 우리가 원하는 객체는 도메인 영역에서의 객체이다.
따라서 도메인영역에서의 객체지향이 아니면 사실 크게 의미가 있을까 싶다.
따라서 나는 이러한 문제점들을 바탕으로 다시 한번 리팩터링을 진행해 보겠다.
다시 처음으로 돌아와 차분하게 생각을 해보면, 객체지향 프로그래밍의 설계는 협력, 역할, 책임을 분할하는 것부터 시작한다.
현재 내 협력은 "거래"라는 협력이고, 이 협력에 참여하는 객체는 현재 Account라는 계좌 객체와 Trade라는 거래 객체다.
이 두 객체는 서로 다른 어그리거트로 서로가 서로에게 직접적으로 메시지를 보내거나 서로 의존해서는 안된다.
하지만 현재 "거래"라는 협력 즉 매도, 매수를 위해서는 Trade와 Account에 둘 다 접근해서 알맞은 로직이 수행되어야 한다.
따라서 Trade를 추상화시켜서 다형성을 통해 확장성을 늘린다고 할지라도, Trade 구현체들에게 알맞는 "거래" 로직을 수행하도록 하기 위해서는 결국 Trade 구현체에서 Account에 의존해야 한다는 것이다.
물론 Service레이어에서 따로 빼서 "거래" 협력을 수행하도록 할 수 있다. 그것이 현재 내 프로젝트 상태이고, 이제 이를 다형성을 통해 해결해야 한다.
정말 간단하게 그려본 관계도다.
Client가 매수 혹은 매도 요청을 할 경우 Trade를 새로 생성하거나 Trade의 quantity 값을 더하거나 뺀다.
그다음 Account의 상태인 money의 값을 거래 타입에 맞게 더하거나 뺀다.
이런 관계도를 가만히 들여다보고 있다 보니 한 가지 생각이 떠올랐다.
Trade가 꼭 Entity로 있어야 하는가?
Entity로 써야 하는가를 판단할 때에는 이것이 식별자가 필요한가를 판단해야 한다.
하지만 현재는 Trade를 식별자값으로 조회하거나 식별자값이 필요한 부분은 존재하지 않는다.
그러면 Trade를 Entity가 아닌 VO로 만들어서 Account의 상태값으로 둔다면 어떨까?
그다음 Account에서 Trade를 관리하도록 하는 것이다.
이렇게 하면 Trade를 상속시켜서 여러 Trade 구현체를 만들 수 있고, 해당 Trade 생성을 Service 레이어에게 넘기면 다형성으로 확장성도 챙길 수 있을 것 같다.
한번 시도해 보자.
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
@Entity
public class Account {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private long id;
private long money = 0;
@OneToOne(mappedBy = "account")
private Member member;
@ElementCollection(fetch = FetchType.LAZY)
@CollectionTable(name = "trades", joinColumns = @JoinColumn(name = "account_id"))
private List<Trade> trades = new ArrayList<>();
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
@Embeddable
public abstract class Trade {
protected String stockCode;
protected int quantity;
@Builder
public Trade(String stockCode, int quantity) {
this.stockCode = stockCode;
this.quantity = quantity;
}
public abstract void proceed(Account account, long price);
public void buy(int quantity) {
this.quantity += quantity;
}
public void sell(int quantity) {
this.quantity -= quantity;
}
}
이렇게 Trade를 Entity에서 VO로 바꿔주었다.
이런 관계가 흐름상 더 적절한 것은 맞다고 생각되나, Trade를 추상클래스로 만들어서 구현체들을 동적으로 생성해 알맞은 로직을 처리하도록 하기 위해서는 Trade가 Account를 참조할 수밖에 없는 상황이었다.
즉 Account에 대한 캡슐화가 깨진 것이다.
물론 이렇게만 한다면 다형성을 통해 유연하게 확장은 가능하다.
원하는 Trade를 새로 만든 다음 처리 로직을 proceed에 구현하면 끝이기 때문이다.
하지만 캡슐화가 깨지는데 과연 이게 좋은 코드일까?
public class SellTrade extends Trade {
@Override
public void proceed(Account account, long price) {
if (account.canSell(stockCode, quantity)) {
account.sell(quantity, price);
Trade targetTrade = account.getTrades().stream()
.filter(hasTrade -> hasTrade.getStockCode().equals(stockCode))
.findFirst()
.get();
targetTrade.sell(quantity);
account.deleteCompletedTradesAsync();
}
}
}
예를 들면 SellTrade의 경우 proceed 메서드에서 Account 객체 자체를 받고 있기 때문에 Account의 캡슐화가 깨진 것을 볼 수 있다.
그렇다면 캡슐화를 지켜주면 된다.
캡슐화를 지키는 방법은 간단하다, 내부의 상태에 직접적으로 접근을 하는 것이 아닌 공개된 인터페이스만으로만 접근이 가능하도록 바꾸면 캡슐화는 이루어진다.
따라서 Account 내부 상태인 List <Trade>에 대한 업데이트 인터페이스를 추가해 주면 될 것이다.
한번 추가해 보자.
코드가 좀 길어졌다.
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Entity
public class Account {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private long id;
@Getter
private long money = 0;
@OneToOne(mappedBy = "account")
private Member member;
@ElementCollection(fetch = FetchType.LAZY)
@CollectionTable(name = "trades", joinColumns = @JoinColumn(name = "account_id"))
private List<Trade> trades = new ArrayList<>();
@Builder
public Account(Member member) {
this.member = member;
}
public void buyTrade(int quantity, long price, Trade trade) {
buy(quantity, price);
addNewTradeOrIncreaseTradeQuantity(trade);
}
public void sellTrade(int quantity, long price, Trade trade) {
Trade targetTrade = findTradeCanSell(trade.getStockCode(), quantity);
sell(quantity, price);
targetTrade.sell(quantity);
deleteCompletedTradesAsync();
}
private void addNewTradeOrIncreaseTradeQuantity(Trade newTrade) {
Optional<Trade> existingTrade = hasTrade(newTrade.getStockCode());
if (existingTrade.isPresent()) {
existingTrade.get().buy(newTrade.getQuantity());
} else {
trades.add(newTrade);
}
}
private void buy(int quantity, long price) {
long priceToPayment = quantity * price;
if (this.money >= priceToPayment) {
this.money -= priceToPayment;
} else {
throw new IllegalArgumentException("not enough money");
}
}
private void sell(int quantity, long price) {
this.money += quantity * price;
}
private Trade findTradeCanSell(String stockCode, int quantity) {
Optional<Trade> candidateTrade = trades.stream().filter(trade -> trade.getStockCode().equals(stockCode)).findFirst();
boolean response = candidateTrade.isPresent() && candidateTrade.get().getQuantity() >= quantity;
if (response) {
return candidateTrade.get();
} else {
throw new IllegalArgumentException("not enough stocks");
}
}
private Optional<Trade> hasTrade(String stockCode) {
return trades.stream().filter(trade -> trade.getStockCode().equals(stockCode)).findFirst();
}
@Async
protected void deleteCompletedTradesAsync() {
trades = trades.stream()
.filter(trade -> trade.getQuantity() > ZeroTradeQuantity.EMPTY_TRADE.getQuantity())
.collect(Collectors.toList());
}
public long calculateBalance() {
long balance = 0;
for (Trade trade : trades) {
balance += trade.getQuantity() * trade.getStock().getPrice();
}
return balance;
}
}
Account 객체 내에 존재하는 Account의 상태값에 대해서는 외부에서 따로 참조하거나 조정할 수 없게 @Getter를 없앤 다음, 최소한의 인터페이스만을 공개해서 Account에게 메시지를 보내도록 하였다.
이를 통해
public class BuyTrade extends Trade {
@Override
public void proceed(Account account, long price) {
account.buyTrade(quantity, price, this);
}
}
public class SellTrade extends Trade {
@Override
public void proceed(Account account, long price) {
account.sellTrade(quantity, price, this);
}
}
BuyTrade와 SellTrade 즉, Trade의 구현체에서는 Account의 상태에 직접적으로 접근하는 게 아닌 공개된 인터페이스만을 사용해서 접근하게 되었고, Account의 캡슐화가 지켜졌다.
이제는 다른 Trade타입이 새로 생겨야 한다고 할지라도, 새로운 Trade 구현체를 하나 만들어서 그에 맞는 proceed 메서드를 오버라이딩해서 재정의 해주면 끝이다.
@RequiredArgsConstructor
@Service
public class TradeService {
private final TradeTraceService traceService;
private final MemberRepository memberRepository;
private final StockRepository stockRepository;
@Transactional
public boolean trade(String memberEmail, TradeRequestDto dto, TradeConstructor tradeConstructor, TradeType tradeType) {
Member member = memberRepository.getMemberByEmail(memberEmail);
Stock stock = stockRepository.findByCode(dto.stockCode())
.orElseThrow(NoSuchElementException::new);
Account account = member.getAccount();
Trade trade = tradeConstructor.createTrade(stock.getName(), dto.stockCode(), dto.quantity());
trade.proceed(account, stock.getPrice());
traceService.recordTrace(account, stock, dto.quantity(), tradeType);
return true;
}
}
public enum TradeConstructor {
BUY {
@Override
public Trade createTrade(String stockName, String stockCode, int quantity) {
return BuyTrade.builder()
.stockName(stockName)
.stockCode(stockCode)
.quantity(quantity)
.build();
}
},
SELL {
@Override
public Trade createTrade(String stockName, String stockCode, int quantity) {
return SellTrade.builder()
.stockName(stockName)
.stockCode(stockCode)
.quantity(quantity)
.build();
}
};
public abstract Trade createTrade(String stockName, String stockCode, int quantity);
}
이처럼 TradeService에서는 tradeConstructor를 통해 동적으로 알맞은 Trade 구현체를 생성할 수 있으며, 각각의 Trade 구현체들은 자신이 맡은 역할을 다할 수 있게 되었고, 이전의 코드와는 다르게 Service 계층에서의 다형성이 아닌 도메인 계층에서의 다형성을 구현해 냈다.
이렇게 Trade, "거래"라는 협력은 성공적으로 다형성을 통해 해결되었지만, 문제가 하나 생겼다.
본래의 Trade는 Entity로써 Stock과도 매핑이 되어있었기에 계좌 잔고 계산을 위한 Account의 calculateBalance() 메서드가 있었지만, 이제는 사용이 불가능해졌다.
public long calculateBalance() {
long balance = 0;
for (Trade trade : trades) {
balance += trade.getQuantity() * trade.getStock().getPrice();
}
return balance;
}
이제 얘를 처리하기 위해서는 외부에서 List <Trade>를 받아온 다음 StockRepository에서 StockPrice를 따로 받아와서 처리를 해야 한다.
이는 분명한 책임의 이동이고, 분명 이 책임의 Account의 것이다.
하지만 이미 Trade가 Entity에서 벗어난 만큼 Stock과 매핑을 할 수는 없기 때문에 어쩔 수 없이 이 메서드의 구현은 외부로 나갈 수밖에 없다.
책임을 이동시키지 않고 협력을 통해 이를 해결할 수 있는 방안이 있는가 고민해 봤지만, 떠오르는 방안이 없었고, 결국 나는 따로 Balance를 계산하는 Service 계층을 하나 만들 수밖에 없었다.
@RequiredArgsConstructor
@Service
public class BalanceService {
private final AccountService accountService;
private final StockService stockService;
public long getBalance(String email) {
long balance = 0;
Map<String, Integer> trades = accountService.getTrades(email);
List<Long> prices = stockService.getStockPriceByName(trades.keySet().stream().toList());
int index = 0;
for (int quantity : trades.values()) {
balance += prices.get(index) * quantity;
}
return balance;
}
}
Stock에 대한 정보와 Account에 대한 정보가 둘 다 필요한 만큼 Facade 패턴을 사용해서 Service 클래스를 하나 만들어주었고, 여기서 Map연산과 List연산등 여러 Collection연산이 발생하는 만큼 성능이 줄어들겠지만, 잔고 계산의 경우 실시간을 중요시하지도 않을뿐더러, 성능이 중요한 기능은 아니기 때문에 괜찮을 것 같다.
물론 Account의 책임이 이동한 것은 안 좋은 상황이다.
이렇게 2차 리팩터링까지 끝이 났다.
이제 리팩터링이 끝났으니, 마지막 단계이다.
과연 리팩터링 하기 전과 후는 어떤 차이가 있는가?
먼저 리팩터링 하기 전, Service 계층 하나에서 다 처리하던, 2차 리팩터링 시작 부분이다.
이때는 분명 책임도 명확했고, 역할도 명확했다.
다만, 현재는 기본적인 매도, 매수만 있지만 추후에 예약 매수, 예약 매도 등등 여러 거래 타입이 생긴다면, TradeService클래스에서 새로 생길 때마다 메서드를 추가시켜줘야 했다.
물론 이 방법도 괜찮다. 책임과 역할이 분명하니 문제 될 것은 없었지만, 내 생각에 이는 객체지향적이지 못했다.
객체지향이란 객체들이 주고받는 메시지를 통해서 로직을 처리하는 객체를 지향하는 방식을 의미한다.
하지만 하나의 Service에서 복잡한 로직을 다 처리하는 것은 객체들은 그저 상태만 가지고 있고, Service 클래스가 로직을 처리하는 꼴이었다.
이는 객체지향적이라고 할 수 없다고 생각한다.
반면에 리팩터링을 한 후 지금의 코드는 확실하게 객체들의 메시지를 통해 로직을 처리하고 있다.
또한 Service 계층에서의 다형성이 아닌, 도메인 계층에서의 다형성 구현으로 확장성을 챙겼기에, 더 객체지향적이라고 볼 수 있다.
아래는 내가 생각하는 각각의 장단점이다.
리팩터링 하기 전
장점
- 확장이 더 쉬울 수도 있음
- 가독성이 좋음
단점
- 객체지향적이지 않음
- 다소 복잡한 거래 타입이 생길 경우 가독성도 떨어지고, 코드가 복잡해짐
리팩터링 후
장점
- 확장에 용이함
- 객체지향적임
- 유지보수가 쉬움
단점
- 추가된 클래스 수가 많아져서 가독성이 떨어질 수 있음
- 책임의 이동으로 인한 책임의 불분명함이 존재함
이렇게 다시 한번 합쳤다가 나눠보는 방식을 거쳐봤고, 이 과정에서 나는 정말 객체지향적 설계에 대해서 많은 것을 배운 것 같다....
하루종일 몇 시간 동안 계속 책임, 역할, 협력에 대해서 생각하면서, 코드를 고쳐보고 다시 롤백하고를 반복하다 보니, 다음에는 정말 잘할 수 있을 것 같다는 생각이 든다.
물론 지금 짠 이 코드도 멘토님께 리뷰를 받은 후 다시 고쳐야 할지도 모르지만....^^ 일단 너무 뿌듯하다.
아쉬운 점은 마지막에 Balance를 계산하는 Account의 책임이 Service 계층으로 이동한 것이 나는 정말 마음에 안 든다.
하지만 이 부분은 급한 부분이 아닌 만큼 일단 넘어가고 다음에 다시 고쳐보도록 해야겠다.
'백엔드 멘토링' 카테고리의 다른 글
| 주식 투자 프로젝트 실시간 Real time DB 적용기 (9) | 2024.09.06 |
|---|---|
| 계좌 잔액 업데이트 동시성 문제 해결 과정 (5) | 2024.09.03 |
| 순환 참조를 해결해보기 (0) | 2024.02.28 |
| 좋아요 기능 성능 개선해보기 (2) (0) | 2024.02.22 |
| 좋아요 기능 성능 개선해보기 (1) (1) | 2024.02.16 |