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: 필터 API를 구현한다 #451
feat: 필터 API를 구현한다 #451
Conversation
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.
수고하셨습니다 혹시 몇가지만 확인 부탁드리겠습니다
COMPANIES("companies"), | ||
CAPACITIES("capacities"), | ||
CONNECTOR_TYPES("connectorTypes"); |
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.
type에 복수형을 하신 이유가 있나요?
@@ -0,0 +1,10 @@ | |||
package com.carffeine.carffeine.filter.dto; |
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.
확인하지 못했네요
현재 구조에서 원시 타입을 service에서 controller로 넘기는 구조입니다.
따라서 service 패키지 안으로 넣겠습니다
public record FiltersRequest( | ||
List<String> companies, | ||
List<String> capacities, | ||
List<String> connectorTypes | ||
) { |
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.
request를 이렇게 받으시는 이유가 있나요?
{[
{
"type": "company",
"name": "GA1,
},
{
"type": capacity,
"name": 50
}
]}
이런식으로 받으면 안되나요..?
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.
그렇게된다면 filter Type이 추가되더라도 해당 request의 필드를 추가하거나 변경하지 않아도 될 것 같아서요
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.
좋은 방법 감사합니다!
@Transactional | ||
public FiltersResponse addFilters(Long memberId, FiltersRequest filtersRequest) { | ||
validateRole(memberId); | ||
|
||
List<Filter> companies = saveFilters(filtersRequest.companies(), FilterType.COMPANIES); | ||
List<Filter> capacities = saveFilters(filtersRequest.capacities(), FilterType.CAPACITIES); | ||
List<Filter> connectorTypes = saveFilters(filtersRequest.connectorTypes(), FilterType.CONNECTOR_TYPES); | ||
|
||
return FiltersResponse.from(companies, capacities, connectorTypes); |
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.
저장하고 response를 return하는 이유가 있나요? 도메인 객체를 return하고 controller에서 조립해도 괜찮아 보여서요,
그리고 아까 위의 request 방식으로 하면 각 type마다 save하는 메서드를 만들지 않아도 될 것 같아요
filterRepository.findByName(filterName) | ||
.orElseThrow(() -> new FilterException(FilterExceptionType.FILTER_NOT_FOUND)); | ||
|
||
filterRepository.deleteByName(filterName); |
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.
findByName으로 찾아왔으면 해당 객체의 id로 지우는 것이 indexing 측면에서 성능이 좀 더 좋지 않을까요?
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.
수고하셨습니다 ~~
@Override | ||
public void deleteById(final Long id) { | ||
map.remove(id); | ||
this.id--; |
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.
이 부분은 필요없을 것 같아요 디비에서 칼럼이 지워진다고 id가 지워지지는 않으니깐요
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.
이 부분만 고민해보시면 좋을 것 같아요
.body(FiltersResponse.from(filterService.addFilters(memberId, filtersRequest))); | ||
} | ||
|
||
@DeleteMapping("/{filterName}") |
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.
Filter id 가 아니라 name 인 이유가 있나요?
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.
ㅎㅎ.. 반환값을 프론트와 맞춰서 id를 주지 않습니다 그래서 name으로 받고 있습니다
public static Filter of(String name, String filterType) { | ||
FilterType type = FilterType.from(filterType); | ||
|
||
if (type.isCapacities()) { |
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.
Type==cFilterType.Capacity 를 써도 좋을 것 같아요
FilterType type = FilterType.from(filterType); | ||
|
||
if (type.isCapacities()) { | ||
String capacity = makeCapacity(name); |
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.
Name 이라는 변수가 뭔가 직관적이지 않은 것 같아요
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.
사실상 필터의 이름인데 capacitiy 값은 BigDecimal이라서 그런가봐요 ㅠㅠ
그래도 확장이나 전반적으로 생각하면 filter의 명이니 name이 좋을 것 같아서 냅두겠습니다!
|
||
if (type.isCapacities()) { | ||
String capacity = makeCapacity(name); | ||
return new Filter(null, capacity, type); |
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.
null을 제외한 생성자를 만드는 것은 어떤가요?
|
||
public enum FilterType { | ||
|
||
COMPANY("company"), |
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.
이 name 이라는 필드가 언제 쓰이는지 잘 모르겠어요
is~~~~ 라는 네이밍의 메서드 대신 == 으로 바로 비교한다면 없애도 좋을 것 같아요
} | ||
|
||
private void validateRole(Long memberId) { | ||
memberRepository.findById(memberId) |
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.
메서드를 아래로 내려주세요
validateRole 대신 admin 인지 체크하면 더 좋을 것 같습니다
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.
수고하셨어요. 궁금한거 하나, 제안 하나 있는데 저는 어프룹할게요
.body(FiltersResponse.from(filterService.addFilters(memberId, filtersRequest))); | ||
} | ||
|
||
@DeleteMapping("/{filterName}") |
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 boolean isCompanies() { | ||
return this.name.equals(COMPANY.name); | ||
} | ||
|
||
public boolean isConnectorTypes() { | ||
return this.name.equals(CONNECTOR_TYPE.name); | ||
} | ||
|
||
public boolean isCapacities() { | ||
return this.name.equals(CAPACITY.name); |
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.
isCompanies(회사들인가) 라는 메서드 명이 조금 어색한 것 같아욥
📄 Summary
여러 필터가 와도 하나의 테이블에서 관리할 수록 있도록 변경하였습니다.
Filter(id, name, filter_type)의 구조를 가졌습니다.
기존에는
충전기명
같은 경우 프론트엔드와 다음과 같이 협의 했습니다.ex) 충전기명(key = "GA1", value = "광주시")
협의 후 key값만 서버에서 반환해도록 변경했고, 현재 이 key 값은 Filter 테이블에서
name
column에 해당합니다.🕰️ Actual Time of Completion
🙋🏻 More