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

new snippet - swap numbers #172

Merged

Conversation

Emosans
Copy link
Contributor

@Emosans Emosans commented Jan 4, 2025

Description

Type of Change

  • ✨ New snippet
  • πŸ›  Improvement to an existing snippet
  • 🐞 Bug fix
  • πŸ“– Documentation update
  • πŸ”§ Other (please describe):

Checklist

  • I have tested my code and verified it works as expected.
  • My code follows the style and contribution guidelines of this project.
  • Comments are added where necessary for clarity.
  • Documentation has been updated (if applicable).
  • There are no new warnings or errors from my changes.

Related Issues

Closes #

Additional Context

Screenshots (Optional)

Click to view screenshots

@Emosans Emosans requested a review from saminjay as a code owner January 4, 2025 07:25
@Mathys-Gasnier
Copy link
Collaborator

Hey, Thank you for your contribution.

After reviewing your PR we've found that all/some snippets you are adding don't match the scope of Quicksnip.
The ability to convert a string to an int in c++ is already provided by std::to_string.

Please remove the PR that don't match the scope (Listed bellow), if your PR contains no snippet matching the scope of Quicksnip, it will be closed without further notice.

If no fix is provided within 7-10 days this PR will be closed without further notice

@Emosans
Copy link
Contributor Author

Emosans commented Jan 4, 2025

It is a snippet that converts a string to integer. Built in function for string to int would be std::to_integer() or stoi() with complexity O(N), my snippet is basically the same. Should I update it to use the built in function?

@Mathys-Gasnier
Copy link
Collaborator

Oh sorry, the title is a bit miss leading... if an std function already exists to do that a snippet isnt necessary

@Emosans
Copy link
Contributor Author

Emosans commented Jan 4, 2025

i have another snippet idea can i make the changes in this pr itself or should i open a new one... the new snippet would be swap 2 nums without third variable.. any suggestions abt this?

@Mathys-Gasnier
Copy link
Collaborator

Add it here, we will see if it fits as a snippet in quicksnip

@Mathys-Gasnier Mathys-Gasnier reopened this Jan 4, 2025
@Emosans
Copy link
Contributor Author

Emosans commented Jan 4, 2025

done...

Copy link
Collaborator

@majvax majvax left a comment

Choose a reason for hiding this comment

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

Hey, Thank you for your contribution! πŸ™Œ However, we can't currently accept your snippet:

The arithmetic swapping logic is redundant since the function is already returning a tuple. You can achieve the same result more efficiently by directly swapping the values.

I believe it could fit in the C categories, where there is no std::pair or std::tuple. You could take the parameters as pointer and achieve the same result.

Let me know if you have any questions or need further clarification.

snippets/cpp/math-and-numbers/swap-numbers.md Outdated Show resolved Hide resolved
@majvax
Copy link
Collaborator

majvax commented Jan 4, 2025

Could you please update the name of your PR ? It would make it more easier to track.

@Emosans Emosans changed the title new snippet - string to int new snippet - swap numbers Jan 4, 2025
@Emosans
Copy link
Contributor Author

Emosans commented Jan 4, 2025

updated the colon as well simple swapping using pointers in C

@Emosans Emosans requested a review from majvax January 4, 2025 11:06
Copy link
Collaborator

@majvax majvax left a comment

Choose a reason for hiding this comment

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

For me it's good, could you however format your code, it seems really compact. Here's an example:

*num1 = *num1 + *num2;
int a = 3, b = 4;
swap(&a, &b);


// Usage:
int a=3,b=4;
auto swapped=swap(&a,&b); // simply use printf after this to print swapped values
Copy link
Collaborator

Choose a reason for hiding this comment

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

auto is a C++ keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it

@majvax majvax added the Snippets label Jan 4, 2025
@Emosans Emosans requested a review from majvax January 4, 2025 11:15
Copy link
Collaborator

@majvax majvax left a comment

Choose a reason for hiding this comment

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

It's seems fine for me, however I want a 2nd opinion, could you please wait for another maintainers to review your snippet

@Emosans
Copy link
Contributor Author

Emosans commented Jan 4, 2025

works for me thanykou!


// Usage:
int a = 3,b = 4;
swap(&a,&b); // simply use printf after this to print swapped values
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't quite like the comment here, i would prefer something along the lines of Swaps the values of the a and b variables

Appart from that it looks good

@Emosans
Copy link
Contributor Author

Emosans commented Jan 4, 2025

done

@majvax majvax added the mergeable Reviewed and ready to be merged label Jan 4, 2025
@Mathys-Gasnier Mathys-Gasnier merged commit 621db73 into dostonnabotov:main Jan 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeable Reviewed and ready to be merged Snippets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants