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

Add Floor and Ceiling method to Treemap #82

Merged
merged 1 commit into from
Sep 22, 2018
Merged

Add Floor and Ceiling method to Treemap #82

merged 1 commit into from
Sep 22, 2018

Conversation

loxp
Copy link
Contributor

@loxp loxp commented Aug 15, 2018

No description provided.

@emirpasic
Copy link
Owner

@loxp Thank you for the input.

May i propose another solution of just exposing the underlying Tree of the TreeMap?

In other words, you could get the same result with

m := treemap.NewWithIntComparator() 
... //  add elements 
m.Tree.Floor(key)

...or do you prefer your solution?

@emirpasic
Copy link
Owner

perhaps, given I expose the Tree in the TreeMap and implement the seek iterator withing red-black tree, I could kill two flies with this approach w.r.t. #66

@loxp
Copy link
Contributor Author

loxp commented Sep 21, 2018

..or do you prefer your solution?

You are right. Moreover, do you think the solution below is better?

type Map struct {
    rbt.Tree
}

All the methods of Tree struct can be invoked directly through a reference to the Map struct, and all the proxy methods of Map struct can be removed.

@emirpasic
Copy link
Owner

You are right. Moreover, do you think the solution below is better?

That was my question to you as well, what do you think, would that work for you?

On another note, just checked on Java's TreeMap that they do expose a lot of these method directly in TreeMap https://docs.oracle.com/javase/7/docs/api/java/util/TreeMap.html so I am wondering if GoDS should do the same. Perhaps accessing the Tree within the map would not be to intuitive and evident for most people and creating proxy functions in our treemap, as you have done, would still be the better approach.

Last night I was more inclined towards exposing the Tree, today I am more inclide towards your approach. Perhaps we could do both, since I can't make up my mind? :D

@loxp
Copy link
Contributor Author

loxp commented Sep 22, 2018

You are right. Moreover, do you think the solution below is better?

That was my question to you as well, what do you think, would that work for you?

On another note, just checked on Java's TreeMap that they do expose a lot of these method directly in TreeMap https://docs.oracle.com/javase/7/docs/api/java/util/TreeMap.html so I am wondering if GoDS should do the same. Perhaps accessing the Tree within the map would not be to intuitive and evident for most people and creating proxy functions in our treemap, as you have done, would still be the better approach.

Last night I was more inclined towards exposing the Tree, today I am more inclide towards your approach. Perhaps we could do both, since I can't make up my mind? :D

Is it ok that exposing the Node struct to TreeMap user? If ok, I think exposing the Tree struct is better. In Java, a Map.Entry can be exposed, but in gods there isn't a equivalent abstraction.

@emirpasic
Copy link
Owner

after thinking it though a bit, i like your approach better, i.e. creating proxy functions rather than exposing the tree. i will merge this into development branch, make some changes and merge into master thereafter. the changes are minor documentation/comment updates and change of the output parameters to resemble e.g.

// Max returns the maximum key and its value from the tree map.
// Returns nil, nil if map is empty.
func (m *Map) Max() (key interface{}, value interface{}) {
	if node := m.tree.Right(); node != nil {
		return node.Key, node.Value
	}
	return nil, nil
}

in other words removing the thrid output paramter.

@emirpasic emirpasic changed the base branch from master to development September 22, 2018 23:22
@emirpasic emirpasic merged commit 95d9cc1 into emirpasic:development Sep 22, 2018
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.

2 participants