Return to Answer

###Random observations...

Why is the ListViewColumnSorter class partial?

Compare

I think the code could get clearer here. First thing I'd address, I'd declare a int result; and return only once.

Then the declarations and casting:

ListViewItem listviewX, listviewY;
// Cast the objects to be compared to ListViewItem objects
listviewX = (ListViewItem)x;
listviewY = (ListViewItem)y;

Not sure what this is buying you. Wouldn't it be more readable like this?

var listViewX = x as ListViewItem;
var listViewY = y as ListViewItem;
if (listViewX == null || listViewY == null) throw new ArgumentException();

Throwing an ArgumentException (perhaps with a more detailed message) would be less surprising than the InvalidCastException thrown by the explicit cast if x or y isn't a ListViewItem.

The naming of the variables is confusing. You have int valueX and then string xValue:

// Compare the two items
int valueX;
int valueY;
string xValue = listviewX.SubItems[ColumnToSort].Text;
string yValue = listviewY.SubItems[ColumnToSort].Text;

If xValue is the text value of the column to sort for ListViewX, wouldn't textValueX and textValueY be more descriptive names?

Instead of timeFormat != "" you should be testing for !String.IsNullOrEmpty(timeFormat).

Private fields

Your private fields don't follow C# naming conventions:

private SortOrder OrderOfSort;

Should be orderOfSort or _orderOfSort... although I don't see a reason for it not to be _sortOrder or sortOrder.

#region

#region is usually a code smell in itself. It's not so bad in this case, because it's wrapping an entire class, but then why wrap a class in a #region anyway? Move the class into its own file!

switch (m.Msg)

I think I'd try to convert that to an enum with the values you're expecting, so as to eliminate the cryptic 0x1007, 0x104D and 0x1008 "magic numbers", making your cases look like:

case WndProcMessage.ListViewItemAddedA:
case WndProcMessage.ListViewItemAddedW:
case WndProcMessage.ListViewItemRemoved:

Swallowed Exceptions

SetAlternateColor has this catch block:

 catch (Exception)
 {
 }

Are you expecting a particular exception type? This code is swallowing all exceptions, which is bad. At least output it somewhere (log, debug output, whatever):

 catch (Exception exception)
 {
 Console.WriteLine(exception);
 }

Random observations...

Why is the ListViewColumnSorter class partial?

Compare

I think the code could get clearer here. First thing I'd address, I'd declare a int result; and return only once.

Then the declarations and casting:

ListViewItem listviewX, listviewY;
// Cast the objects to be compared to ListViewItem objects
listviewX = (ListViewItem)x;
listviewY = (ListViewItem)y;

Not sure what this is buying you. Wouldn't it be more readable like this?

var listViewX = x as ListViewItem;
var listViewY = y as ListViewItem;
if (listViewX == null || listViewY == null) throw new ArgumentException();

Throwing an ArgumentException (perhaps with a more detailed message) would be less surprising than the InvalidCastException thrown by the explicit cast if x or y isn't a ListViewItem.

The naming of the variables is confusing. You have int valueX and then string xValue:

// Compare the two items
int valueX;
int valueY;
string xValue = listviewX.SubItems[ColumnToSort].Text;
string yValue = listviewY.SubItems[ColumnToSort].Text;

If xValue is the text value of the column to sort for ListViewX, wouldn't textValueX and textValueY be more descriptive names?

Instead of timeFormat != "" you should be testing for !String.IsNullOrEmpty(timeFormat).

Private fields

Your private fields don't follow C# naming conventions:

private SortOrder OrderOfSort;

Should be orderOfSort or _orderOfSort... although I don't see a reason for it not to be _sortOrder or sortOrder.

#region

#region is usually a code smell in itself. It's not so bad in this case, because it's wrapping an entire class, but then why wrap a class in a #region anyway? Move the class into its own file!

switch (m.Msg)

I think I'd try to convert that to an enum with the values you're expecting, so as to eliminate the cryptic 0x1007, 0x104D and 0x1008 "magic numbers", making your cases look like:

case WndProcMessage.ListViewItemAddedA:
case WndProcMessage.ListViewItemAddedW:
case WndProcMessage.ListViewItemRemoved:

Swallowed Exceptions

SetAlternateColor has this catch block:

 catch (Exception)
 {
 }

Are you expecting a particular exception type? This code is swallowing all exceptions, which is bad. At least output it somewhere (log, debug output, whatever):

 catch (Exception exception)
 {
 Console.WriteLine(exception);
 }

