Dissolve BSFormatter

[bodymindarts]

BSFormatter was a statefull catch all type class with a huge interface.
It had dependencies to globals and very many classes depended on it.

It was hard to reason about code flow due to how tangled up things were,
making refactoring attempts of higher level classes difficult.

To Dissolve BSFormatter the following changes were made:

• Move functions that only have 1 call-site to private functions on the caller.
• Make functions that do not depend on local state static.
• Move functions that are only depended on by the desktop project to DisplayUtils.
• Move functions that are Parsing related to ParsingUtils.
• Move remaining static functions to FormattingUtils.
• Create interface CoinFormatter and class ImmutableCoinFormatter to handle statefull functions.
• Remove dependency to globals by instantiating an instance in CoreModule and inject it via Guice.

These changes are motivated by the following design principles.

• Minimize dependencies on all levels. ie. small public interfaces for classes and libraries.
• Prefer static functions for stateless helpers that don't contain domain logic
• Prefer composition over inheritance for code reuse
• Prefer interfaces over inheritance for polymorphism
• Don't depend on global state in regular classes, better to inject it via constructor

I assume these are not contentious and am omitting justification. Happy to discuss these points if there is dissent.

This refactoring is not an end state but an intermediate step in a larger refactoring I am attempting. Just want the PRs to stay focused.

[christophsturm]

this pr has too many import and whitespace changes.

[freimair]

please remove the non-functional changes, we do not have the resources to filter that for you.

EDIT: substitute "non-functional" with "non-changes". Whitepaces, new lines, linebreaks, ...

[battleofwizards]

Thanks a lot for attempting this code refactoring!

I cannot say for actual reviewers (ripcurlx, sqrrm, freimar, ManfredKarrer) but here are my 5 cents. Context: I am a new contributor myself so please don't put to much weight into my comments.

1. The "Optimize Imports" commit is huge and serious enough for being a PR on its own. Whether this will be welcome by other developers I don't know. Please seek advice on #dev channel. Personally I would very much welcome this (but first we need to make sure import optimization feature is stable across IntelliJ installations).

2. The remaining changes should be probably split into smaller PR-s that are easy for reviewers and do feel safe. Yes, that may be a pain from the productivity point of view. Being "reviewer friendly" is a priority though.

Hope this helps!

[battleofwizards]

please remove the non-functional changes, we do not have the resources to filter that for you.

My understanding is that this PR is intended as pure refactoring so by definition there are no changes in behavior (at least not intentional). I think the key is is to make the same refactoring much more gradually so it becomes viable for you to review.

[christophsturm]

This PR is intended as pure refactoring so by definition there are no changes in behavior (at least not intentional). I think the key is is to make the same refactoring much more gradually so it becomes viable for you to review.

i think florian is refering to the formatting and whitespace changes.

[freimair]

@battleofwizards

(but first we need to make sure import optimization feature is stable across IntelliJ installations).

First, you assume that everyone is using IntelliJ, that IntelliJ does not change its formatting default (which it did actually), and that IntelliJ is around as long as Bisq is around.

Second, "stable across installations". that will never happen. Just take a look at any bigger project, even one that is not open-source, you will never find this level of "stability".

so please keep formatting-only changes to a minimum, and let us try to focus on real work.

[bodymindarts]

Thank you all for your comments. I have removed the OptimizeImports commit. I thought intellij was what the community had converged on since the .idea dir is commited.

Regarding making smaller PRs - I could do that but it would be quite some work not sure yet if I will get to it.

[sqrrm]

@battleofwizards on the topic of reviewers. I think the task of reviewing is up to everyone contributing to the project. I see the maintainer role, ie, merge rights, as just a 'click the merge button when the PR has enough ACKs' kind of job. I will try to review PRs as I can and will merge when I see PRs with ACKs from reputable contributors and no NACKs.

On your other points I agree, in particular making PRs easy to review is in the PR creator's own interest. If it's too hard I will likely prioritize other easier to review PRs.

@bodymindarts I think most developers use intellij so to me it's not unreasonable to optimize imports and do other formatting by intellij.

[bodymindarts]

I'm closing this PR and will open smaller ones so that the individual steps become more evident.