Add Integers into parser and solvers #36
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- added class CryptaConstant extending CryptaLeaf - test long multiplication - update all classes so that integers are treated as constants and in evaluation case are not added in the map of symbols - we can do long multiplications, there is a test for that taken from issue #22 ``` SEE * SO = ----- EMOO + MESS = ------ MIMEO ``` - test on long division - other thest have been taken from http://cryptarithms.awardspace.us/puzzles.html TODO : test for BigNum and add more tests for basic cases expecially negative tests
- Added method in CryptaConstant to convert decimal integer to integer in a given base in LittleEndian - BigNumModeler corrected method to add right integer to model in the wanted base (from config.arithBase) - Added tests on bignum with integers - Added tests with base change (both for bignum and normal solver)
arnaud-m
requested changes
Nov 10, 2022
There was a problem hiding this comment.
- We must next check the formatting issue.
- Tests are good.
We must solve the issue related to constants:
- The word is enclosed by ticks.
- The number is not.
A simple solution is to define two methods :
getWordwithout ticksformatWordwith ticks.
There may be more elegant solution
src/main/java/cryptator/solver/AbstractModelerNodeConsumer.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public void accept(ICryptaNode node, int numNode) { | ||
| if (node.isLeaf() && !node.isConstant()) { |
| } | ||
|
|
||
| private static void writeWord(ICryptaNode node, PrintWriter out) { | ||
| if (node.isConstant()) out.write("'"); |
There was a problem hiding this comment.
Should it be the node that adds the ticks ?
| try { | ||
| BigInteger v = BigInteger.ZERO; | ||
| final BigInteger b = BigInteger.valueOf(base); | ||
| if (node instanceof CryptaConstant constant) |
There was a problem hiding this comment.
You have chosen to add an attribute to the class.
Do not use both the attribute and instanceof.
I know that it allows to access the getConstant method, but we must find another way.
arnaud-m
approved these changes
Nov 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Breif summary:
Now we are able to parse and analyse long multiplications (see here) and long divisions (see here)