Skip to content

Comments

Assignments at declaration#77

Merged
codecop merged 5 commits intoemilybache:pick1from
pfichtner:pick1
Feb 22, 2026
Merged

Assignments at declaration#77
codecop merged 5 commits intoemilybache:pick1from
pfichtner:pick1

Conversation

@pfichtner
Copy link

There is no need to create the instances inside the setUp method.

In our coaching sessions, we often observe that it is not widely known that JUnit creates member variables anew for each test. As a result, there are often @BeforeEach-annotated methods that are not necessary. This is another argument for the refactoring, as initialization can be done directly at the declaration.

Another minor change (second commit cb06ed6) is the remove of pulblic modifiers which are not necessary with JUnit5 or newer.

On a related note, I personally prefer leaving field members without access modifiers in JUnit 5 tests (i.e., no private). However, this is just my personal style, and I understand that some IDEs or tools might flag this and suggest making the members private. For this reason, I decided not to propose this change.

What do you think about this change(s)? I wanted to discuss it first because I would have to make an additional PR for each further branch. Alternatively, a maintainer could do cherry-picking to avoid those PRs.

@pfichtner
Copy link
Author

Draft since I'd like to discuss first

@codecop
Copy link
Collaborator

codecop commented Feb 19, 2026

Looks good. Is more typical like that. Does not change the exercise at all.

@pfichtner pfichtner marked this pull request as ready for review February 19, 2026 18:26
@pfichtner
Copy link
Author

@codecop what's the best option to spread this change over all branches? Shall I open a PR for each affected branch or can you cherry-pick the commit on them?

Changed instance variables to final for immutability.
@pfichtner
Copy link
Author

Just saw that the fields have final modifiers in the sammancoaching fork so I did here as well for standardization with commit 469dfb3

@codecop
Copy link
Collaborator

codecop commented Feb 21, 2026

There are many branches here that I do not know. main and with_tests are usually the current and important ones. Maybe you know more about them. Let's please start with these two

@pfichtner pfichtner mentioned this pull request Feb 21, 2026
@pfichtner
Copy link
Author

Added two commits:
9f4ea43 since in branch with_tests enum constants are named "correctly" (UPPER_SNAKE_CASE) I had to change the tests there so reflect ProductUnit.Each vs ProductUnit.EACH so i fixed it here as well (see #78)
e286424 with_tests uses Java 21, since there is no need for Java 22 here we should use 21 as well

@pfichtner
Copy link
Author

There are many branches here that I do not know. main and with_tests are usually the current and important ones. Maybe you know more about them. Let's please start with these two

On main there are no member variables (this is the todo in the kata). The public modifiers on main have already been removed with 8bc02ac 20 months ago.

@codecop
Copy link
Collaborator

codecop commented Feb 21, 2026

Sometimes I compare both branches in a tool that shows me all diffs.
Maybe there are commits to cherry-pick from one or the other branch. But hard to find usually.
So I just try to make both branches similar by manually changing things on one or the other or both like you do right now.

@codecop codecop merged commit 83005c2 into emilybache:pick1 Feb 22, 2026
@pfichtner
Copy link
Author

@codecop what about all the other branches? I don't have permissions to push to this repo so if I do the cherry-picking I'd have to open a PR for each branch. Same would apply to #79. I can do so but that looks like work that could be avoided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants