Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TECHNICAL DEBT] Decoders use getter methods instead of something clever, like dereferencing the box #165

Open
bee-san opened this issue Dec 25, 2022 · 0 comments
Labels
Technical Debt A conscious decision we have made to reach our goals faster which has resulted in technical debt

Comments

@bee-san
Copy link
Owner

bee-san commented Dec 25, 2022

What?

In this commit:
fd4963e

The Crack trait has added 2 new methods:

  1. get_tags to get all the tags of a decoder
  2. get_name to get the name of the decoder

This is added to every decoder and will be very annoying to update or use in the future.

Why?

It would be nicer if we could simply loop over the boxes:

rust
    Decoders {
        components: vec![
            Box::new(reversedecoder),
            Box::new(base58_bitcoin),
            Box::new(base58_monero),
            Box::new(base58_ripple),
            Box::new(base58_flickr),
            Box::new(base64),
            Box::new(base91),
            Box::new(base64_url),
            Box::new(base65536),
            Box::new(binary),
            Box::new(hexadecimal),
            Box::new(base32),
            Box::new(morsecodedecoder),
            Box::new(atbashdecoder),
            Box::new(caesardecoder),
            Box::new(citrix_ctx1),
        ],
    }

Like:

for i in decoders{
i.name
}

But since they are in a box we do not know which fields they have. We can only guarantee it has a Crack trait (which lets us run the crack on the decoder).

The compiler does not know what's in the box as the Decoders is a trait object with Vec of Box<dyn Crack + Sync > and not our struct itself. It only knows that the things in the box have a Crack trait and a Sync trait but they could be completely different structs.

We did this because they are different implementations of the same struct (think like inheritance), which was a silly idea because Rust doesn't really support this 🙈

That commit is technical debt we have adopted to move faster in the moment and ship a product.

How will this affect us?

Anytime we want to add a new field to say, get popularity, we need to add it to all of the decoders. As they grow, this becomes ridiculous.

What can we do to fix this in the future?

There's somethings we can try...

Remove the boxes

We remove all the boxes and let the compiler yell at us and fix each error as they come.

I do not like this as much because the priority queue requires the ability to have structs with .Crack methods 🙈

Derive Macros

We can look into using Derive Macros that let us add additional scope onto existing structs:
https://doc.rust-lang.org/reference/procedural-macros.html#derive-macros

This seems promising, but requires actually studying how macros work 😢

@bee-san bee-san added the Technical Debt A conscious decision we have made to reach our goals faster which has resulted in technical debt label Dec 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Technical Debt A conscious decision we have made to reach our goals faster which has resulted in technical debt
Projects
None yet
Development

No branches or pull requests

1 participant