Baconian - courtesy of Sir Thomas#35
Conversation
|
You're a legend for working on this, thanks for all your help. I might not get time to review it this week, but I will get around to it when I can. |
|
Okay, great. In the meantime, I'll have a look at the code too, I think can already spot some improvements. |
| //! Each character of the message plaintext is encoded as a 5-bit binary, | ||
| //! these are then "hidden" in a decoy message through the use of font variation. | ||
| //! | ||
| //! This cipher is very easy to crack, once the method of hiding is known, therefore this |
There was a problem hiding this comment.
I would suggest changing this to:
"
Each character of the plaintext message is encoded as a 5-bit binary character. These characters are then "hidden" in a decoy message through the use of font variation. This cipher is very easy to crack once the method of hiding is known. As such, this implementation includes the option to set whether the substitution is distinct for the whole alphabet, or whether it follows the classical method of treating 'I' and 'J', and 'U' and 'V' as interchangeable characters (as would have been the case in Bacon's time).
Additionally, this implementation allows the user to change the underlying binary character choice. This is traditionally 'a' and 'b', but the user can choose any pair of characters. If no concealing text is given and the boilerplate of "Lorem ipsum..." is used, a plaintext message of up to 50 characters may be hidden.
"
There was a problem hiding this comment.
Done. (with amendments...)
|
|
||
| /// This struct is created by the `new()` method. See its documentation for more. | ||
| pub struct Baconian { | ||
| distinct: bool, |
There was a problem hiding this comment.
Could you please explain to me the purpose of the distinct? This variable name does not seem intuitive to me...
There was a problem hiding this comment.
How about non_traditional_dictionary? I'd like to keep the default behaviour as traditional as that is how the original cipher was used.
There was a problem hiding this comment.
How about: use_distinct_alphabet?
There was a problem hiding this comment.
Okay will sort this. Rammed busy right now, but will get on to it this week.
| /// - as they had equivalent value in Bacon's day | ||
| /// - generally, 'I' and 'V' were more common and used here | ||
| lazy_static! { | ||
| static ref TRAD_CODES: HashMap<&'static str, &'static str> = hashmap!{ |
There was a problem hiding this comment.
For TRAD_CODES and DISTINCT_CODES hashmap, there seems to be a lot of overlap in the data stored. Surely there is a way we only need to define this information once?
There was a problem hiding this comment.
Agreed, I'll change it to that the outcome for the TRADITIONAL mapping is met, but using a DISTINCT codeset.
| fn new(key: (bool, Option<String>)) -> Result<Baconian, &'static str> { | ||
| Ok(Baconian { | ||
| distinct: key.0, | ||
| decoy_text: key.1.unwrap_or_else(|| String::from(DEFAULT_DECOY)), |
There was a problem hiding this comment.
Rather using a static default decoy text, perhaps we could randomly generate a string as the default decoy? I know this might diverge from other implementations but I think it would be a cool feature.
There was a problem hiding this comment.
I need to look into that. Easy thing in Python, not sure about Rust, maybe there is a crate that could be used. I agree, it would be cool, but I'll park this for now. Maybe make an issue for it..?
There was a problem hiding this comment.
It might be not that hard to integrate, see the crate: https://github.com/mgeisler/lipsum. However I understand if you want to leave this out for now, we can create an issue.
There was a problem hiding this comment.
Will have a look. Will flag whether it would be better as an issue. It would certainly be interesting, and allow the default always to manage the size of the plaintext
There was a problem hiding this comment.
Done.
Used the crate you advised. However, still a good idea to open an issue as the code is not very elegant as it just assigns an arbitrary default decoy text of 160chars long... So, could be better with a bit more time and thought...
| fn encrypt(&self, message: &str) -> Result<String, &'static str> { | ||
| let mut non_alphas = 0; // A counter for non_alphas | ||
|
|
||
| for c in self.decoy_text.chars() { |
There was a problem hiding this comment.
You could tricky with iterators here... something like:
self.decoy_text.chars().for_each(|c| {if !c.is_alphabetic() {non_alphas += 1}});
I don't know what the exact call would look like but it's worth exploring if you haven't done so already.
There was a problem hiding this comment.
Okay, I'll look into it. Personally, I'm not a great fan of (pythonic) idioms like this as it makes code readability worse imho. I get it for interpreted languages where concise code gives performance gains, like python and Javascript, but compiled languages, it just makes for hard to maintain code. </rant> 😄
From a learning perspective, though, it's good to use, so when I see it I'll be able to read it (maybe)...
There was a problem hiding this comment.
Lunchtime look: your snippet is sound and does the same as original. But, actually not gaining anything there - rustfmt will reformat over the same number of lines... and don't actually make any savings.
From a learner perspective, I can see what you are thinking. I'm sure I (we) can find a better example to learn that syntax.
There was a problem hiding this comment.
Sure. I don't feel to strongly either way and I haven't really used streams extensively in this project, so we can keep it as you have for now.
| for c in decoy_slice.chars() { | ||
| if c.is_alphabetic() { | ||
| match secret.remove(0) { | ||
| 'B' => { |
There was a problem hiding this comment.
Some comments would be nice to explain what is happening here
| #[macro_use] | ||
| extern crate lazy_static; | ||
| #[macro_use] | ||
| extern crate maplit; |
There was a problem hiding this comment.
Are you still using this crate? If so, where?
There was a problem hiding this comment.
Yes, I use both these in the initialisation of the static Hashmap constants for the code look ups.
arosspope
left a comment
There was a problem hiding this comment.
Hey @avastmick, I've added some questions and suggestions throughout that I would like to discuss before merging the PR. Overall this is a pretty cool cipher.
|
Thanks @arosspope, I'll look at this and comment / make changes after Easter. Some good thinking. |
|
Had to change |
|
That's me for the week, I'm afraid. Anything else, shout out and I'll look at them early next week. In particular, the autogen of text - as you said fun, but I only had a couple of hours today... PS: @arosspope Not sure what editor you prefer, but integrating in |
| // for traditional usage. | ||
| let mut key_upper = key.to_uppercase(); | ||
| if !distinct { | ||
| match key_upper.as_str() { |
There was a problem hiding this comment.
Perfect, this is what I had in mind 👏
|
@avastmick Almost ready to merge, just two points that I would like to discuss before that happens. Let me know what you think. In terms of text editor, I usually use Atom. I'm pretty sure it can integrate rustfmt... I should check that out, thanks. 😄 |
|
I covered you two changes and raised an issue to improve the usage of the I may drop off radar for a bit as I have a heap of work changes coming up. Cheers, Mick |
This was a fun one!
Under the hood, it uses a simple substitution cipher, mapping alphabetical characters to 5-bit binary equivalents - so 'a' maps to
AAAAA, 'b' toAAAABand so on.There are two code mappings, traditional that maps 'I' and 'J' to the same value, and 'U' and 'V' to the same. This was due to the fact in Elizabethan England, these were variants of the same character and not distinct, as they are today. I also added a distinct mapping that has a full alphabet mapped to discrete values.
Next, the fun part: hiding the produced substitution ciphertext in a decoy text. I do this by mapping alphabetical characters to italics if the binary is given. I use the mathematical italic
utf-8characters. There maybe other fonts or characters that could be used, if so the code would need to be modularised.Please have a look at the code and check for newbie
Rustissues and feedback for me to fix.