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

Money Pattern Implementation - Issue #1305 #1673

Closed
wants to merge 4 commits into from
Closed

Money Pattern Implementation - Issue #1305 #1673

wants to merge 4 commits into from

Conversation

ImLynxie
Copy link
Contributor

Money Pattern Implementation

  • I've implemented the Money pattern, and provided a demonstration of how it can be used.
  • This implementation is to showcase how to work with multiple types of currencies.

Pull request description

  • There are certain things that I would have done differently within the demonstration aspect of the Money Pattern Implementation, i.e Error Handling, adding more checks to get rid of certain bugs, etc. but at that point I'd probably end up writing a whole banking system. 😂

@ImLynxie ImLynxie mentioned this pull request Mar 11, 2021
@ImLynxie
Copy link
Contributor Author

SonarCloud found 2 bugs, which I'll push a fix for.

@ohbus ohbus self-assigned this Mar 12, 2021
Copy link
Contributor

@ohbus ohbus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with the bugs reported you also need to mitigate any code smell that it has and more importantly increase code coverage by providing test cases.

@ohbus ohbus linked an issue Mar 12, 2021 that may be closed by this pull request
@sonarqubecloud
Copy link

<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the maven-assembly-plugin as in the other patterns, so that the example can be easily and independently executed on the command line.

* @param money Contains the information required to create the balance.
*/
public Balance createBalance(Money money) {
Balance createdBalance = new Balance(this, money);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're able to use the var keyword here instead of explicitly defining the type. Check this elsewhere in the code too.

try {
balance.getMoney().subtractMoneyBy(moneyToWithdraw);
} catch (SubtractionCannotOccurException e) {
System.err.println("Insufficient funds in balance.");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a logger for output. See Lombok @Slf4j annotation.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check this throughout the example.

Comment on lines +38 to +48
public Money getMoney() {
return money;
}

public Account getAccount() {
return account;
}

public boolean isPrimaryBalance() {
return isPrimaryBalance;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To replace boilerplate such as these getters you're able to use Lombok annotations.

boolean writableStackTrace) {
super(message, cause, enableSuppression, writableStackTrace);
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you need all these constructors. Maybe define only the ones you use?

@ImLynxie
Copy link
Contributor Author

I'll get these issues fixed as soon as possible, I just haven't had any extra time on my hands.

@ohbus
Copy link
Contributor

ohbus commented Mar 25, 2021

I'll get these issues fixed as soon as possible, I just haven't had any extra time on my hands.

No worries. Take your time and just let us know when you'll be able to complete it.

@iluwatar
Copy link
Owner

The pull request has remained inactive and is about to be closed. Please comment if you're still working on it.

@iluwatar iluwatar added this to the 1.26.0 milestone Jan 6, 2022
@iluwatar
Copy link
Owner

iluwatar commented Jan 6, 2022

Closed due to inactivity

@iluwatar iluwatar closed this Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Money pattern
3 participants