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

Remove wrong abstraction in IAccount and use impl instead #439

Merged
merged 1 commit into from
Aug 21, 2017

Conversation

ChristophWurst
Copy link
Member

This removes a minor flaw in our OOP design. Those two methods are only implemented for a specific account, hence they shouldn't be part of the interface. In fact, the mail transmission logic even assumed to get an instance of Account and the impl in UnifiedAccount would throw an exception anyway.

Found while digging into the drafts code for #9.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member Author

FYI the Account class is by far the most complex class we have in our code base. It's tightly coupled to several other classes and does not leverage the advantages of IoC/DI and hence can only be tested partly (just look at the constructor and see how dependencies are located statically). Still it is one of the most used classes spread over the full code base. As a long time goal we should try to break it into functional blocks (something like the mail transmission service that I created).

@ChristophWurst
Copy link
Member Author

Minor change, CI is happy and no feedback within a week -> merging.

@ChristophWurst ChristophWurst merged commit 663b77b into master Aug 21, 2017
@ChristophWurst ChristophWurst deleted the refactor/remove-wrong-abstraction branch August 21, 2017 06:16
@lock
Copy link

lock bot commented Nov 20, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and questions.

@lock lock bot locked and limited conversation to collaborators Nov 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant