Add new line.

Why?

http://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

Avoid unnecessary formatting

The formatting is not unnecessary. I added DefaultParameterSetName to the Cmdlet attribute. The line becomes far too long and hard to read. Formatting makes the content readable without unnecessary side-scrolling. Unless the PS Team requires this change for some reason, this code stays as is.

Consider grouping similar tests below in Context

If I need to go into this PR to address issues with the logic I'll consider adding context to the Pester tests. Otherwise they are grouped as other tests in the file were grouped, and I'm comfortable with that.

1. Move duplicate initializers to BeforeAll {} (and throughout all code below too)
2. Why do you use so complex data? I would prefer 1,2,-3,... a,b,z,... Enough 5 or 6 values.

BeforeAll isn't documented (I didn't even know it existed), and I simply followed the model in the file already.

The numeric data used was intentionally generated using Get-Random with the range of all int values so that the test would result in a random outcome that covered a broad base of numeric input. With sorting and -Top/-Bottom, 5 or 6 values to start with is not always a sufficient test, so I'll leave it with 10 values.

Many tests on this repository contain BeforeAll. See these examples.

Simple source data easier to understand and easier to maintain. I would say that it is better to have the test which return results you can evaluate your own eyes.

As per our test guidelines, please do use BeforeAll.

It would be better to move repeated code to functions.

I had written a function at first, with the tests being simply one or two lines of code; this resulted in a complicated function which would have to be tested by itself. I really didn't like this model, and preferred a short test with a half-dozen lines of code or so, which allows anyone to review a failing test in place and see more easily why it may be failing, plus enable testing outside of Pester by simply copying and pasting the test contents.

This doesn't need to be public, right?
Also consider making this a struct, it seems like that would be a win performance wise.

Changed locally, will commit with other changes soon. On the struct comment, should this be a struct as well then, for the performance win?

    internal sealed class OrderByPropertyEntry
    {
        internal PSObject inputObject = null;
        internal List<ObjectCommandPropertyValue> orderValues = new List<ObjectCommandPropertyValue>();
    }

Avoid initializing properties with their default values. Old code may be doing this, but there is no reason to do so and we'll fix the old code eventually.

Fixed locally, will commit soon.

Again, avoid initializing properties with their default values.

Fixed locally, will commit soon.

The actual sort should be in it's own method.

Moved entire body of else block except for WriteObject to new Heapify method, will commit soon.

This needlessly creates a new list and copies the elements.

Replaced with RemoveRange, will commit soon.

RemoveRange might be worse - it copies and clears.

I was thinking of iterating over the range explicitly with integers instead of the enumerator of the list.

As per our guidelines, avoid formatting changes. I realize test files are inconsistent w.r.t. tabs vs. spaces, but we want to keep meaningful additions in different commits from formatting changes.

@lzybkr Understood. So should I convert that file back to use tabs instead of spaces?

Please do.

As per our test guidelines, please do use BeforeAll.

It seems silly to splat an empty hashtable.

It seems a little silly to splat a single parameter here.

Why not create $unsortedData just once in BeforeAll?

Actually, after a closer look, these tests are a good example of why Pester has a parameter -TestCases to It. Please consider changing the tests to use -TestCases, I think it will simplify these test quite a bit.

Returning the heap suggests it's not done in place even though it is.

Maybe return heapCount instead?

Yeah, I should have made that change when I switched to in-place sorting. It's done locally now.

spelling: compariing

Fixed local, thanks.

Can you handle -Unique here too? If they compare equal, you just skip the item, right?

I don't think it's that simple. When I looked at supporting -Unique in heapify, my initial reaction was that it wouldn't work, at least not the way you might think. Since the data in the heap is not sorted, the challenge when checking for uniqueness of each value is which values you compare against. If the next value could go into the heap (as determined by comparing with the root), then I'd have to look at children of the current item (root) to see if I either find the same item or determine whether or not it would be possible to find the same item in a child of the item (in which case I'd have to recurse into that child item and repeat with its children). Worst case would result in N comparisons, where N is the size of the heap. Best case would be 1 comparison. Anyhow, that's roughly what I thought would be required. I could try it and see how well (or not) that performs vs the current method.

Array.Equals is reference equality - so this can't be what you meant.

Array does implement IStructuralEquatable, so if you can create an EqualityComparer, you could use something like:

([System.Collections.IStructuralEquatable]$nSortEntry).Equals($fullSortEntry,
    [System.Collections.Generic.EqualityComparer[int]]::Default) | Should Be $true

Or if you really meant to use reference equality, use [object]::ReferenceEquality and add a comment why.

I'll add the comment and update to the other reference equality test you suggested. I use reference equality because I'm verifying arrays are sorted into the proper position.

Am I mistaken in assuming you can easily handle -Unique in heapify?

Same comment about -Unique and heapify applies here. I'm testing something with it though to see what impact it might have.

As long as you're splatting, you can simplify with something like:

... = @{Descending = $orderType -eq 'descending'}

And you decide to change this, you might as well fix the case to be consistent.

As is, if you're doing an ascending sort, no parameter is passed. If I change this then an ascending sort passes -Descending:$false. I suppose for my tests that doesn't matter much since I'm verifying output order and not explicitly testing the switch parameter, so I changed it to simplify the logic (that's what I had before anyway).

You aren't using any of the assignments, so can they be removed?

Sure, I removed them. I would use the assignments when testing various things outside of Pester, but it's good now so I took them out.

My first thought was about Compare-Object, because many tests using it, but Sort-Object is specific. So maybe for the future explicitly add: 'Compare-Object is not applicable here!'

Previously I had in mind, that these variables ($topBottom and $topBottomAscendingDescending) are good candidates to move in BeforeAll.

That doesn't buy anything though -- those variables don't change.