Java: Remove final keyword from all test class declarations

[stkent]

Discussion and consensus: https://github.com/exercism/xjava/issues/373

Marking our test classes as final is (1) more syntax for a learner to parse, and (2) not increasing the clarity of the writer's intent given the context of an individual problem's codebase. We should remove it from all test class declarations.

[morrme]

Totally abusing this issue to ask: I understand that there is a goal to have a significant number of good first patch issues. However, I'm always looking to pitch in and there are very few issues not reserved for first timers. Are these tracks generally supposed to be seen as a "first stop" towards OSS contribution, and not really made for "repeat" contributors ?

[stkent]

No, you are absolutely welcome and actively encouraged to tackle any other issues you feel comfortable with! The good first patch backlog is maintained to allow newcomers to ease into open source contribution, but once you've got some of those under your belt, moving on to more complex tasks is a natural step. There are always new exercises to build (just make sure you check that no-one has called dibs yet, and that the exercise is not marked deprecated or foregone in the config.yaml file at the root of the repo) as well as the open issues. If you're unsure about jumping in to an issue, just comment requesting help, and a maintainer will be more than happy to help you figure out anything you need to get started. ❤️

[stkent]

@morrme also, just as a heads up, here are the guidelines we try to stick to with respect to good first patches: https://github.com/exercism/xjava/blob/master/POLICIES.md#good-first-patches. As you'll see, they aren't always off-limits to experienced contributors - right now we're running a surplus, I believe!

[FridaTveit]

What about new starter implementations? I think some of those classes are also marked as final. Should we remove the final keyword from those as well?

[FridaTveit]

The test classes which are marked as final:

• [x] WordProblemSolverTest
• [x] BracketCheckerTest
• [x] BaseConverterTest
• [x] FlattenerTest
• [x] OpticalCharacterReaderTest
• [x] HandshakeCalculatorTest
• [x] RobotTest
• [x] RectangleCounterTest
• [x] QueenAttackCalculatorTest
• [x] NaturalNumberTest
• [x] MinesweeperBoardTest
• [x] LargestSeriesProductCalculatorTest
• [x] DifferenceOfSquaresTest
• [x] DiamondPrinterTest
• [x] ChangeCalculatorTest

[morrme]

Since I've polluted this issue anyway 😄 I am happy to help. @FridaTveit are you going to tackle some as well?

[stkent]

What about new starter implementations? I think some of those classes are also marked as final. Should we remove the final keyword from those as well?

I'm torn. If we take the argument made for the test cases to its logical conclusion, then we should

1. remove final from all starter (and example) implementations;
2. remove public from all starter (and example) implementations.

If final is unnecessary complexity, then so too is public, I would think. 🤔

[morrme]

I have started these but I'd like a review to make sure they're ok... @stkent @FridaTveit

[stkent]

@morrme I think the actual test suites need the public keyword still. I see CI failures with the following messages on your PRs:

BaseConverterTest > initializationError FAILED
    java.lang.Exception: The class BaseConverterTest is not public.
BaseConverterTest > initializationError FAILED
    java.lang.Exception: Test class should have exactly one public constructor

Removing the public keyword was something I was musing on in the context of starter and example implementations (i.e. the code under the main and example directories); I think for test code (under the test directory) we only want to/are able to remove the final keyword :)

[morrme]

stkent done.

[FridaTveit]

@stkent I don't think we necessarily have to remove public. I think people are more used to seeing it and know what it means. But I wouldn't object if we decided to remove it :)

And go ahead and do as many as you want @morrme, I didn't have any specific plans :)

[morrme]

@FridaTveit i don't mind doing them all. is there any way to modify the checkboxes above when i've submitted a PR ? i have done the first 3 so far--wanted to make sure i was doing it right.

[FridaTveit]

@morrme You can edit my comment (by clicking on the pencil icon) and put an "x" between the square brackets that make up the checkbox :) Does that make sense? I've checked the first three now :)

[stkent]

@stkent I don't think we necessarily have to remove public. I think people are more used to seeing it and know what it means. But I wouldn't object if we decided to remove it :)

Yeah, if we're targeting folks who are new to Java, which was the audience in #373, public is not necessarily any more familiar than final, though it's meaning is probably a lot easy to guess at. But it's also unnecessary in all of these implementations because of the package structure, so it seems like it would be more correct to omit anyway. Is there some artificial problem structure we could come up with to 'introduce' the concept of public? Perhaps exercises that provide helper files (enums, etc.) could place them in a support package within the main package, thereby forcing the use of the public modifier?

[morrme]

@FridaTveit It does but I don't have the ability/permission level that would allow me to edit your comment. Thanks for doing the first few for me! I'll be sending over a few more today.

[FridaTveit]

Oh, okay. I'll try to keep the list up to date! :)

[FridaTveit]

Is there some artificial problem structure we could come up with to 'introduce' the concept of public? Perhaps exercises that provide helper files (enums, etc.) could place them in a support package within the main package, thereby forcing the use of the public modifier?

@stkent, I think this sounds like a great idea! :)

[morrme]

Any thoughts on this line : https://github.com/exercism/xjava/blob/master/exercises/robot-name/src/test/java/RobotTest.java#L12

It uses final to define the string.

[FridaTveit]

I vote for definitely keeping final in that context. That uses final to define a constant which I would argue is the easiest use of final to understand. It's also useful even in the limited context of these exercises, as the code is more readable when you can easily see which values are constant.

[stkent]

@morrme thanks for the latest round of removals! 😍