Skip to content
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

Implement Random Selection of Register Operations When Not Using selectName #1108

Draft
wants to merge 4 commits into
base: 31-register
Choose a base branch
from

Conversation

YongGoose
Copy link

@YongGoose YongGoose commented Nov 29, 2024

Summary

resolved #1081

  1. Randomized Operation Selection:
  • Before: Always applied the first registered operation in cases of multiple registrations.
  • After: Randomly selects an operation from those registered via the registeredName method.

Open Question

  1. In the registeredName, if there are register operations with assigned priorities and others without, how should the operations without assigned priorities be handled?
  2. Is the priority field allowed to be null?

How Has This Been Tested?

  • existing tests

Is the Document updated?

Later

@YongGoose
Copy link
Author

YongGoose commented Nov 29, 2024

@seongahjo

Random selection의 경우 의도한대로 등록한 순서가 아닌, 무작위 순서로 구현을 하였습니다.
아래의 테스트를 실행한 결과 100번 중 34번 성공함을 확인했습니다.

@RepeatedTest(100)
void registeredNameWithNoSelectName() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.registeredName(
			"test",
			String.class,
			monkey -> monkey.giveMeBuilder("test")
		)
		.registeredName(
			"test2",
			String.class,
			monkey -> monkey.giveMeBuilder("test2")
		)
		.registeredName(
			"test3",
			String.class,
			monkey -> monkey.giveMeBuilder("test3")
		)
		.build();

	String actual = sut.giveMeBuilder(String.class)
		.sample();

	then(actual).isEqualTo("test");
}

image

우선 순위의 경우 Open Question에도 작성하긴 했지만, 우선순위를 배정한 register 연산과 배정하지 않은 register 연산이 있을 때 어떻게 처리를 해야 할 지 정해야 할 것 같습니다.

아래의 두 가지 방법이 떠오르긴 하는데, 한 번 의견 부탁드립니다!

  • 우선순위를 배정하지 않은 것은 임의의 우선순위를 배정한다.
  • 등록한 순서대로 우선 순위를 부여한다. (오름차순)

또한 우선순위를 null로 설정할 수 있는지도 결정해야 할 것 같습니다. ex. (@nullable)
만약 무조건 우선순위를 부여해야 한다면 별도의 API로 분리 시키는 것도 괜찮을 것 같습니다.

@YongGoose YongGoose force-pushed the feature/random-selection branch from 477d170 to 06111d8 Compare November 29, 2024 10:57
@seongahjo
Copy link
Contributor

seongahjo commented Dec 1, 2024

아래의 두 가지 방법이 떠오르긴 하는데, 한 번 의견 부탁드립니다!

넵, 말씀주신 방법이 좋아보입니다. 우선순위가 있는 register 연산이 있다면 우선순위가 있는 연산끼리 경쟁하고, 등록한 register 연산이 모두 우선순위가 없다면 모두 랜덤하게 처리하는 방향이 좋아보입니다.

이 정책은 정리해서 문서나 JavaDoc으로 남겨두면 좋을 것 같습니다.

만약 무조건 우선순위를 부여해야 한다면 별도의 API로 분리 시키는 것도 괜찮을 것 같습니다.

두가지 옵션 registerGroupregister가 일관적인 API로 우선순위를 입력받는 게 좋을 것 같은데, 이 부분을 고민해보시면 좋을 것 같습니다.

