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 maps concept #2593

Closed
wants to merge 18 commits into from
Closed

Add maps concept #2593

wants to merge 18 commits into from

Conversation

smcg468
Copy link
Contributor

@smcg468 smcg468 commented Dec 4, 2023

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

        "lists",
        "tuples",
        "anonymous-functions",
        "default-arguments"
    

to

       "numbers"
       "strings"
  • Could also add an howManyPlayers task to utilise the .size() method if it would help, or maybe best to keep it consistent with the elixir track.

Let me know of any changes you'd like me to make.

Reviewer Resources:

Track Policies

@sanderploegsma
Copy link
Contributor

Awesome! I'll try to review this sometime this week 👍

Copy link
Contributor

@sanderploegsma sanderploegsma left a 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."
Copy link
Contributor

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.

@@ -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'
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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();
Copy link
Contributor

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!

Comment on lines 5 to 16
- 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`.
Copy link
Contributor

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.

Comment on lines 20 to 26
- `AbstractMap`
- `IdentityHashMap`
- `WeakHashMap`
- `EnumMap`
- `ConcurrentHashMap`
- `ConcurrentSkipListMap`
- `SortedMap`
Copy link
Contributor

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.
Copy link
Contributor

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());
Copy link
Contributor

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() {
Copy link
Contributor

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.

@sanderploegsma
Copy link
Contributor

@smcg468 Are you still working on this? Leave a comment if you are, otherwise I'm going to close this due to inactivity.

@smcg468
Copy link
Contributor Author

smcg468 commented Dec 27, 2023

@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

@sanderploegsma
Copy link
Contributor

@smcg468 Are you still working on this? Leave a comment if you are, otherwise I'm going to close this due to inactivity.

@smcg468
Copy link
Contributor Author

smcg468 commented Feb 21, 2024

@sanderploegsma Sorry yes will get back to it this weekend

@sanderploegsma
Copy link
Contributor

Take your time! I didn't mean to rush you, but I figured you may have lost track of this PR 😉

@manumafe98
Copy link
Contributor

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 😃

@sanderploegsma
Copy link
Contributor

@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. 😉

@manumafe98
Copy link
Contributor

hey @smcg468 do you need some help with this pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants