Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

Edit: it's been pointed out to me by Mat's Mug that there is a tiny performance difference between .Where(predicate).FirstOrDefault() and .FirstOrDefault(predicate) there is a tiny performance difference between .Where(predicate).FirstOrDefault() and .FirstOrDefault(predicate) so you might want to keep this under consideration.

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

Edit: it's been pointed out to me by Mat's Mug that there is a tiny performance difference between .Where(predicate).FirstOrDefault() and .FirstOrDefault(predicate) so you might want to keep this under consideration.

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

Edit: it's been pointed out to me by Mat's Mug that there is a tiny performance difference between .Where(predicate).FirstOrDefault() and .FirstOrDefault(predicate) so you might want to keep this under consideration.

added 336 characters in body
Source Link
Jeroen Vannevel
  • 11.6k
  • 2
  • 39
  • 79

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

Edit: it's been pointed out to me by Mat's Mug that there is a tiny performance difference between .Where(predicate).FirstOrDefault() and .FirstOrDefault(predicate) so you might want to keep this under consideration.

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

Edit: it's been pointed out to me by Mat's Mug that there is a tiny performance difference between .Where(predicate).FirstOrDefault() and .FirstOrDefault(predicate) so you might want to keep this under consideration.

added 1628 characters in body
Source Link
Jeroen Vannevel
  • 11.6k
  • 2
  • 39
  • 79

Minor remark (I'll look for more later):

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

Minor remark (I'll look for more later):

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

Source Link
Jeroen Vannevel
  • 11.6k
  • 2
  • 39
  • 79
Loading
lang-cs

AltStyle によって変換されたページ (->オリジナル) /