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

fix: ternary logic #75

Merged
merged 1 commit into from
Jan 16, 2025
Merged

fix: ternary logic #75

merged 1 commit into from
Jan 16, 2025

Conversation

tolluset
Copy link
Contributor

@tolluset tolluset commented Jan 15, 2025

Fix ternary examples' default value. Synchronzie with improved code.

Before default value: undefined
After default value: "NONE"

Copy link

vercel bot commented Jan 15, 2025

@tolluset is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

@tolluset tolluset changed the title fix: tenary logic fix: ternary logic Jan 15, 2025
const status = A조건 && B조건 ? "BOTH" : 둘다아닌경우 ? "NONE" : A조건 ? "A" : undefined;
const status = A조건 && B조건 ? "BOTH" : A조건 ? "A" : "NONE";
Copy link
Member

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?

Copy link
Contributor Author

@tolluset tolluset Jan 15, 2025

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";
})();

Copy link
Contributor Author

@tolluset tolluset Jan 15, 2025

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`에 지정하는 코드예요.

Copy link
Member

@Kimbangg Kimbangg Jan 15, 2025

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";
})();

Copy link
Contributor Author

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.

Copy link
Contributor Author

@tolluset tolluset Jan 15, 2025

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';

Copy link
Member

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? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kimbangg

I’ve applied the changes. Thanks for your suggestions!
04bdc46 (#75)

@tolluset tolluset requested a review from Kimbangg January 15, 2025 10:45
Copy link
Member

@Kimbangg Kimbangg left a comment

Choose a reason for hiding this comment

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

@tolluset

Thank you for accepting the feedback!
Can you resolve merge conflict please? 🙏

@tolluset
Copy link
Contributor Author

@Kimbangg
Conflict resolved. Thank you!

@tolluset tolluset requested a review from Kimbangg January 15, 2025 12:55
Copy link
Member

@Kimbangg Kimbangg left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@Kimbangg Kimbangg merged commit d40f3bb into toss:main Jan 16, 2025
1 check failed
@Seol-JY Seol-JY mentioned this pull request Jan 17, 2025
19 tasks
kaehehehe added a commit to kaehehehe/frontend-fundamentals that referenced this pull request Jan 17, 2025
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