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

Add exists function to HazeDatabase.java #88

Merged
merged 13 commits into from
Feb 15, 2023
Merged

Add exists function to HazeDatabase.java #88

merged 13 commits into from
Feb 15, 2023

Conversation

RehasLiamsi
Copy link
Contributor

I have created an exists function that takes in a number of String parameters as keys, compares them to the existing Map to see if the key exists and send back an integer value equating to the number of keys available in the database that match the parameters sent in.

@RehasLiamsi RehasLiamsi linked an issue Feb 12, 2023 that may be closed by this pull request
VenuModi
VenuModi previously approved these changes Feb 12, 2023
Copy link
Contributor

@VenuModi VenuModi left a comment

Choose a reason for hiding this comment

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

The use of varargs parameter in exists() was nice. This parameter avoids writing boilerplate code which can handle an arbitrary number of parameters automatically, using an array under the hood.

Copy link
Contributor

@safstromo safstromo left a comment

Choose a reason for hiding this comment

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

Good job, almost there.

I think you need to return a string from your method like this:
":" + numberOfKeys + "\r\n"
because the outputstream will send a string to the client and the client is expecting the format ":1\r\n"

Some tests would also be nice to check if your code works as intended.
Add that this pull request or create a new issue for that?

@RehasLiamsi
Copy link
Contributor Author

Thank you for the feedback @safstromo. I'll make the changes and add the tests to this pull request. I would appreciate if you could take a look at my code afterwards as well.

safstromo
safstromo previously approved these changes Feb 14, 2023
safstromo
safstromo previously approved these changes Feb 14, 2023
Copy link
Contributor

@Ahlberg-iths Ahlberg-iths left a comment

Choose a reason for hiding this comment

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

Nice, the code looks good and it clearly does what it is supposed to do. The unit tests are also adequate and they are all passing, good job!

I also like the idea of being efficient and using varargs as a parameter, but I'm a bit unsure about how we are going to be able to take advantage of the flexibility though. As this is like a server application where we are dependent on arbitrary user input during runtime, how do we call the function with the correct number of arguments?

There is something called a spread operator in JavaScript (looks just like the varargs actually: ...) that you can use to spread out values from an array or object into for example a method call. Is there something similar we could use in Java, or is there any other solution? Otherwise, maybe it would be better to go with a list/array containing all the keys to check for.

VenuModi
VenuModi previously approved these changes Feb 15, 2023
Copy link
Contributor

@VenuModi VenuModi left a comment

Choose a reason for hiding this comment

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

Looks good after the changes Oliver suggested

Copy link
Contributor

@jLereback jLereback left a comment

Choose a reason for hiding this comment

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

I think it would be better to use List as input parameter, otherwise there will be issues when you call the method from executeCommand due to compatibility with the other methods 🤗

src/main/java/org/fungover/haze/HazeDatabase.java Outdated Show resolved Hide resolved
src/main/java/org/fungover/haze/HazeDatabase.java Outdated Show resolved Hide resolved
src/test/java/org/fungover/haze/HazeDatabaseTest.java Outdated Show resolved Hide resolved
src/test/java/org/fungover/haze/HazeDatabaseTest.java Outdated Show resolved Hide resolved
src/test/java/org/fungover/haze/HazeDatabaseTest.java Outdated Show resolved Hide resolved
src/test/java/org/fungover/haze/HazeDatabaseTest.java Outdated Show resolved Hide resolved
jLereback
jLereback previously approved these changes Feb 15, 2023
Copy link
Contributor

@jLereback jLereback left a comment

Choose a reason for hiding this comment

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

Great work! I imported java.util.List and java.util.Collections

safstromo
safstromo previously approved these changes Feb 15, 2023
RehasLiamsi and others added 8 commits February 15, 2023 14:18
…ing keys in database and asking for one key twice, and the other is asked for once.
Co-authored-by: Julia Lerebäck Corell <112405931+jLereback@users.noreply.github.com>
Co-authored-by: Julia Lerebäck Corell <112405931+jLereback@users.noreply.github.com>
Co-authored-by: Julia Lerebäck Corell <112405931+jLereback@users.noreply.github.com>
Co-authored-by: Julia Lerebäck Corell <112405931+jLereback@users.noreply.github.com>
Co-authored-by: Julia Lerebäck Corell <112405931+jLereback@users.noreply.github.com>
Co-authored-by: Julia Lerebäck Corell <112405931+jLereback@users.noreply.github.com>
@sonarcloud
Copy link

sonarcloud bot commented Feb 15, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@Ahlberg-iths Ahlberg-iths left a comment

Choose a reason for hiding this comment

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

Nice!

@kappsegla kappsegla added this pull request to the merge queue Feb 15, 2023
Merged via the queue into main with commit e023401 Feb 15, 2023
@jLereback jLereback deleted the 10-exists branch February 15, 2023 13:55
@kappsegla kappsegla added the enhancement New feature or request label Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EXISTS
7 participants