Writing a class that represents a sorted list of filenames












6














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?










share|improve this question




















  • 1




    Your class supports all three operations, so what exactly are you confused about?
    – Pieter Witvoet
    Nov 27 at 12:42










  • @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
    Nov 27 at 14:15










  • What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
    – Mathieu Guindon
    Nov 27 at 15:00
















6














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?










share|improve this question




















  • 1




    Your class supports all three operations, so what exactly are you confused about?
    – Pieter Witvoet
    Nov 27 at 12:42










  • @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
    Nov 27 at 14:15










  • What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
    – Mathieu Guindon
    Nov 27 at 15:00














6












6








6


1





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?










share|improve this question















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






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Nov 28 at 2:55









Jamal

30.3k11116226




30.3k11116226










asked Nov 27 at 11:08









raman

1334




1334








  • 1




    Your class supports all three operations, so what exactly are you confused about?
    – Pieter Witvoet
    Nov 27 at 12:42










  • @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
    Nov 27 at 14:15










  • What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
    – Mathieu Guindon
    Nov 27 at 15:00














  • 1




    Your class supports all three operations, so what exactly are you confused about?
    – Pieter Witvoet
    Nov 27 at 12:42










  • @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
    Nov 27 at 14:15










  • What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
    – Mathieu Guindon
    Nov 27 at 15:00








1




1




Your class supports all three operations, so what exactly are you confused about?
– Pieter Witvoet
Nov 27 at 12:42




Your class supports all three operations, so what exactly are you confused about?
– Pieter Witvoet
Nov 27 at 12:42












@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
Nov 27 at 14:15




@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
Nov 27 at 14:15












What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
– Mathieu Guindon
Nov 27 at 15:00




What version of C# are you using? Given 6.0+, there are a number of language opportunities here.
– Mathieu Guindon
Nov 27 at 15:00










2 Answers
2






active

oldest

votes


















11














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.






share|improve this answer































    7














    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.






    share|improve this answer





















      Your Answer





      StackExchange.ifUsing("editor", function () {
      return StackExchange.using("mathjaxEditing", function () {
      StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
      StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
      });
      });
      }, "mathjax-editing");

      StackExchange.ifUsing("editor", function () {
      StackExchange.using("externalEditor", function () {
      StackExchange.using("snippets", function () {
      StackExchange.snippets.init();
      });
      });
      }, "code-snippets");

      StackExchange.ready(function() {
      var channelOptions = {
      tags: "".split(" "),
      id: "196"
      };
      initTagRenderer("".split(" "), "".split(" "), channelOptions);

      StackExchange.using("externalEditor", function() {
      // Have to fire editor after snippets, if snippets enabled
      if (StackExchange.settings.snippets.snippetsEnabled) {
      StackExchange.using("snippets", function() {
      createEditor();
      });
      }
      else {
      createEditor();
      }
      });

      function createEditor() {
      StackExchange.prepareEditor({
      heartbeatType: 'answer',
      autoActivateHeartbeat: false,
      convertImagesToLinks: false,
      noModals: true,
      showLowRepImageUploadWarning: true,
      reputationToPostImages: null,
      bindNavPrevention: true,
      postfix: "",
      imageUploader: {
      brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
      contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
      allowUrls: true
      },
      onDemand: true,
      discardSelector: ".discard-answer"
      ,immediatelyShowMarkdownHelp:true
      });


      }
      });














      draft saved

      draft discarded


















      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

























      2 Answers
      2






      active

      oldest

      votes








      2 Answers
      2






      active

      oldest

      votes









      active

      oldest

      votes






      active

      oldest

      votes









      11














      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.






      share|improve this answer




























        11














        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.






        share|improve this answer


























          11












          11








          11






          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.






          share|improve this answer














          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.







          share|improve this answer














          share|improve this answer



          share|improve this answer








          edited Nov 27 at 15:39

























          answered Nov 27 at 15:07









          Henrik Hansen

          6,8081824




          6,8081824

























              7














              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.






              share|improve this answer


























                7














                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.






                share|improve this answer
























                  7












                  7








                  7






                  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.






                  share|improve this answer












                  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.







                  share|improve this answer












                  share|improve this answer



                  share|improve this answer










                  answered Nov 27 at 17:00









                  benj2240

                  6017




                  6017






























                      draft saved

                      draft discarded




















































                      Thanks for contributing an answer to Code Review Stack Exchange!


                      • Please be sure to answer the question. Provide details and share your research!

                      But avoid



                      • Asking for help, clarification, or responding to other answers.

                      • Making statements based on opinion; back them up with references or personal experience.


                      Use MathJax to format equations. MathJax reference.


                      To learn more, see our tips on writing great answers.





                      Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


                      Please pay close attention to the following guidance:


                      • Please be sure to answer the question. Provide details and share your research!

                      But avoid



                      • Asking for help, clarification, or responding to other answers.

                      • Making statements based on opinion; back them up with references or personal experience.


                      To learn more, see our tips on writing great answers.




                      draft saved


                      draft discarded














                      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





















































                      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







                      Popular posts from this blog

                      Probability when a professor distributes a quiz and homework assignment to a class of n students.

                      Aardman Animations

                      Are they similar matrix