mark-wiemer / calculator-game Goto Github PK

Description
Currently, clients build a state (value, movesLeft, rules) and pass that to CalculatorGame.generateGame, which may return null, but otherwise returns a CalculatorGame instance

Problems
Using an intermediate method instead of a constructor is surprising, making the code more difficult to follow for newcomers to the project. Returning null can, as always, result in surprises

Proposed Solution
N/A

Acceptance Criteria

• Game construction logic is clear and unsurprising

Is your feature request related to a problem? Please describe.
When I provide command-line arguments, I only want to solve/play one level. Do not prompt me to enter an infinite loop, as that makes it more difficult to automate usage of the project.

Describe the solution you'd like
Don't prompt for infinite loop, just return 0 if game was solved, nonzero otherwise with informative error message

Describe alternatives you've considered
Currently I have to wrap all scripts using this with "no" inputs and provide that as well, wrapped in a scanner. Lots of maintenance code when a simple fix is possible

Additional context
N/A

Acceptance criteria

• Don't prompt me to enter an infinite loop if I provide command-line arguments
• Give me a return code of 0 if nothing went wrong, nonzero otherwise

Description
Rule subclass constructors are usually public

Problems
Other packages shouldn't be able to create rules except through the Rule.makeRule method.

Proposed Solution
Make Rule subclass constructors package-level visible.

Acceptance Criteria

• External clients cannot instantiate any Rule subclass directly through its constructor

Description
Run the tests on PR to absolutely verify that everything is working before merge

Acceptance Criteria

• All unit tests are run on PR
• PRs cannot be merged until unit tests pass

Description
Lots of functions convert lists from one type to another manually without a consistent interface.

Problems
Cluttered, clunky code is hard to maintain

Proposed Solution
Java 8 Streams! Read all about them here and use maps and stuff!
All arrays (Rule[] rules, int[] portals, etc.) should be converted to lists (preferably immutable, but that's for another issue) for easier stream usage.

Acceptance Criteria

• No more clunky list conversion code, if possible
• Code should remain readable: we aren't using streams for the sake of streams. If it's

A control flow diagram for the main loops would be a nice overview of how the project works

Description
The Meta Store rule is currently hidden from users.

Problems
Program functionality is ambiguous to users. They also cannot use Meta Store rules in the future "play" logic

Proposed Solution
The Meta Store rule should not be necessarily tied to a Store rule. If it is added to a game without a Store rule, an exception should be thrown. If it is explicitly added to a game by a user, a message should be printed to tell users that it is not necessary to add the rule.

Acceptance Criteria

• Meta Store rule should be renamed Update Store rule
• Meta Store rule independent of Store rule
• Exception Generate null when adding a Meta Store rule to a game without a Store rule
• Users should be able to create Meta Store rules

Description
Documenting the history of this project would be good practice to better understand how it has historically functioned

Acceptance Criteria

• At least a short bulleted list for each release (1+ bullet points), all within one file

For the Main class, any old input is allowed and it's super gross. The user should never see exceptions.

Description
Right now, Config.java acts as one monolith of nearly all config settings

Problems
Monoliths are bad and violate SRP. It's becoming difficult to read/maintain and it's unclear exactly what code belongs there

Proposed Solution
Split config into several files

Acceptance Criteria

• The purpose of Config is clear
• New files are created to host Config logic, and their purposes are also clear

Description
Creating instances of CalculatorGame currently results in null often or throwing exceptions. Maybe Optional would fix this issue?

Acceptance Criteria

• Understand the basics of Optional in Java
• Understand the use cases of Optional
• Decide whether using Optional is beneficial in this project

Description
The pad rule logic is duplicated in PadRule.java and StoreRule.java.

Problems
This isn't DRY

Proposed Solution
Have StoreRule call a package visible method in PadRule

Acceptance Criteria

• DRY code

Description
Currently, all Rule instances have a getOperand1 and getOperand2 method. For many instances, these methods are unnecessary.

Problems
Violates the interface segregation principle, leading to potential issues when clients use those methods on Rule instances that do not define them

Proposed Solution
Subclass Rule with 'zero operators' type, 'one operator' type, and 'two operator' type.

Acceptance Criteria

• Implement ISP in Rule and its subclasses

Description
Static methods are used to build and tear down the testing environment

Problems
Static methods only have one return value and are stateless. This means the client must track things instead, violating SRP and cluttering client code

Proposed Solution
Make IoUtils methods instance-based. This way, the IoUtils instance can store the default System.out and can build up / tear down testing environments with one method call

Acceptance Criteria

• IoUtils methods are instance-based
• One-line usage within all clients

Description
The naming is murky: transitioning from "game" to "level" hasn't been consistent, and what exactly a "test case" is compared to a "level" isn't immediately clear.

Problems
Ambiguity is bad!

Proposed Solution

• References (in method names, variable names, comments, and documentation) to an instance of what is currently CalculatorGame should call it a level.
• The solver.Game interface should probably be renamed solver.Level.
• The CalculatorGame class should be renamed, but remain concise, maybe CalculatorLevel?

Acceptance Criteria

• A clear naming scheme is chosen and implemented across the entire repository

Description
Currently Rule instances are created using Rule.makeRule and Rule.ruleFromString

Problems
This is not the default way to instantiate a type, and surprises the developer, making the code harder to follow.

Proposed Solution
Use constructors

Acceptance Criteria

• No more Rule.makeRule
• No more Rule.ruleFromString

Right now, the user can only solve a given game. Playing the game could be a lot of fun.

Is your feature request related to a problem? Please describe.
As a user,
I want to be able to create my own rules.
That would be really fun for me.

Describe the solution you'd like
Let me input a rule somehow to the program and then create levels using that rule. Ideally, the rule is persistent across runs.

Describe alternatives you've considered
Right now, as a user, I can't create my own rules at all.

Additional context
N/A

Acceptance criteria

• I can create my own rules within the Solver control flow (currently main.Main)
• Bonus: The rules stay persistent across runs

Description
Currently, I/O logic (printing prompts, getting input, enforcing typed input) is scattered about the three classes in the main package

Problems
Logic is probably not DRY. It's also not the most readable

Proposed Solution
Create a new main.IO class to handle all IO

Acceptance Criteria

• No more print statements within any main class (This was a bad criterion as simple print statements are allowed)
• No more scanners within any main class (Using scanners is good design)
• All complex I/O logic is moved to a single module

Description
The Meta Store rule should be only internally represented and instantiable. Although not mission critical, it would be nice to ensure that this is the case. If external users can create Meta Store rules, they may run into bugs and other undefined behaviors.

Acceptance Criteria

• Ensure MetaStoreRule cannot be instantiated by external clients

Description
Some of this code is super gross due to weird formatting behaviors

Acceptance Criteria

• The formatter doesn't make the code so gross

Tests are currently not very granular and, in general, poorly organized.

Acceptance Criteria:

• MainTests.assertCreatesGame becomes two methods: one for the scanner, one for CLA
• Stay DRY with fixture-like functionality
• Remove printing from passing tests (#41)
• IoUtils interface becomes instance-based, not static, is improved (see #17)
• Remove redundant tests (LevelTests is redundant, already tested within PlayTests)
• Tests are at top of class

Description
Currently, to quit Developer.main, the user enters Config.QUIT == "n "

Problems
That's a gross entry. It ends with a space.

Proposed Solution
Trim the comparison so that n is also valid input

Acceptance Criteria

• Better Developer quit entry logic