Writing a class that represents a sorted list of filenames
up vote
4
down vote
favorite
I have a question:
Write a class that represents a sorted list of filenames. The filenames are sorted by most recently added first. Duplicates are not allowed.
And I wrote this solution:
public class RecentlyUsedList
{
private readonly List<string> items;
public RecentlyUsedList()
{
items = new List<string>();
}
public void Add(string newItem)
{
if (items.Contains(newItem))
{
int position = items.IndexOf(newItem);
string existingItem = items[position];
items.RemoveAt(position);
items.Insert(0, existingItem);
}
else
{
items.Insert(0, newItem);
}
}
public int Count
{
get
{
int size = items.Count;
return size;
}
}
public string this[int index]
{
get
{
int position = 0;
foreach (string item in items)
{
if (position == index)
return item;
++position;
}
throw new ArgumentOutOfRangeException();
}
}
}
Now, other person asked me, do the code review of your own code and tell the answers of following questions:
Three operations are required:
How long is the list?
Access filename at a given position
Add a filename to the list; if it already exists in the list it gets moved to the top otherwise the new filename is simply prepended to the
top.
I got confused and tried to look on Google but I didn't get much from there. What would be the best answers to these questions?
c# sorting
New contributor
add a comment |
up vote
4
down vote
favorite
I have a question:
Write a class that represents a sorted list of filenames. The filenames are sorted by most recently added first. Duplicates are not allowed.
And I wrote this solution:
public class RecentlyUsedList
{
private readonly List<string> items;
public RecentlyUsedList()
{
items = new List<string>();
}
public void Add(string newItem)
{
if (items.Contains(newItem))
{
int position = items.IndexOf(newItem);
string existingItem = items[position];
items.RemoveAt(position);
items.Insert(0, existingItem);
}
else
{
items.Insert(0, newItem);
}
}
public int Count
{
get
{
int size = items.Count;
return size;
}
}
public string this[int index]
{
get
{
int position = 0;
foreach (string item in items)
{
if (position == index)
return item;
++position;
}
throw new ArgumentOutOfRangeException();
}
}
}
Now, other person asked me, do the code review of your own code and tell the answers of following questions:
Three operations are required:
How long is the list?
Access filename at a given position
Add a filename to the list; if it already exists in the list it gets moved to the top otherwise the new filename is simply prepended to the
top.
I got confused and tried to look on Google but I didn't get much from there. What would be the best answers to these questions?
c# sorting
New contributor
1
Your class supports all three operations, so what exactly are you confused about?
– Pieter Witvoet
14 hours ago
@Iztoksson Commenting on the use of whitespace would be a legitimate point for an answer. Changes to the code in the question, especially by people other than the OP, are rarely a good idea here on Code Review.
– Graipher
13 hours ago
What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
– Mathieu Guindon
12 hours ago
add a comment |
up vote
4
down vote
favorite
up vote
4
down vote
favorite
I have a question:
Write a class that represents a sorted list of filenames. The filenames are sorted by most recently added first. Duplicates are not allowed.
And I wrote this solution:
public class RecentlyUsedList
{
private readonly List<string> items;
public RecentlyUsedList()
{
items = new List<string>();
}
public void Add(string newItem)
{
if (items.Contains(newItem))
{
int position = items.IndexOf(newItem);
string existingItem = items[position];
items.RemoveAt(position);
items.Insert(0, existingItem);
}
else
{
items.Insert(0, newItem);
}
}
public int Count
{
get
{
int size = items.Count;
return size;
}
}
public string this[int index]
{
get
{
int position = 0;
foreach (string item in items)
{
if (position == index)
return item;
++position;
}
throw new ArgumentOutOfRangeException();
}
}
}
Now, other person asked me, do the code review of your own code and tell the answers of following questions:
Three operations are required:
How long is the list?
Access filename at a given position
Add a filename to the list; if it already exists in the list it gets moved to the top otherwise the new filename is simply prepended to the
top.
I got confused and tried to look on Google but I didn't get much from there. What would be the best answers to these questions?
c# sorting
New contributor
I have a question:
Write a class that represents a sorted list of filenames. The filenames are sorted by most recently added first. Duplicates are not allowed.
And I wrote this solution:
public class RecentlyUsedList
{
private readonly List<string> items;
public RecentlyUsedList()
{
items = new List<string>();
}
public void Add(string newItem)
{
if (items.Contains(newItem))
{
int position = items.IndexOf(newItem);
string existingItem = items[position];
items.RemoveAt(position);
items.Insert(0, existingItem);
}
else
{
items.Insert(0, newItem);
}
}
public int Count
{
get
{
int size = items.Count;
return size;
}
}
public string this[int index]
{
get
{
int position = 0;
foreach (string item in items)
{
if (position == index)
return item;
++position;
}
throw new ArgumentOutOfRangeException();
}
}
}
Now, other person asked me, do the code review of your own code and tell the answers of following questions:
Three operations are required:
How long is the list?
Access filename at a given position
Add a filename to the list; if it already exists in the list it gets moved to the top otherwise the new filename is simply prepended to the
top.
I got confused and tried to look on Google but I didn't get much from there. What would be the best answers to these questions?
c# sorting
c# sorting
New contributor
New contributor
edited 44 mins ago
Jamal♦
30.2k11115226
30.2k11115226
New contributor
asked 16 hours ago
raman
1213
1213
New contributor
New contributor
1
Your class supports all three operations, so what exactly are you confused about?
– Pieter Witvoet
14 hours ago
@Iztoksson Commenting on the use of whitespace would be a legitimate point for an answer. Changes to the code in the question, especially by people other than the OP, are rarely a good idea here on Code Review.
– Graipher
13 hours ago
What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
– Mathieu Guindon
12 hours ago
add a comment |
1
Your class supports all three operations, so what exactly are you confused about?
– Pieter Witvoet
14 hours ago
@Iztoksson Commenting on the use of whitespace would be a legitimate point for an answer. Changes to the code in the question, especially by people other than the OP, are rarely a good idea here on Code Review.
– Graipher
13 hours ago
What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
– Mathieu Guindon
12 hours ago
1
1
Your class supports all three operations, so what exactly are you confused about?
– Pieter Witvoet
14 hours ago
Your class supports all three operations, so what exactly are you confused about?
– Pieter Witvoet
14 hours ago
@Iztoksson Commenting on the use of whitespace would be a legitimate point for an answer. Changes to the code in the question, especially by people other than the OP, are rarely a good idea here on Code Review.
– Graipher
13 hours ago
@Iztoksson Commenting on the use of whitespace would be a legitimate point for an answer. Changes to the code in the question, especially by people other than the OP, are rarely a good idea here on Code Review.
– Graipher
13 hours ago
What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
– Mathieu Guindon
12 hours ago
What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
– Mathieu Guindon
12 hours ago
add a comment |
2 Answers
2
active
oldest
votes
up vote
10
down vote
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.
add a comment |
up vote
5
down vote
In addition to Henrik Hansen's excellent answer, I'll add a note about encapsulation.
Your class is currently acting as a wrapper around a List, hiding that list from public view. The only way for other actors to access that list is through your Add
, Count
, and indexer methods. This is a good thing, because it allows you to enforce the guarantee that the order of the elements in the list will be meaningful.
The danger that I see is this: Perhaps in the future someone will want to iterate through this List and, having no way to do that, will simply pop in and make your private List into a protected, internal, or even public List. This will break the encapsulation, and you will no longer be able to guarantee that the order of elements is meaningful: an external class with a reference to the list itself will be able to add and remove elements at will.
For that reason, I would consider implementing IEnumerable<string>
on your class. It can be as simple as adding these two lines:
public IEnumerator<string> GetEnumerator() => items.GetEnumerator();
public IEnumerator GetEnumerator() => this.GetEnumerator();
Now, adding this behavior before it's actually required is toying with YAGNI, so you should consider carefully before you do so. But, you may find that it's a good idea and a good fit for your class.
add a comment |
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
10
down vote
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.
add a comment |
up vote
10
down vote
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.
add a comment |
up vote
10
down vote
up vote
10
down vote
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.
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.
edited 12 hours ago
answered 12 hours ago
Henrik Hansen
6,3831724
6,3831724
add a comment |
add a comment |
up vote
5
down vote
In addition to Henrik Hansen's excellent answer, I'll add a note about encapsulation.
Your class is currently acting as a wrapper around a List, hiding that list from public view. The only way for other actors to access that list is through your Add
, Count
, and indexer methods. This is a good thing, because it allows you to enforce the guarantee that the order of the elements in the list will be meaningful.
The danger that I see is this: Perhaps in the future someone will want to iterate through this List and, having no way to do that, will simply pop in and make your private List into a protected, internal, or even public List. This will break the encapsulation, and you will no longer be able to guarantee that the order of elements is meaningful: an external class with a reference to the list itself will be able to add and remove elements at will.
For that reason, I would consider implementing IEnumerable<string>
on your class. It can be as simple as adding these two lines:
public IEnumerator<string> GetEnumerator() => items.GetEnumerator();
public IEnumerator GetEnumerator() => this.GetEnumerator();
Now, adding this behavior before it's actually required is toying with YAGNI, so you should consider carefully before you do so. But, you may find that it's a good idea and a good fit for your class.
add a comment |
up vote
5
down vote
In addition to Henrik Hansen's excellent answer, I'll add a note about encapsulation.
Your class is currently acting as a wrapper around a List, hiding that list from public view. The only way for other actors to access that list is through your Add
, Count
, and indexer methods. This is a good thing, because it allows you to enforce the guarantee that the order of the elements in the list will be meaningful.
The danger that I see is this: Perhaps in the future someone will want to iterate through this List and, having no way to do that, will simply pop in and make your private List into a protected, internal, or even public List. This will break the encapsulation, and you will no longer be able to guarantee that the order of elements is meaningful: an external class with a reference to the list itself will be able to add and remove elements at will.
For that reason, I would consider implementing IEnumerable<string>
on your class. It can be as simple as adding these two lines:
public IEnumerator<string> GetEnumerator() => items.GetEnumerator();
public IEnumerator GetEnumerator() => this.GetEnumerator();
Now, adding this behavior before it's actually required is toying with YAGNI, so you should consider carefully before you do so. But, you may find that it's a good idea and a good fit for your class.
add a comment |
up vote
5
down vote
up vote
5
down vote
In addition to Henrik Hansen's excellent answer, I'll add a note about encapsulation.
Your class is currently acting as a wrapper around a List, hiding that list from public view. The only way for other actors to access that list is through your Add
, Count
, and indexer methods. This is a good thing, because it allows you to enforce the guarantee that the order of the elements in the list will be meaningful.
The danger that I see is this: Perhaps in the future someone will want to iterate through this List and, having no way to do that, will simply pop in and make your private List into a protected, internal, or even public List. This will break the encapsulation, and you will no longer be able to guarantee that the order of elements is meaningful: an external class with a reference to the list itself will be able to add and remove elements at will.
For that reason, I would consider implementing IEnumerable<string>
on your class. It can be as simple as adding these two lines:
public IEnumerator<string> GetEnumerator() => items.GetEnumerator();
public IEnumerator GetEnumerator() => this.GetEnumerator();
Now, adding this behavior before it's actually required is toying with YAGNI, so you should consider carefully before you do so. But, you may find that it's a good idea and a good fit for your class.
In addition to Henrik Hansen's excellent answer, I'll add a note about encapsulation.
Your class is currently acting as a wrapper around a List, hiding that list from public view. The only way for other actors to access that list is through your Add
, Count
, and indexer methods. This is a good thing, because it allows you to enforce the guarantee that the order of the elements in the list will be meaningful.
The danger that I see is this: Perhaps in the future someone will want to iterate through this List and, having no way to do that, will simply pop in and make your private List into a protected, internal, or even public List. This will break the encapsulation, and you will no longer be able to guarantee that the order of elements is meaningful: an external class with a reference to the list itself will be able to add and remove elements at will.
For that reason, I would consider implementing IEnumerable<string>
on your class. It can be as simple as adding these two lines:
public IEnumerator<string> GetEnumerator() => items.GetEnumerator();
public IEnumerator GetEnumerator() => this.GetEnumerator();
Now, adding this behavior before it's actually required is toying with YAGNI, so you should consider carefully before you do so. But, you may find that it's a good idea and a good fit for your class.
answered 10 hours ago
benj2240
5067
5067
add a comment |
add a comment |
raman is a new contributor. Be nice, and check out our Code of Conduct.
raman is a new contributor. Be nice, and check out our Code of Conduct.
raman is a new contributor. Be nice, and check out our Code of Conduct.
raman is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f208515%2fwriting-a-class-that-represents-a-sorted-list-of-filenames%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
1
Your class supports all three operations, so what exactly are you confused about?
– Pieter Witvoet
14 hours ago
@Iztoksson Commenting on the use of whitespace would be a legitimate point for an answer. Changes to the code in the question, especially by people other than the OP, are rarely a good idea here on Code Review.
– Graipher
13 hours ago
What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
– Mathieu Guindon
12 hours ago