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

feat: add property when group add as uuid #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abdurrahmanekr
Copy link

I didn't notice the #135 that include same context. But if @petrisorcibian accept this pr I'm okay with that.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3 New Code Smells (required ≤ 0)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@daniel-frak
Copy link
Owner

daniel-frak commented Mar 25, 2024

Hello, thanks for contributing!

Some feedback:

  • Please provide tests for your functionality
  • It's not clear at a glance what is happening in the code and why. The code seems to use try-catch only to check if the groupName is a UUID - the try-catch should be extracted to a single isUuid method, which will make it both more readable and safer (the code won't accidentally catch an exception from another line of code)
  • Consider using a static regex Pattern instead of catching exceptions - I think it would be more performant
  • Any new functionality should be documented in README.md
  • From a client's perspective, it's unclear why we should use getGroupById if the groupName is UUID. How about making it a configurable option (whether to treat groups from the API as names or IDs) or just add groupIds to the API?

@daniel-frak daniel-frak added the requested changes Waiting for requested changes to be made to the code label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requested changes Waiting for requested changes to be made to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants