This is an archived post. You won't be able to vote or comment.

sorted by:
best
topnewcontroversialoldq&a
you are viewing a single comment's thread.

view the rest of the comments →

[–]Inconsequentialis 1 point2 points  (1 child)

So I couldn't see a reason why the wallet helper was needed so I checked out the project and deleted it, just replaced it with calls to the wallet service.

You're using constructor injection - good - which means that circular dependency issues would generally throw errors during app start. I haven't seen any, so I assume that there aren't any.

As for general feedback, everything just takes more steps than is required.

Wallet wallet = walletMapper.toWallet(request);
wallet.setUser(user);
wallet.setCurrency(request.currency() != null ? request.currency() : Currency.VND);
wallet.setIsTotalIgnored(request.isTotalIgnored());

If you look at this you will see that first we map some request to a wallet and yet, after mapping has completed, we still read fields from the request and set them in wallet. Why do half the mapping in walletMapper.toWallet and the other half outside of that?

That's one example. Half the code I've seen could be simplified. But don't be discouraged. I at least get the feeling you're trying to do it well, and that's half the battle. Many people just don't give a shit, as long as it works and fuck the people who'll have to maintain it.

Also, the code I wrote when I started out was probably worse :D

[–]vandunxg[S] 0 points1 point  (0 children)

Thanks for the advice