Skip to main content
added 2 characters in body
Source Link
user73941
user73941

Empty lines as separation around code lines that are related are always a good idea. Your code has a little too much empty lines where it is not common in C# (or C/C++, Java, JavaScript):

public class RecentlyUsedList

{

This in fact decreases readability. Instead just write:

public class RecentlyUsedList
{

      private readonly List<string> items;

      public RecentlyUsedList()

      {

        items = new List<string>();

      }

Here the constructor is not necessary. Just do:

private readonly List<string> items = new List<string>();

Your Add(...) method can be simplified to this:

  public void Add(string newItem)
  {
    items.Remove(newItem);
    items.Insert(0, newItem);
  }

items.Remove(newItem) just returns false, if the item is not present, so it's safe to use in any case. There is no need to be concerned about the existing string because it is the same as the newItem.


public int Count... can be simplified to:

public int Count => items.Count;

List<string> items

has an indexer it self, so yoyou can use that when implementing the indexer:

  public string this[int index]
  {
    get
    {
      if (index < 0 || index >= items.Count)
      {
        throw new ArgumentOutOfRangeException();
      }
      return items[index];
    }
  }

Here I throw an exception if the index argument is out of range. You could let items handle that as well...

In fact your foreach-loop is potentially much slower than the List<T>[index] because you make a kind of search where List<T>[index] just performperforms a look up. So you hide an efficient behavior with a not so efficient one.

Empty lines as separation around code lines that are related are always a good idea. Your code has a little too much empty lines where it is not common in C# (or C/C++, Java, JavaScript):

public class RecentlyUsedList

{

This in fact decreases readability. Instead just write:

public class RecentlyUsedList
{

      private readonly List<string> items;

      public RecentlyUsedList()

      {

        items = new List<string>();

      }

Here the constructor is not necessary. Just do:

private readonly List<string> items = new List<string>();

Your Add(...) method can be simplified to this:

  public void Add(string newItem)
  {
    items.Remove(newItem);
    items.Insert(0, newItem);
  }

items.Remove(newItem) just returns false, if the item is not present, so it's safe to use in any case. There is no need to be concerned about the existing string because it is the same as the newItem.


public int Count... can be simplified to:

public int Count => items.Count;

List<string> items

has an indexer it self, so yo can use that when implementing the indexer:

  public string this[int index]
  {
    get
    {
      if (index < 0 || index >= items.Count)
      {
        throw new ArgumentOutOfRangeException();
      }
      return items[index];
    }
  }

Here I throw an exception if the index argument is out of range. You could let items handle that as well...

In fact your foreach-loop is potentially much slower than the List<T>[index] because you make a kind of search where List<T>[index] just perform a look up. So you hide an efficient behavior with a not so efficient one.

Empty lines as separation around code lines that are related are always a good idea. Your code has a little too much empty lines where it is not common in C# (or C/C++, Java, JavaScript):

public class RecentlyUsedList

{

This in fact decreases readability. Instead just write:

public class RecentlyUsedList
{

      private readonly List<string> items;

      public RecentlyUsedList()

      {

        items = new List<string>();

      }

Here the constructor is not necessary. Just do:

private readonly List<string> items = new List<string>();

Your Add(...) method can be simplified to this:

  public void Add(string newItem)
  {
    items.Remove(newItem);
    items.Insert(0, newItem);
  }

items.Remove(newItem) just returns false, if the item is not present, so it's safe to use in any case. There is no need to be concerned about the existing string because it is the same as the newItem.


public int Count... can be simplified to:

public int Count => items.Count;

List<string> items

has an indexer it self, so you can use that when implementing the indexer:

  public string this[int index]
  {
    get
    {
      if (index < 0 || index >= items.Count)
      {
        throw new ArgumentOutOfRangeException();
      }
      return items[index];
    }
  }

Here I throw an exception if the index argument is out of range. You could let items handle that as well...

In fact your foreach-loop is potentially much slower than the List<T>[index] because you make a kind of search where List<T>[index] just performs a look up. So you hide an efficient behavior with a not so efficient one.

added 26 characters in body
Source Link
user73941
user73941

Empty lines as separation around code lines that are related are always a good idea. Your code has a little too much empty lines where it is not common in C# (or C/C++, Java, JavaScript):

public class RecentlyUsedList

{

This in fact decreases readability. Instead just write:

public class RecentlyUsedList
{

      private readonly List<string> items;

      public RecentlyUsedList()

      {

        items = new List<string>();

      }

Here the constructor is not necessary. Just do:

private readonly List<string> items = new List<string>();

Your Add(...) method can be simplified to this:

