-
-
Notifications
You must be signed in to change notification settings - Fork 680
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 maps concept #2593
Add maps concept #2593
Conversation
Awesome! I'll try to review this sometime this week 👍 |
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.
Thanks for picking this up! It already looks very good, and I left a bunch of comments mainly related to making the exercise a little bit more object-oriented, which is more in line with the Java track's policies.
Could you have a look at them and request a new review when you have addressed them?
"forked_from": [ | ||
"elixir/high-score" | ||
], | ||
"blurb": "Maps are a data structure that holds key-value pairs. Keys and values can be of any reference data type, and the keys must be unique." |
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.
Could you come up with a blurb about the exercise instead? Here's the one that the Elixir track uses:
Learn about maps by keeping track of the high scores in your local arcade hall.
exercises/settings.gradle
Outdated
@@ -20,6 +20,7 @@ include 'concept:salary-calculator' | |||
include 'concept:squeaky-clean' | |||
include 'concept:tim-from-marketing' | |||
include 'concept:wizards-and-warriors' | |||
include 'concept:arcade-high-score' |
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.
The list of exercises is now sorted alphabetically to limit the number of merge conflicts occurring when multiple people are working on new concept exercises at the same time.
Could you move this entry to the appropriate place?
concepts/maps/introduction.md
Outdated
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.
In my opinion this introduction into Maps should focus more on the basic operations when working with maps. Specifically, I think it should show:
- How to create a new Map instance.
- How to set values using the
put
method. - How to retrieve values using the
get
method.
To me, the keySet
and values
methods are OK to mention in the about.md
, but should not be introduced until a student knows how to interact with the map contents directly.
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.
Personally I'd like to change this exercise to be a little bit more object-oriented. I have the following reference implementation in mind, what do you think?
class ArcadeHighScore {
private Map<String, Integer> highScores = new HashMap<>();
Map<String, Integer> getHighScores() {
return highScores;
}
void addPlayer(String name, Integer score) {
highScores.put(name, score);
}
void removePlayer(String name) {
highScores.remove(name);
}
void resetScore(String name) {
highScores.put(name, 0);
}
void updateScore(String name, Integer score) {
Integer oldScore = highScores.getOrDefault(name, 0);
highScores.put(name, oldScore + score);
}
Set<String> getPlayers() {
return highScores.keySet();
}
}
The first task would then cover defining the private highScores
field and returning it in the getHighScores()
method. The other tasks should speak for themselves.
|
||
public class ArcadeHighScoreTest { | ||
|
||
private final ArcadeHighScore arcadeHighScore = new ArcadeHighScore(); |
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.
You probably want to create a new instance for every test using @BeforeEach
to make sure the SUT doesn't retain any state from a previous test, this makes the tests easier to reason about, also for students!
- Know what a map is. | ||
- Know how to define a map literal. | ||
- Know how to add key/value pairs in a map. | ||
- Know how to get values from a map. | ||
- Know how to get keys from a map. | ||
- Know how to update values in a map. | ||
- Know how to remove items from a map. | ||
- Know how to retrieve all items in a map. | ||
- Know what a `HashMap` is. | ||
- Know what a `LinkedHashMap` is. | ||
- Know what a `TreeMap` is. | ||
- Know the main differences in `HashMap`, `LinkedHashMap` and `TreeMap`. |
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.
Note on these objectives: they should cover the learning objectives from the exercise, not the concept. I believe this exercise doesn't cover the different implementations of the Map
interface, nor how to retrieve all items in a map.
- `AbstractMap` | ||
- `IdentityHashMap` | ||
- `WeakHashMap` | ||
- `EnumMap` | ||
- `ConcurrentHashMap` | ||
- `ConcurrentSkipListMap` | ||
- `SortedMap` |
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.
We can probably cover all future additions of Map
implementations in Java by summarizing this as "Other implementations of the Map
interface"
|
||
## Prerequisites | ||
|
||
- `strings`: know how to use string formatting. |
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.
Technically I don't think string formatting is used in this exercise, so maybe strings
doesn't have to be a prerequisite.
@Tag("task:1") | ||
@DisplayName("Define a map to store the high scores") | ||
public void defineMap() { | ||
assertNotNull(arcadeHighScore.defineMap()); |
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.
We should probably (also) check that it's empty.
@Test | ||
@Tag("task:5") | ||
@DisplayName("Update a players score") | ||
public void updateScore() { |
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.
I think we need to add a second test for this task that verifies the requirement when updateScore
is called with a player that doesn't exist in the map yet.
@smcg468 Are you still working on this? Leave a comment if you are, otherwise I'm going to close this due to inactivity. |
Yes still working on this, been quite busy over Christmas but should get back to it soon |
@smcg468 Are you still working on this? Leave a comment if you are, otherwise I'm going to close this due to inactivity. |
@sanderploegsma Sorry yes will get back to it this weekend |
Take your time! I didn't mean to rush you, but I figured you may have lost track of this PR 😉 |
Hey @smcg468! we will like to know if you need help finishing this PR! it's an interesting concept to be added to the java track, so if you are busy with work we can give you a hand with it! Thanks for your time 😃 |
@smcg468 I see you've been busy, nice! Since some of the CI jobs are still failing I'm going to assume that you're still working on addressing the feedback. Just make sure to hit the 're-request review' button on this PR once you're done, so I know it's ready for another look. 😉 |
hey @smcg468 do you need some help with this pr? |
pull request
This issue addresses #2583
Renamed the exercise from 'high-score' to 'arcade-high score' to avoid clashing of exercise names
Added 'maps' to the practises and prerequisites for the 7 exercises listed in the issue
Changed the prerequisites for the arcade-high-score concept exercise from what was on the elixir track which was
to
Let me know of any changes you'd like me to make.
Reviewer Resources:
Track Policies