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

[core] Added OrderBook sorted constructors #2378

Merged
merged 1 commit into from
Mar 13, 2018
Merged

[core] Added OrderBook sorted constructors #2378

merged 1 commit into from
Mar 13, 2018

Conversation

cymp
Copy link
Contributor

@cymp cymp commented Mar 6, 2018

we can now specify if we want to sort bids and asks at orderbook creation

@cymp
Copy link
Contributor Author

cymp commented Mar 11, 2018

@timmolter Feel free to close this if you don't like it as I have other pull requests to make and I don't want to batch them into this one.

@timmolter
Copy link
Member

@rafalkrupinski why do you feel it won't work?
@cymp I'll address this ASAP. You could always create separate feature branches for parallel PRs.

@npomfret
Copy link
Contributor

Instead of this constructor I think a better approach would be to add a static factory method, something like createWithSortedOrders(...)

@cymp
Copy link
Contributor Author

cymp commented Mar 12, 2018

@npomfret fair point, but I don't like too much mixing public constructors and static factory methods, and it is obviously out of question to privatize the OrderBook constructor.

@npomfret
Copy link
Contributor

Ok, but factory methods are a good alternative to having multiple (and easily confusable) constructors. You can't name constructors so it's often not clear that they might be doing something other than constructing (assigning parameters to fields). Personally, I would never make a constructor do anything other than check the validity of the parameters and then assign them to fields - I'd always use a factory method if those parameters needed some pre-processing before they can be assigned. Plus the stack trace you get if something goes wrong is a tiny bit clearer in a factory method.

@cymp
Copy link
Contributor Author

cymp commented Mar 12, 2018

Yeah sure you don't need to convince me here, I would have prefered having factory methods too, but only them.

I just think designing a class with public constructor + factory methods a bit odd, I'm not used to have both since it mixes two designs. I mean if anyone starts looking at how to instanciate an OrderBook, he will type new OrderBook(... in his IDE and since there is a public constructor, he won't instinctively look at the factory methods and fully understand the class.

Thanks for giving your opinion.

@rafalkrupinski
Copy link
Contributor

@timmolter I thought XChange is java7 compatible. My Bad, removing the comment

@timmolter
Copy link
Member

I think the best way for XChange to construct objects is the builder pattern, if I had to pick one out of the many ways possible. We have implemented builders for quite a lot of the generic DTOs, but some haven't yet been converted.

@timmolter timmolter merged commit 9012767 into knowm:develop Mar 13, 2018
@rafalkrupinski
Copy link
Contributor

@timmolter why just one design pattern for the whole project?

@timmolter
Copy link
Member

personal preference, I guess.

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

Successfully merging this pull request may close these issues.

None yet

4 participants