  public void Add(string newItem)
  {
    items.Remove(newItem);
    items.Insert(0, newItem);
  }

items.Remove(newItem) just returns false, if the item is not present, so it's safe to use in any case. There is no need to be concerned about the existing string because it is the same as the newItem.


public int Count... can be simplified to:

public int Count => items.Count;

List<string> items

has an indexer it self, so yo can use that when implementing the indexer:

  public string this[int index]
  {
    get
    {
      if (index < 0 || index >= items.Count)
      {
        throw new ArgumentOutOfRangeException();
      }
      return items[index];
    }
  }

Here I throw an exception if the index argument is out of range. You could let items handle that as well...

In fact your foreach-loop is potentially much slower than the List<T>[index] because you make a kind of search where List<T>[index] just perform a look up. So you hide an efficient behavior with a not so efficient one.

Empty lines as separation around code lines that are related are always a good idea. Your code has a little too much empty lines where it is not common in C# (or C/C++, Java, JavaScript):

public class RecentlyUsedList

{

This in fact decreases readability. Instead just write:

public class RecentlyUsedList
{

      private readonly List<string> items;

      public RecentlyUsedList()

      {

        items = new List<string>();

      }

Here the constructor is not necessary. Just do:

private readonly List<string> items = new List<string>();

Your Add(...) method can be simplified to this:

  public void Add(string newItem)
  {
    items.Remove(newItem);
    items.Insert(0, newItem);
  }

items.Remove(newItem) just returns false, if the item is not present, so it's safe to use in any case. There is no need to be concerned about the existing string because it is the same as the newItem.


public int Count... can be simplified to:

public int Count => items.Count;

List<string> items

has an indexer it self, so yo can use that when implementing the indexer:

  public string this[int index]
  {
    get
    {
      if (index < 0 || index >= items.Count)
        throw new ArgumentOutOfRangeException();
      return items[index];
    }
  }

Here I throw an exception if the index argument is out of range. You could let items handle that as well...

In fact your foreach-loop is potentially much slower than the List<T>[index] because you make a kind of search where List<T>[index] just perform a look up. So you hide an efficient behavior with a not so efficient one.

Empty lines as separation around code lines that are related are always a good idea. Your code has a little too much empty lines where it is not common in C# (or C/C++, Java, JavaScript):

public class RecentlyUsedList

{

This in fact decreases readability. Instead just write:

public class RecentlyUsedList
{

      private readonly List<string> items;

      public RecentlyUsedList()

      {

        items = new List<string>();

      }

Here the constructor is not necessary. Just do:

private readonly List<string> items = new List<string>();

Your Add(...) method can be simplified to this:

  public void Add(string newItem)
  {
    items.Remove(newItem);
    items.Insert(0, newItem);
  }

items.Remove(newItem) just returns false, if the item is not present, so it's safe to use in any case. There is no need to be concerned about the existing string because it is the same as the newItem.


public int Count... can be simplified to:

public int Count => items.Count;

List<string> items

has an indexer it self, so yo can use that when implementing the indexer:

  public string this[int index]
  {
    get
    {
      if (index < 0 || index >= items.Count)
      {
        throw new ArgumentOutOfRangeException();
      }
      return items[index];
    }
  }

Here I throw an exception if the index argument is out of range. You could let items handle that as well...

In fact your foreach-loop is potentially much slower than the List<T>[index] because you make a kind of search where List<T>[index] just perform a look up. So you hide an efficient behavior with a not so efficient one.

Source Link
user73941
user73941

Empty lines as separation around code lines that are related are always a good idea. Your code has a little too much empty lines where it is not common in C# (or C/C++, Java, JavaScript):

public class RecentlyUsedList

{

This in fact decreases readability. Instead just write:

public class RecentlyUsedList
{

      private readonly List<string> items;

      public RecentlyUsedList()

      {

        items = new List<string>();

      }

Here the constructor is not necessary. Just do:

private readonly List<string> items = new List<string>();

Your Add(...) method can be simplified to this:

  public void Add(string newItem)
  {
    items.Remove(newItem);
    items.Insert(0, newItem);
  }

items.Remove(newItem) just returns false, if the item is not present, so it's safe to use in any case. There is no need to be concerned about the existing string because it is the same as the newItem.


public int Count... can be simplified to:

public int Count => items.Count;

List<string> items

has an indexer it self, so yo can use that when implementing the indexer:

  public string this[int index]
  {
    get
    {
      if (index < 0 || index >= items.Count)
        throw new ArgumentOutOfRangeException();
      return items[index];
    }
  }

Here I throw an exception if the index argument is out of range. You could let items handle that as well...

In fact your foreach-loop is potentially much slower than the List<T>[index] because you make a kind of search where List<T>[index] just perform a look up. So you hide an efficient behavior with a not so efficient one.