Adding analyzer feedback to design.md for bird watcher concept exercise

[manumafe98]

pull request

Related issue: #2670

Reviewer Resources:

Track Policies

[manumafe98]

I have a couple of doubts on the reference resolution:

java/exercises/concept/bird-watcher/.meta/src/reference/java/BirdWatcher.java

Line 10 in 02d488e

return birdsPerDay.clone();

The clone in the method getLastWeek is necessary? Because it's already making the copy in the constructor.

java/exercises/concept/bird-watcher/.meta/src/reference/java/BirdWatcher.java

Line 18 in 02d488e

birdsPerDay[birdsPerDay.length - 1] = birdsPerDay[birdsPerDay.length - 1] + 1;

Also I was thinking that this could be simplified to

    public void incrementTodaysCount() {
        birdsPerDay[birdsPerDay.length - 1]++;
    }

But this one is more peaky.

[sanderploegsma]

I have a couple of doubts on the reference resolution:

java/exercises/concept/bird-watcher/.meta/src/reference/java/BirdWatcher.java

Line 10 in 02d488e

return birdsPerDay.clone();

The clone in the method getLastWeek is necessary? Because it's already making the copy in the constructor.

I think the author added the .clone() here because the array is a reference value, and by returning a reference to the field, the caller could make changes to it:

birdWatcher.getLastWeek()[0] = 0;

Adding the .clone() makes the method return a copy of the array to prevent this type of mis-use.

java/exercises/concept/bird-watcher/.meta/src/reference/java/BirdWatcher.java

Line 18 in 02d488e

birdsPerDay[birdsPerDay.length - 1] = birdsPerDay[birdsPerDay.length - 1] + 1;

Also I was thinking that this could be simplified to

    public void incrementTodaysCount() {
        birdsPerDay[birdsPerDay.length - 1]++;
    }

But this one is more peaky.

Yeah I think you're right. The exercise's .meta/config.json indicates that this exercise was forked from the C# track, and they seem to be doing this too: https://github.com/exercism/csharp/blob/main/exercises/concept/bird-watcher/.meta/Exemplar.cs.

[sanderploegsma]

Reading the exercise instructions again, I think that the implementation of getLastWeek() in the exemplar solution is actually wrong!

For comparison purposes, you always keep a copy of last week's counts nearby, which were: 0, 2, 5, 3, 7, 8 and 4. Implement the BirdWatcher.getLastWeek() method that returns last week's counts:

BirdWatcher.getLastWeek();
// => [0, 2, 5, 3, 7, 8, 4]

From this I conclude that the solution must always return those values. The array passed to the constructor contains the values for this week, not last week. So the exemplar solution should implement getLastWeek() similar to the C# track: https://github.com/exercism/csharp/blob/63dd1aa3fe0bd2b59033158f243d096bd556b9f4/exercises/concept/bird-watcher/.meta/Exemplar.cs#L14

[sanderploegsma]

🤔 interesting that the tests don't fail after this change. Makes me wonder whether the test suite is complete... But, I guess that is out of scope for this PR. Might be worth looking into in a separate issue.

[phoenix-1729]

Why should the test cases fail?(Can you please elaborate a little bit)
The array which getLastWeek() is returning is the same array which is being passed while creating the object of Birdwatcher.

`
private static final int DAY1 = 0;
private static final int DAY2 = 2;
private static final int DAY3 = 5;
private static final int DAY4 = 3;
private static final int DAY5 = 7;
private static final int DAY6 = 8;
private static final int TODAY = 4;
private BirdWatcher birdWatcher;

private int lastWeek[] = {DAY1, DAY2, DAY3, DAY4, DAY5, DAY6, TODAY};
@BeforeEach
public void setUp() {
    birdWatcher = new BirdWatcher(lastWeek);
}
@Test
@Tag("task:1")
@DisplayName("The getLastWeek method correctly returns last week's counts")
public void itTestGetLastWeek() {
    assertThat(birdWatcher.getLastWeek())
        .containsExactly(DAY1, DAY2, DAY3, DAY4, DAY5, DAY6, TODAY);
}

`

[manumafe98]

Hey @phoenix-1729 I recommend to ask the questions in the issue itself

[sanderploegsma]