-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix: ternary logic #75
Conversation
@tolluset is attempting to deploy a commit to the Toss Team on Vercel. A member of the Team first needs to authorize it. |
code/examples/ternary-operator.md
Outdated
const status = A조건 && B조건 ? "BOTH" : 둘다아닌경우 ? "NONE" : A조건 ? "A" : undefined; | ||
const status = A조건 && B조건 ? "BOTH" : A조건 ? "A" : "NONE"; |
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.
Complex conditions can be hard to understand at a glance when expressed using a ternary operator.
How about modifying the condition so that we can get A, B, BOTH, and NONE all in one?
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.
I agree with your perspectives.
However, the code change above is for a bad example, so I think that changes would make the documentation harder to understand.
The improved code exists in the documentation below.
const status = (() => {
if (A조건 && B조건) {
return "BOTH";
}
if (A조건) {
return "A조건";
}
return "NONE";
})();
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.
The main reason for the changes was the inconsistency between the above sentence and the code.
다음 코드는 `A조건`과 `B조건`에 따라서 `'BOTH'`, `'A'`, 또는 `'NONE'` 중 하나를 `status`에 지정하는 코드예요.
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.
The main reason for the changes was the inconsistency between the above sentence and the code.
"PR request intent is clearly understood. :)"
However, the code change above is for a bad example, so I think that changes would make the documentation harder to understand.
when the condition edits(B Condition Return Added), could you explain why it would make the documentation harder to understand? 👀
I believe that if the B condition return is included, the complexity of the ternary operator would increase, and the code would seem like it needs further improvement.
If you find it difficult to agree with the points I mentioned, or if you'd prefer to only modify the condition expression to match the original, please let me know."
"다음 코드는 A조건과 B조건에 따라 'BOTH', 'A', 'B', 또는 'NONE'을 status로 반환하는 코드입니다."
const status = (A && B) ? 'BOTH' : (A || B) ? (A ? 'A' : 'B') : 'NONE';
const status = (() => {
if (A조건 && B조건) return "BOTH";
if (A조건) return "A조건";
if (B조건) return "B조건";
return "NONE";
})();
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.
when the condition edits(B Condition Return Added), could you explain why it would make the documentation harder to understand? 👀
I think that improvement is valid.
My concern was about making the improved code different from the existing code.
The code you provided looks good. 👍
I believe that if the B condition return is included, the complexity of the ternary operator would increase, and the code would seem like it needs further improvement.
I hadn't considered the part about adding a B condition return, and I think your code would improve that.
If you find it difficult to agree with the points I mentioned, or if you'd prefer to only modify the condition expression to match the original, please let me know."
I think the method you provided would help users understand.
As far as understanding the ternary operator goes, I think either way is fine.
I was only thinking of making changes to align the code with the documentation, but if you thinks that additional improvements would help users understand better, I agree to make those changes.
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.
By the way, ternary inside of ternary(I didn't know this was possible 🤓) is hard to recognize but it looks COOL. I like it.
const status = (A && B) ? 'BOTH' : (A || B) ? (A ? 'A' : 'B') : 'NONE';
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.
if you thinks that additional improvements would help users understand better, I agree to make those changes.
It would be great if the things we discussed together could be included in the examples.
Could you possibly create a commit that includes the above �code and content? 🙏
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.
I’ve applied the changes. Thanks for your suggestions!
04bdc46
(#75)
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.
Thank you for accepting the feedback!
Can you resolve merge conflict please? 🙏
@Kimbangg |
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.
LGTM 💯
Fix ternary examples' default value. Synchronzie with improved code.
Before default value:
undefined
After default value:
"NONE"