Skip to content

Dialac dev#16

Open
dckcosta wants to merge 2 commits intoellismg:add-system-sortingfrom
dckcosta:dialac-dev
Open

Dialac dev#16
dckcosta wants to merge 2 commits intoellismg:add-system-sortingfrom
dckcosta:dialac-dev

Conversation

@dckcosta
Copy link

@dckcosta dckcosta commented Mar 3, 2016

This is the refactoring class for Array.Sort method.
This work is tracked by Issue - #13
One change that happened since the issue was filled is that Instead of having Sorting Class generic, I made the method Sorting.Sort Generic because of backwards compatibility for the existent overloaded methods that receives Key and Value pair.

/// <param name="index"></param>
/// <param name="length"></param>
/// <param name="comparer"></param>
internal virtual void SortInternal<T>(T[] array, int index, int length, IComparer<T> comparer) //where T : IComparable
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better if this was protected and abstract. It being abstract will require it to be implemented.

The convention we follow for methods were you have to implement them but not do argument checking is to suffix the method with the word "Core", so I would also rename this to SortCore.

@ellismg
Copy link
Owner

ellismg commented Mar 8, 2016

Would you mind doing a pass over this and removing the commented out code if it's not needed?


T[] control = SimpleSort<T>(array, index, length, comparer);

//TODO - Can use Reflection to get the derivedTypes of Sorting and invoking the methods.
Copy link
Owner

Choose a reason for hiding this comment

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

I call out later why I think this is a bad idea here but if you needed to do this at some other point you would want to use the MakeGenericType method on TypeInfo to substitute concrete values for the type parameters and then use Activator.CreateInstance to build an instance of that type.

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.

3 participants

Comments