@@ -204,13 +206,18 @@ private void initializeRegisteredArbitraryBuilders(
private void initializeNamedArbitraryBuilderMap(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이러면 registerName이 아닌 register에는 적용이 안되지 않나요??

Copy link
Author

@YongGoose YongGoose Dec 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

register 연산에 모두 적용해야 하는데 registeredName을 통해 등록하는 register 연산만 적용하는 것으로 착각했네요.😅
수정하겠습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 commit에서 작업했습니다. b2abc78

검토 부탁드립니다 :)

@YongGoose
Copy link
Author

이 정책은 정리해서 문서나 JavaDoc으로 남겨두면 좋을 것 같습니다.

넵!
해당 메서드에 javadoc으로 자세히 서술하겠습니다.

두가지 옵션 registerGroup와 register가 일관적인 API로 우선순위를 입력받는 게 좋을 것 같은데, 이 부분을 고민해보시면 좋을 것 같습니다.

register 연산을 등록하는 API가 일관적인 방법으로 우선순위를 입력받는 것이 좋다고 이해하면 될까요?

@seongahjo
Copy link
Contributor

register 연산을 등록하는 API가 일관적인 방법으로 우선순위를 입력받는 것이 좋다고 이해하면 될까요?

넵, registerGroup은 reflection 혹은 ArbitraryBuilderGroup으로 입력받을 수 있고,
register는 타입 혹은 Matcher와 함께 연산을 입력할텐데요, 어떤 방식으로 우선순위를 지정해야 이 모든 방법에서 일관적으로 제공할 수 있을지 고민이 필요할 것 같습니다.

예를 들어, reflection으로 등록하는 registerGroup에서는 등록할 때 클래스에 어노테이션으로 마킹해서 우선순위를 지정할 수 있을텐데요, 다른 입력지점에서는 어노테이션으로 제공하기가 어렵습니다.

입력 방법이 일관적이지 않더라도, 사람들이 자주 쓰는 패턴이여서 인지적 비용이 적다면 일관적이지 않아도 괜찮을 것 같긴 합니다.

Signed-off-by: yongjunhong <[email protected]>
@YongGoose YongGoose force-pushed the feature/random-selection branch from 06111d8 to b2abc78 Compare December 1, 2024 08:37
@YongGoose
Copy link
Author

YongGoose commented Dec 5, 2024

우선순위 기능을 해당 커밋에서 작업했습니다. c143264

간략하게 설명하면, register, registeredName의 경우 우선순위를 받는 API를 하나 더 생성해서 진행했습니다.

  • 우선순위를 부여하지 않았을 땐 priorityInteger.MAX_VALUE로 설정해 Last Place에 배치했습니다. (기존의 API)
    아래의 코드와 같이 파라미터로 정수 값을 받도록 구현했습니다.

registeredName도 파라미터에서 priority 값을 받도록 구현했습니다.

  • 아직 문서 작업은 하지 않은 상태입니다. code 관련 작업이 끝난 뒤, javadoc 작업 하겠습니다🙂
public FixtureMonkeyBuilder register(
	Class<?> type,
	Function<FixtureMonkey, ? extends ArbitraryBuilder<?>> registeredArbitraryBuilder,
	Integer priority
) {
	return this.register(MatcherOperator.assignableTypeMatchOperator(type, registeredArbitraryBuilder), priority);
}

이때 우선순위는 0보다 커야합니다.

registerGroup의 경우 @Order 어노테이션을 통해 우선순위를 부여하는 방법과, 낱개로 우선순위를 부여하는 방법으로 구현했습니다.

Spring-Boot, JUnit 사용자로 하여끔 친숙하게 사용할 수 있도록 Order이라는 이름을 채택했습니다.

낱개로 등록하는 방법입니다.

public FixtureMonkeyBuilder registerGroup(
	ArbitraryBuilderGroup arbitraryBuilderGroup,
	Integer priority
) {
	List<ArbitraryBuilderCandidate<?>> candidates = arbitraryBuilderGroup.generateCandidateList()
		.getCandidates();

	for (ArbitraryBuilderCandidate<?> candidate : candidates) {
		this.register(
			candidate.getClassType(),
			candidate.getArbitraryBuilderRegisterer(),
			priority
		);
	}
	return this;
}

@Order 어노테이션을 통해 등록하는 방법입니다.

if (arbitraryBuilderGroup.isAnnotationPresent(Order.class)) {
	Order order = arbitraryBuilderGroup.getAnnotation(Order.class);
	this.register(actualType, registerArbitraryBuilder, order.priority());
}
this.register(actualType, registerArbitraryBuilder);

아래의 테스트 코드를 실행한 결과 예상한대로 작동함을 확인했습니다.
또한, registeredNameregister을 같이 사용하는 경우에서도 예상한대로 작동함을 확인했습니다.

register(우선순위 1), registeredName(우선순위 2) -> register 적용

@Property
void registerWithPriority() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.register(String.class, monkey -> monkey.giveMeBuilder("test"), 1)
		.register(String.class, monkey -> monkey.giveMeBuilder("test2"), 2)
		.build();

	String actual = sut.giveMeBuilder(String.class)
		.sample();

	then(actual).isEqualTo("test");
}

@Property
void registeredNameWithPriority() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.registeredName(
			"test",
			String.class,
			monkey -> monkey.giveMeBuilder("test"),
			1
		)
		.registeredName(
			"test2",
			String.class,
			monkey -> monkey.giveMeBuilder("test2"),
			2
		)
		.build();

	String actual = sut.giveMeBuilder(String.class)
		.sample();

	then(actual).isEqualTo("test");
}

@Property
void registerGroupWithPriorityAnnotation() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.registerGroup(RegisterGroup.class, RegisterGroupWithPriority.class)
		.build();

	String actual = sut.giveMeOne(SimpleObject.class)
		.getStr();
	List<String> actual2 = sut.giveMeOne(new TypeReference<List<String>>() {
	});
	ConcreteIntValue actual3 = sut.giveMeOne(ConcreteIntValue.class);

	then(actual).hasSizeBetween(4, 6);
	then(actual2).hasSizeGreaterThan(4);
	then(actual3).isEqualTo(RegisterGroup.FIXED_INT_VALUE);
}

@Property
void registerGroupWithPriority() {
	FixtureMonkey sut = FixtureMonkey.builder()
		.registerGroup(new SiblingBuilderGroup(), 1)
		.registerGroup(new ChildBuilderGroup(), 2)
		.build();

	String actual = sut.giveMeOne(SimpleObject.class)
		.getStr();
	List<String> actual2 = sut.giveMeOne(new TypeReference<List<String>>() {
	});
	ConcreteIntValue actual3 = sut.giveMeOne(ConcreteIntValue.class);

	then(actual).hasSizeBetween(4, 6);
	then(actual2).hasSizeGreaterThan(3);
	then(actual3).isEqualTo(ChildBuilderGroup.FIXED_INT_VALUE);
}

@Property
void registerWithPriorityLessThenZero() {
	thenThrownBy(() -> FixtureMonkey.builder()
		.register(String.class, monkey -> monkey.giveMeBuilder("test"), -1)
		.build()
	).isExactlyInstanceOf(IllegalArgumentException.class)
		.hasMessage("Priority must be greater than or equal to 0");
}

@YongGoose YongGoose requested a review from seongahjo December 5, 2024 05:27
import com.navercorp.fixturemonkey.FixtureMonkey;
import com.navercorp.fixturemonkey.api.matcher.MatcherOperator;

public interface MatcherInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이게 필요한 이유는 뭔가요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다음과 같이 처리를 설계하였으며, 이를 위해 인터페이스를 통해 접근하도록 구성했습니다.

하지만 MatcherMetadata와 다르게 이번 경우에는 내부 구현을 감출 필요가 없다는 생각이 드네요.

따라서 인터페이스를 삭제하고 구현 클래스에 바로 접근을 하도록 수정하려고 하는데 어떻게 생각하시나요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

새로운 인터페이스를 추가하는 이유가 뭔가요?? 새로운 관심사라고 보시는건가요??

Copy link
Author

@YongGoose YongGoose Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

관심사를 프로그램의 특정 기능이나 책임을 나타내는 독립적인 부분으로 정의한다면, 우선순위가 적용된 register 연산을 하나의 관심사로 간주할 수 있다고 생각합니다.

우선 제가 생각하는 관심사 분리의 장점은 재사용성유지보수성 향상입니다.

우선, 우선순위가 적용된 register 연산은 재사용이 가능합니다.
예를 들어, 다른 모듈에서 유사한 우선순위 기반 등록 메커니즘이 필요할 때, 이 로직을 그대로 활용할 수 있습니다.

