10
\$\begingroup\$

I am new to programming and have been taking the time to learn C# by reading, doing some classes online for Unity, and using Youtube. I think you learn more from doing your own projects so I decided to write a program that asked the user to enter a movie they have not seen and then store the movie titles into a list and print out the movies they listed. I had problems getting it to take multiple movies but finally figured a way to do it but I believe there is a better way to write the program. So just asking general advice to improve upon what I have already written. Thank you.

 using System;
 using System.Collections.Generic;

 namespace MovieListProjectWithSwitch
 {
  internal class Program
  {
    private static void Main(string[] args)
    {
        Console.WriteLine("Enter a movie you want to see but have not seen it");
        var movieNotSeen = new List<string>();
        movieNotSeen.Add(Console.ReadLine());

        Console.WriteLine("Do you want to enter another movie? Y/N?");

        const string answerYes = "y";
        const string answerNo = "n";
        string userInput;

        while (true)
        {
            userInput = Console.ReadLine().ToLower();

            switch (userInput)
            {
                case answerYes:
                    {
                        Console.WriteLine("Enter your next movie you wish to add to the list");
                        movieNotSeen.Add(Console.ReadLine());
                        Console.WriteLine("Now the movies on your list are:");
                        Console.WriteLine();
                        foreach (var movie in movieNotSeen)
                            Console.WriteLine(movie);
                        Console.WriteLine();

                        Console.WriteLine("Enter another movie? Y/N?");
                        var userInput2 = Console.ReadLine().ToLower();
                        if (userInput2 == "y")
                        {
                            goto case answerYes;
                        }
                        else
                        {
                            goto case answerNo;
                        }
                    }

                case answerNo:
                    {
                        Console.WriteLine("Your movies in your list are:\n");
                        foreach (var movies in movieNotSeen)
                            Console.WriteLine(movies);
                        return;
                    }
            }
        }
    }
  }
 }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ An example of a better title might be "List the movies I haven't seen". \$\endgroup\$
    – pacmaninbw
    Jul 8, 2020 at 11:56

2 Answers 2

21
\$\begingroup\$

Welcome to codereview. Here are my suggestions:

Functional decomposition and Naming

The trickiest part of programming is to decouple your complex problem into smaller easier (sub)problems. It is easier to tackle small and focused problems than one large and vague. So, my first suggestion would be to try to find small and independent pieces of functionalities.

In your case here is one way to decompose:

  • Ask for a movie and store it
  • Ask for continuation
  • Print out the already gathered movies

You can directly map these to functions:

void EnterANewMovieAndStoreIt()
bool WishToContinue()
void PrintOutMovies()

Then you can compose them like this:

Ask for a movie and store it then
Print out the already gathered movies until
the consumer Answers with yes for continuation
when (s)he Answers with no then Print out already gathered movies

With that in hand your Main function will look like this:

static void Main(string[] args)
{
    do
    {
       EnterANewMovieAndStoreIt();

       PrintOutMovies();

    } while (WishToContinue());

    PrintOutMovies();
}

Store and Retrieve functionalities

Because we have separated storage and retrieval functionalities that's why they can be really small and concise functions:

private static void EnterANewMovieAndStoreIt()
{
    Console.WriteLine();
    Console.WriteLine("Please enter a movie you want to see but have not seen it yet.");
    var movieName = Console.ReadLine();
    ToBeWatchedMovies.Add(movieName);
}

private static void PrintOutMovies()
{
    Console.WriteLine();
    Console.WriteLine("Your haven't seen but to be watched movies:");
    foreach (var movie in ToBeWatchedMovies)
        Console.WriteLine(movie);
}

As you can see they rely on a shared resource called ToBeWatchedMovies. It is defined on a class level like this: static readonly List<string> ToBeWatchedMovies = new List<string>();

Error handling

Your current implementation does not handle that case when the user enters something different then Y or N. It is a good practice to assume that a userInput can be anything and handle that accordingly. In this example we can easily ask them nicely to try to provide the answer again.

So, the WishToContinue would look like this:

private static bool WishToContinue()
{
    while(true)
    {
        Console.WriteLine();
        Console.WriteLine("Do you want to enter another movie? Y/N?");
        var userInput = Console.ReadLine();

        if (string.Equals(userInput, AnswerYes, StringComparison.OrdinalIgnoreCase))
            return true;

        if (string.Equals(userInput, AnswerNo, StringComparison.OrdinalIgnoreCase))
            return false;

        Console.WriteLine($"Please provide either '{AnswerYes}' or '{AnswerNo}' ");
    }
}

Obviously this code could be further optimized but my main point here is that you should try to handle the wrong inputs as well.


Let's put everything together:

class Program
{
    static readonly List<string> ToBeWatchedMovies = new List<string>();
    const string AnswerYes = "y", AnswerNo = "n";
    static void Main(string[] args)
    {
        do
        {
           EnterANewMovieAndStoreIt();

           PrintOutMovies();

        } while (WishToContinue());

        PrintOutMovies();
    }

    private static void EnterANewMovieAndStoreIt()
    {
        Console.WriteLine();
        Console.WriteLine("Please enter a movie you want to see but have not seen it yet.");
        var movieName = Console.ReadLine();
        ToBeWatchedMovies.Add(movieName);
    }

    private static void PrintOutMovies()
    {
        Console.WriteLine();
        Console.WriteLine("Your movies in your list are:");
        foreach (var movie in ToBeWatchedMovies)
            Console.WriteLine(movie);
    }

    private static bool WishToContinue()
    {
        while(true)
        {
            Console.WriteLine();
            Console.WriteLine("Do you want to enter another movie? Y/N?");
            var userInput = Console.ReadLine();

            if (string.Equals(userInput, AnswerYes, StringComparison.OrdinalIgnoreCase))
                return true;

            if (string.Equals(userInput, AnswerNo, StringComparison.OrdinalIgnoreCase))
                return false;

            Console.WriteLine($"Please provide either '{AnswerYes}' or '{AnswerNo}' ");
        }
    }
}

I hope this helped you a bit.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ A final decomposition step: Refactor code out of the .NET generic entry point static void Main to a separate class. s.. v.. Main should only ever be where the "driver" program is instantiated and called. Name? MovieList, MovieListMain, MovieListDriver, etc., whatever.` \$\endgroup\$
    – radarbob
    Sep 10, 2022 at 18:10
8
\$\begingroup\$

One of the things you might want to consider as an extension to this program is to store the list of movies in a file and read the list back from a file. Also there should be a way to delete movies once they have been seen. Any other suggestions I would give have already been given in another review.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.