-
-
Notifications
You must be signed in to change notification settings - Fork 710
binary-search: use generics #868
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
Conversation
The binary-search tests force the practitioner to generify the class but only one type (integer) was actually being used in the tests. This should be changed so that more than one type is actually used, to make it more clear that generics are necessary and to demonstrate how generics is used. As discussed in exercism#230, move linked-list to one of the last difficulty 6 exercises, so that binary-seach is the first exercise that introduces generics. Add a hint to the binary-search exercise to explain generics as this is now the first exercise to require it. Fixes exercism#230.
Is this up for grabs? If so, I can do it :-) |
What exactly do you mean by "up for grabs"? I've made this PR to solve the issue of using generics in |
Ah, sorry, I thought I was looking at an issue :-) |
I see, no problem :) |
|
||
For example you could construct a list of `Integers`: | ||
|
||
`List<Integer> someList = new LinkedList();` |
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.
Should use LinkedList<>();
here.
|
||
Now `someList` can only contain `Integers`. You could also do: | ||
|
||
`List<String> someOtherList = new LinkedList()` |
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.
Should use LinkedList<>();
here.
|
||
Now `someOtherList` can only contain `Strings`. | ||
|
||
Another constraint is that any type used with generics cannot be a [primitive type](https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html), such as `int` or `long`. |
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.
Might be good to add a reminder that every primitive type has a corresponding reference type!
@@ -13,23 +13,23 @@ | |||
|
|||
@Test | |||
public void findsAValueInAnArrayWithOneElement() { | |||
List<Integer> listOfUnitLength = Collections.singletonList(6); | |||
List<Character> listOfUnitLength = Collections.singletonList('6'); |
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.
Same question as for custom-set
; is there a pattern for the generic types being used now?
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.
Not really. I've tried to use Integer
, Character
and String
and to make sure I've got a couple of tests using each, but I've not got a strict pattern as I didn't think it mattered too much. I could make it a more strict pattern if you think that would be useful?
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.
Nah, I think it's good as-is!
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.
Will change it to cycle through Character
, String
, Integer
as it's probably best to be consistent :)
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.
As I was changing it I remembered why I didn't do that. Some of them can't use Character
because they use higher numbers, e.g. 11
... So it's only possible to use Character
for some of the earlier tests, at least for this exercise. I could make the rest cycle through String
and Integer
?
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.
As I was changing it I remembered why I didn't do that. Some of them can't use
Character
because they use higher numbers, e.g.11
... So it's only possible to useCharacter
for some of the earlier tests, at least for this exercise. I could make the rest cycle throughString
andInteger
?
This sounds like a good compromise to me!
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.
Okay, will do that :) Will do that for the others too, try to cycle through all three but skip Character
if not possible :)
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.
Sadly, as it turns out String
causes problems as well since it will sort it lexicographically and we're using numbers in all our test cases... So should I instead try to cycle through all three, skip Chracter
if not possible and also skip String
if not possible? And make sure that Character
and String
are used at least once?
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.
That sounds ideal, yep!
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.
Great, will do that then :)
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.
Comments inline!
Also removed an unused method that I found in the reference implementation :) |
❤️ |
The binary-search tests force the practitioner to generify the class
but only one type (integer) was actually being used in the tests.
This should be changed so that more than one type is actually used, to
make it more clear that generics are necessary and to demonstrate how
generics is used.
As discussed in #230, move linked-list to one of the last difficulty 6
exercises, so that binary-seach is the first exercise that introduces
generics.
Add a hint to the binary-search exercise to explain generics as this
is now the first exercise to require it.
Should hopefully fix the
binary-search
part of #230.Reviewer Resources:
Track Policies