만약 다른 case에도 사용을 하려고 한다면 Generic를 사용해 우선순위가 적용된 Object를 받는 것이 좋을 것 같습니다.

또한, 우선순위가 적용된 register 연산에서 우선순위 입력 정책을 변경하려고 할 때 시스템의 다른 부분에 영향을 주지 않고 독립적으로 변경할 수 있습니다.
예를 들어, 0 미만의 숫자를 입력하면 예외를 발생시킨다는 정책을 -100으로 수정할 때, 해당 로직만 수정하면 되므로 다른 코드에 영향을 주지 않습니다.


하지만 MatcherMetadata와 다르게 이번 경우에는 내부 구현을 감출 필요가 없다는 생각이 드네요.

해당 코멘트를 작성한 이유는, MatcherMetadata와 달리 MatcherInfo는 단일 구현체만 존재할 가능성이 높아 보였기 때문입니다. 따라서 불필요한 추상화 계층을 제거하는 것이 더 적합하다고 판단했습니다.

@seongahjo 어떻게 생각하시나요?

Copy link
Contributor

@seongahjo seongahjo Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인터페이스를 분리하면 구현에 따라서 register 연산우선순위를 가지는 register 연산이 다르게 처리되는 문제가 생길수도 있을 것 같습니다. 실행흐름이 두 개로 분리되는데 오히려 유지보수가 어려워지는 게 아닐까요? 코드가 기존 흐름대로 실행되지 않고 계속 새로운 필드를 추가하고 독립적인 실행흐름을 만드는 코드는 피하시는 게 좋아보입니다. 개인적인 생각에는, 기존 흐름을 유지하면서 새로운 기능을 제공할 수 있을지 고민이 필요해보입니다.

인터페이스를 새로 추가하는 건 큰 비용이므로 정말 불가피하지 않는 생각하시지 않는 게 좋아보입니다. 인터페이스를 나누는 기준에 대해서 다시 생각해보시는 게 좋을 것 같습니다.

예를 들어, 다른 모듈에서 유사한 우선순위 기반 등록 메커니즘이 필요할 때, 이 로직을 그대로 활용할 수 있습니다.

재사용이 될지도 모르는 코드를 추상화하는 시도는 위험한 것 같은데요, 최소한 3번 이상 쓰여야 추상화를 할지 결정하는 방향이 좋은 것 같습니다. 특히, 고려해야할 요소가 많고 유즈 케이스가 명백하지 않은 경우인 것 같아서 지금 단계에서 추상화는 위험해보입니다.
https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)

저는 MatcherInfo라는 이름에서 우선순위가 적용된 register 연산 이라는 정보를 유추하기가 어려운 것 같은데요, 이건 어떻게 알 수 있을까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실행흐름이 두 개로 분리되는데 오히려 유지보수가 어려워지는 게 아닐까요?

사용 흐름을 고려하지 않았네요,, 좋은 지적 감사합니다.

재사용이 될지도 모르는 코드를 추상화하는 시도는 위험한 것 같은데요, 최소한 3번 이상 쓰여야 추상화를 할지 결정하는 방향이 좋은 것 같습니다.

동의합니다. 재사용 여부가 불확실한 코드는 추상화하기에 위험할 것 같네요,,
건설적인 피드백 감사합니다.

  • 앞으로 리팩토링 할 때 꼭 참고하겠습니다!🙂

저는 MatcherInfo라는 이름에서 우선순위가 적용된 register 연산 이라는 정보를 유추하기가 어려운 것 같은데요, 이건 어떻게 알 수 있을까요??

해당 네이밍은 성아님과 함께 대화를 하며 수정하려고 했습니다!
코드 설계 만큼 네이밍 어렵네요,,🥲

인터페이스를 삭제하고 클래스를 참조하도록 변경하겠습니다.

  • Class name도 한 번 더 고민해보겠습니다..

덕분에 인사이트가 넓어진 것 같습니다. 감사합니다 :)

import com.navercorp.fixturemonkey.api.matcher.MatcherOperator;

