Skip to content

RTS 795#589

Merged
alexmoore merged 9 commits intodevelopfrom
RTS-795/DESCRIBE-Command
Jan 11, 2016
Merged

RTS 795#589
alexmoore merged 9 commits intodevelopfrom
RTS-795/DESCRIBE-Command

Conversation

@alexmoore
Copy link
Contributor

Built off the work from RTS-665, this adds a fancier interface to the describe command.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be more logical to use inheritance instead of aggregation:

class FullColumnDescription extends ColumnDescription

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started out that way, but since we don't have any cross-use cases between the two I think it's better to keep them separate. If we want to add inheritance later we can without breaking stuff. If we ever wanted to remove inheritance though we'd be out of luck. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

What about passing them as an argument:

// somewhere in a customer's code
void takimgCareAboutTheSchema(Iterable< ColumnDescription > descriptions)

How can I pass FullColumnDescription to this ^^ routine?

@alexmoore
Copy link
Contributor Author

@srgg I think this covers the issues we talked about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that we need to pre-cache keys stuff, since not all users need to use it.

IMO, it is better to introduce utilitarian assessors which will return newly created collections each time. As a result caching will be performed on the user side, if the user really needs this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lean the other way, we can easily create it while in the constructor, and then we have an entire immutable object once we're done. If we do it later then we're saving 8? object references, and introducing more code and possible race conditions later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the whole instance is immutable from the user perspective, there shouldn't be any race conditions. On the other hand, as we are not expecting that user will execute describe table each second, I think that we can leave this as is.

@srgg
Copy link
Contributor

srgg commented Jan 11, 2016

Looks good to me

@alexmoore
Copy link
Contributor Author

Verbal 👍 from @srgg

alexmoore added a commit that referenced this pull request Jan 11, 2016
@alexmoore alexmoore merged commit 8eed28f into develop Jan 11, 2016
@alexmoore alexmoore deleted the RTS-795/DESCRIBE-Command branch January 11, 2016 17:02
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.

2 participants