###Random observations...

Why is the ListViewColumnSorter class partial?

Compare

I think the code could get clearer here. First thing I'd address, I'd declare a int result; and return only once.

Then the declarations and casting:

ListViewItem listviewX, listviewY;
// Cast the objects to be compared to ListViewItem objects
listviewX = (ListViewItem)x;
listviewY = (ListViewItem)y;

Not sure what this is buying you. Wouldn't it be more readable like this?

var listViewX = x as ListViewItem;
var listViewY = y as ListViewItem;
if (listViewX == null || listViewY == null) throw new ArgumentException();

Throwing an ArgumentException (perhaps with a more detailed message) would be less surprising than the InvalidCastException thrown by the explicit cast if x or y isn't a ListViewItem.

The naming of the variables is confusing. You have int valueX and then string xValue:

// Compare the two items
int valueX;
int valueY;
string xValue = listviewX.SubItems[ColumnToSort].Text;
string yValue = listviewY.SubItems[ColumnToSort].Text;

If xValue is the text value of the column to sort for ListViewX, wouldn't textValueX and textValueY be more descriptive names?

Instead of timeFormat != "" you should be testing for !String.IsNullOrEmpty(timeFormat).

Private fields

Your private fields don't follow C# naming conventions:

private SortOrder OrderOfSort;

Should be orderOfSort or _orderOfSort... although I don't see a reason for it not to be _sortOrder or sortOrder.

#region

#region is usually a code smell in itself. It's not so bad in this case, because it's wrapping an entire class, but then why wrap a class in a #region anyway? Move the class into its own file!

switch (m.Msg)

I think I'd try to convert that to an enum with the values you're expecting, so as to eliminate the cryptic 0x1007, 0x104D and 0x1008 "magic numbers", making your cases look like:

case WndProcMessage.ListViewItemAddedA:
case WndProcMessage.ListViewItemAddedW:
case WndProcMessage.ListViewItemRemoved:

Swallowed Exceptions

SetAlternateColor has this catch block:

 catch (Exception)
 {
 }

Are you expecting a particular exception type? This code is swallowing all exceptions, which is bad. At least give yourself a handle for a breakpoint, to inspect anything that might get caught there:

 catch (Exception exception)
 {
 Console.WriteLine(exception);
 }

###Random observations...

Why is the ListViewColumnSorter class partial?

Compare

I think the code could get clearer here. First thing I'd address, I'd declare a int result; and return only once.

Then the declarations and casting:

ListViewItem listviewX, listviewY;
// Cast the objects to be compared to ListViewItem objects
listviewX = (ListViewItem)x;
listviewY = (ListViewItem)y;

Not sure what this is buying you. Wouldn't it be more readable like this?

var listViewX = x as ListViewItem;
var listViewY = y as ListViewItem;
if (listViewX == null || listViewY == null) throw new ArgumentException();

Throwing an ArgumentException (perhaps with a more detailed message) would be less surprising than the InvalidCastException thrown by the explicit cast if x or y isn't a ListViewItem.

The naming of the variables is confusing. You have int valueX and then string xValue:

// Compare the two items
int valueX;
int valueY;
string xValue = listviewX.SubItems[ColumnToSort].Text;
string yValue = listviewY.SubItems[ColumnToSort].Text;

If xValue is the text value of the column to sort for ListViewX, wouldn't textValueX and textValueY be more descriptive names?

Instead of timeFormat != "" you should be testing for !String.IsNullOrEmpty(timeFormat).

Private fields

Your private fields don't follow C# naming conventions:

private SortOrder OrderOfSort;

Should be orderOfSort or _orderOfSort... although I don't see a reason for it not to be _sortOrder or sortOrder.

#region

#region is usually a code smell in itself. It's not so bad in this case, because it's wrapping an entire class, but then why wrap a class in a #region anyway? Move the class into its own file!

switch (m.Msg)

I think I'd try to convert that to an enum with the values you're expecting, so as to eliminate the cryptic 0x1007, 0x104D and 0x1008 "magic numbers", making your cases look like:

case WndProcMessage.ListViewItemAddedA:
case WndProcMessage.ListViewItemAddedW:
case WndProcMessage.ListViewItemRemoved:

Swallowed Exceptions

SetAlternateColor has this catch block:

 catch (Exception)
 {
 }

Are you expecting a particular exception type? This code is swallowing all exceptions, which is bad. At least output it somewhere (log, debug output, whatever):

 catch (Exception exception)
 {
 Console.WriteLine(exception);
 }