public interface MatcherInfo {
Integer getPriority();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

왜 primitive 타입이 아니라 wrapper 타입을 반환하는 걸까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음에 null을 고려하고 설계를 해서 wrapper 클래스로 작성을 했습니다.
하지만, null을 허용하지 않기 때문에 primitive 타입으로 변경하겠습니다. 😅

아래의 코멘트에서 방향이 결정된 뒤 수정하겠습니다.

Signed-off-by: yongjunhong <[email protected]>
@YongGoose YongGoose force-pushed the feature/random-selection branch from c143264 to 6e64284 Compare December 6, 2024 16:09
@YongGoose YongGoose requested a review from seongahjo December 7, 2024 03:28
Comment on lines 63 to 64
private final Map<Integer, List<MatcherOperator<? extends ArbitraryBuilder<?>>>>
priorityGroupedMatchers = new HashMap<>();
Copy link
Contributor

@seongahjo seongahjo Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priorityGroupedMatchersregisteredArbitraryBuilders 중 하나만 남기는 방향이 좋을 것 같습니다.

priorityGroupedMatchers의 이름 안에 register를 포함해야할 것 같습니다.

import com.navercorp.fixturemonkey.FixtureMonkey;
import com.navercorp.fixturemonkey.api.matcher.MatcherOperator;

public final class PriorityMatcherOperator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MatcherOperator는 단순히 MatcherOperator를 가지고 Operator 적용 여부(match)는 외부에서 판단합니다.

적용 여부를 내부에서 판단하는 새로운 인터페이스를 설계하면 PriorityMatcherOperator 도 그 인터페이스의 구현체 중 하나로 내부 구조 노출없이 일관성있게 제공할 수 있지 않을까 싶습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

궁금한 점이 있어 코멘트 남깁니다!

현재 PriorityMatcherOperator의 경우 아래의 경우로 사용되는 것으로 알고 있습니다.

Collectors.mapping(
	it -> new MatcherOperator<>(
		it.getMatcherOperator(), it.getMatcherOperator().getOperator().apply(this)
	),

register 연산을 입력 받을 때 파라미터로 입력 받은 Function 함수형 인터페이스를 가지고 있다가 apply를 통해 ArbitrartyBuilder<?>을 생성하는 것으로 알고 있습니다.

그런 뒤, registeredArbitraryBuilders에 추가를 하고 giveMeBuilder에서 match를 통해 적용 여부를 판단하는 것으로 확인이 됩니다.

이렇게만 본다면 단순히 MatcherOperator우선순위와 함께 정보를 가지고 있다가 초기화를 해주는 것으로 보이는데요, 아래와 같이 PriorityMatcherOperator가 내부에서 match를 해야 하는 case가 있는지 궁금합니다.

적용 여부를 내부에서 판단하는 새로운 인터페이스를 설계하면 PriorityMatcherOperator 도 그 인터페이스의 구현체 중 하나로 내부 구조 노출없이 일관성있게 제공할 수 있지 않을까 싶습니다.

Copy link
Contributor

@seongahjo seongahjo Dec 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

질문을 잘 이해 못했습니다.
초기화는 match랑 상관이 없는 로직인데 어떤 연관성이 있다고 생각하시는 걸까요?

제가 드린 의견은 우선순위 정보를 외부에 노출할 필요가 없게 만드는 게 좋다는 의견이라고 보시면 될 것 같습니다.
우선순위 정보는 구현에 강하게 의존한 정보라고 보고 있습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아아 확인했습니다.

제가 성아님의 코멘트를 잘못 이해한 것 같네요,,😅
작업 후 리뷰 요청하겠습니다!

Copy link
Author

@YongGoose YongGoose Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 드린 의견은 우선순위 정보를 외부에 노출할 필요가 없게 만드는 게 좋다는 의견이라고 보시면 될 것 같습니다.

해당 방법으로 설계를 진행하고 있는데 어려움을 겪고 있습니다.
혹시 성아님께서 생각하신 구체적인 구현 방안이 있다면 간단히 설명해주실 수 있을까요?


제가 어려움을 겪는 부분을 자세히 설명드리자면, 아래의 코드에서 getPriority 메서드를 통해 우선순위 정보를 노출하지 않고 진행하는 것에 어려움을 겪고 있습니다.

현재는 우선순위 : List<연산>의 Key-Value 형태로 저장을 한 뒤, 각 Key에 해당하는 Value를 섞는 방법으로 진행을 하고있어, Key에 해당하는 우선순위 정보를 가져와야 하는 상황입니다.

private Map<Integer, List<MatcherOperator<? extends ArbitraryBuilder<?>>>> groupMatchersByPriority(
	List<PriorityMatcherOperator> registeredArbitraryBuildersWithPriority
) {
	return new HashMap<>(registeredArbitraryBuildersWithPriority
		.stream()
		.collect(Collectors.groupingBy(
			PriorityMatcherOperator::getPriority,
			Collectors.mapping(
				it -> new MatcherOperator<>(
					it.getMatcherOperator(), it.getMatcherOperator().getOperator().apply(this)
				),
				Collectors.toList()
			)
		))
	);
}

private void groupMatchersByPriorityFromNamedMap(
	Map<String, PriorityMatcherOperator> registeredPriorityMatchersByName,
	Map<Integer, List<MatcherOperator<? extends ArbitraryBuilder<?>>>> registeredGroupMatchersListByPriority
) {
	registeredGroupMatchersListByPriority.putAll(
		registeredPriorityMatchersByName.entrySet().stream()
			.collect(Collectors.groupingBy(
				entry -> entry.getValue().getPriority(),
				Collectors.mapping(
					entry -> new MatcherOperator<>(
						new NamedMatcher(entry.getValue().getMatcherOperator().getMatcher(), entry.getKey()),
						entry.getValue().getMatcherOperator().getOperator().apply(this)
					),
					Collectors.toList()
				)
			))
	);
}

Copy link
Author

@YongGoose YongGoose Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다음 단계를 진행하려고 하는데 미리 정해야 하는 것이 있어 코멘트 남깁니다!

다음 단계에서는 �match된 연산들을 shuffle하고, 우선순위에 맞게 적용을 하려고 합니다.

우선순위가 동일한 경우에도 해당 그룹 내에서 shuffle을 해야 하는데요, 아래의 코멘트와 같이 적용된 연산을 미리 섞은 뒤 match된 연산을 우선순위에 맞게 적용을 하면 그룹핑을 해서 shuffle을 하는 것을 방지할 수 있을 것 같습니다.

간단하게 표현하면:

// 1. 그룹핑을 한 뒤, 무작위로 섞음
match 된 연산을 우선순위에 맞게 그룹핑 -> 해당 그룹 내에서 shuffle

// 2. 무작위로 섞은 뒤, 그룹핑
register 연산 초기화 시점에서 shuffle -> match 된 연산을 우선순위에 맞게 그룹핑

만약 다른 아이디어를 생각하시고 계시면 말씀 부탁드리겠습니다!


그리고 기존 코드에 우선순위 기능을 추가하며, 변수 및 메서드 네이밍이 수정이 필요할 것 같습니다.
제가 개인적으로 수정을 하긴 했는데 한 번 보시고 부족한 부분 지적 부탁드립니다 :)

Copy link
Contributor

@seongahjo seongahjo Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 1번안처럼 match가 된 연산들이 기준이 되면 좋을 것 같다는 생각입니다.

다만, 그룹핑이 필요한 이유가 잘 이해가 안되는데요, 혹시 자세히 설명해주실 수 있을까요??
제가 생각했을 때는 우선순위를 기준으로 정렬하면 되지 않을까 생각했었는데, 이렇게 처리했을 때 문제가 있다고 판단하신 걸까요??

Copy link
Author

@YongGoose YongGoose Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

성아님의 의견과 같이 정렬을 통해 해결할 수도 있으나, 아래의 경우와 같이 최우선으로 선택되는 우선순위가 동일한 경우 그룹핑을 통해 해당 우선순위를 가진 연산들만 shuffle을 해야 할 것 같아 그룹핑을 해야 한다고 표현했습니다!

FixtureMonkey sut = FixtureMonkey.builder()
			.register(String.class, monkey -> monkey.giveMeBuilder("test1"), 2)
			.register(String.class, monkey -> monkey.giveMeBuilder("test2"), 1)
			.register(String.class, monkey -> monkey.giveMeBuilder("test3"), 1)
			.register(String.class, monkey -> monkey.giveMeBuilder("test4"), 1)
			.register(String.class, monkey -> monkey.giveMeBuilder("test5"), 1)
			.build();

추가적으로 정렬은 어디에서 하는게 적절하다고 생각하시나요??

Copy link
Contributor

@seongahjo seongahjo Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래의 경우와 같이 최우선으로 선택되는 우선순위가 동일한 경우 그룹핑을 통해 해당 우선순위를 가진 연산들만 shuffle을 해야 할 것 같아 그룹핑을 해야 한다고 표현했습니다

지금 구현을 보니 그룹핑하면 아래와 같은 두가지 문제가 있을 것 같습니다.

  1. 타입이 다른데 우선순위가 같은 경우 불필요한 필터링 필요
  2. 우선순위가 순차적이지 않을경우 순회 방식(1,2,4)

위 두 가지 경우를 어떻게 처리할 생각이신가요??

추가적으로 정렬은 어디에서 하는게 적절하다고 생각하시나요??

구현을 어떻게 하냐에 따라 다를 것 같은데요, Fixture Monkey 인스턴스가 초기화한 후에 해도 되지 않을까 싶네요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seongahjo

생각을 해보니 그룹핑을 하는 것은 불필요한 비용이 될 수 있을뿐더러 지적해주신 내용과 같이 두 가지의 경우에서 문제가 있을 것 같습니다. 지적 감사합니다 :D

2주 전에 작성해주신 코멘트와 같이 현재는 하나의 리스트로 연산을 관리하고 있습니다.

그로 인해 다음 단계로 넘어가려 하는데 혹시 생각하신 단계가 있다면 여쭤봐도 될까요?

저는 무작위로 섞는 연산이나, 초기화된 인스턴스를 정렬 하는 것을 생각 하고 있습니다.
그러나 아직 이러한 작업에 익숙치 않아 불필요한 비용이 발생할까 여쭤봅니다..!

@YongGoose YongGoose force-pushed the feature/random-selection branch from 5faa668 to 0a87f4e Compare December 13, 2024 05:28
Signed-off-by: yongjunhong <[email protected]>
@YongGoose YongGoose force-pushed the feature/random-selection branch from 0a87f4e to 6fa32d7 Compare December 13, 2024 05:30
@YongGoose YongGoose requested a review from seongahjo December 26, 2024 11:48
Comment on lines 876 to +886
@Property
void registerSetFirst() {
String expected = "test2";
void registerWithPriority() {
FixtureMonkey sut = FixtureMonkey.builder()
.register(String.class, monkey -> monkey.giveMeBuilder("test"))
.register(String.class, monkey -> monkey.giveMeBuilder("test"), 1)
.register(String.class, monkey -> monkey.giveMeBuilder("test2"), 2)
.build();

String actual = sut.giveMeBuilder(String.class)
.set("$", expected)
.sample();

then(actual).isEqualTo(expected);
then(actual).isEqualTo("test");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 케이스에서도 잘 동작하는지 확인하는 테스트가 있으면 좋을 것 같습니다!

FixtureMonkey sut = FixtureMonkey.builder()
			.register(String.class, monkey -> monkey.giveMeBuilder("test2"), 2)
			.register(String.class, monkey -> monkey.giveMeBuilder("test"), 1)
			.build();

Copy link
Author

@YongGoose YongGoose Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아직 우선순위, 무작위 적용 관련 부분이 구현되지 않아서 해당 코드를 구현한 뒤 테스트 하겠습니다 :)

@YongGoose YongGoose requested a review from seongahjo January 2, 2025 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants