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

Refactor large files? #15

Open
mobi-nex opened this issue Dec 25, 2022 · 5 comments
Open

Refactor large files? #15

mobi-nex opened this issue Dec 25, 2022 · 5 comments

Comments

@mobi-nex
Copy link
Owner

mobi-nex commented Dec 25, 2022

As highlighted by @fjborquez in #6 there are a lot of large files in the project currently that can be more easily manageable/readable/navigable if they were smaller. Also, unit testing would be easier. Maybe a refactor of some of these files into smaller files should be considered. This issue is created to track discussions related to above referenced refactor.

@fjborquez
Copy link

fjborquez commented Dec 25, 2022

I must confess that I have no experience with Rust but other languages. I'd like to learn while contributing with your project in this. That said, I think a better approach to the project structure is:

  • src/
    • main.rs
    • market/
      • every decoupled part from original market.rs file.
      • test files for every decoupled part.
    • cli/
    • net/

Util, lib and noise, imho, should be renamed to more related names.

@mobi-nex
Copy link
Owner Author

Before addressing your suggested structure, I will explain the project structure and Rust packaging conventions in a nutshell.

The AdbOrc project currently has two separate entities -- a binary (the actual adborc executable) and a library (if other projects want to use AdbOrc from their programs. Currently, one GUI app is a user of the library). In the current file structure, only main.rs and cli.rs are part of the binary, all other files/directories are part of the library. So roughly the project currently is organized as:

binary
-------
    main::
         cli

library
-------
    lib::
         net
         noise
         util::
             adb_utils
             scrcpy_utils
         market::
             request
             marketmaker
             consumer
             supplier

The Rust package manager, Cargo, refers to each of these two entities as crates - a binary crate and a library crate. By convention (and by default), the main entrypoint for a binary crate is main.rs and library crate is lib.rs. The cargo module conventions are such that if module foo has a submodule bar, then the code for bar should be in a file called bar.rs in the same directory as foo.rs, OR it should be in bar/mod.rs relative to foor.rs (older convention but still supported). Any submodules of bar, say baz, should be in bar/baz.rs of bar/baz/mod.rs. You can read more about cargo packaging here

So, if we were to follow Cargo conventions and defaults (which I think we should) the entrypoints (main.rs and lib.rs) should have the same name. I like the idea of having directories for each submodules and the code of each submodule residing inside their respective directories. The problem with that is, if we were to have market.rs code inside src/market then we would have to rename it to mod.rs and same for each of the other submodules. It leads to a lot of mod.rs files in the project. I am not sure what the ideal solution for this should be.

We can (and probably should) rename the noise module. It was named noise because it contains the code for a simple implementation of the noise protocol framework, which is the security protocol used in AdbOrc. Since it is a private module, changing the name for it should be pretty easy. As for changing the name of util module, it is a public module and will lead to making the required changes at every place it is used. Also, there is a GUI tool that currently uses the library and it will lead to name changes there too. As such, I would rather not change the name of a public module unless there is a concrete reason for it.

Apart from the large files, the thing I am not a huge fan of about the current structure -- there is no clear separation between what is part of binary and what is part of library.

@fjborquez
Copy link

I appreciate your explanations :D.
I agree with you that follow the conventions is the best approach.
I'm going to make a later, read the conventions and try some ideas about it.

Maybe the lib and cli should be different packages/repos. lib could be a dependency for cli, gui tool and any other project.

@fjborquez
Copy link

fjborquez commented Jan 8, 2023

Hey @mobi-nex, I'm working on refactor of CLI module but I didn't have much time these days.
When I finish it I will push the changes to my fork and comment here for reviews.

@fjborquez
Copy link

@mobi-nex this is the branch where i'm working on: https://github.com/fjborquez/adborc/tree/refactor
Suggestions are welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants