Problems breaking out of nested loops

Question

I have problems breaking out off these nested loops correctly. What the code is trying to do is to indicate that a customer has rented a certain movie. Both the movie and customer are compared to properties of arraylist objects and then if all checks out the name property and ID property of a movie object are added as a string to another arraylist. All this works correctly as long as I use the first movie (from movies) and the first customer (from customers) but if I try renting other movies further down my arraylist with other customers then it adds the rented movie to the customerRentedMovies arraylist but prints out the "else message". I figure I need to break out of the foreach(blabla) loops aswell? or could goto be used? Comments was removed (looked kinda messy, can explain further if needed)

public void RentMovie(string titel, int movieID, string name, int customerID) 
    {
        foreach (Customer customer in customers)  
        {
            if (name == customer.Name && customerID == customer.CustomerID)  
            {
                foreach (MovieInfo movie in movies)
                {
                    if (titel == movie.Titel && movieID == movie.MovieID)
                    {
                        movie.rented = true;
                        string rentedMovie = string.Format("{0}  ID: {1}", movie.Titel, movie.MovieID);
                        customer.customerRentedMovies.Add(rentedMovie); 

                        break; 
                    }

                    else { Console.WriteLine("No movie with that titel and ID!"); } 

                }
             break;      
            }
            else { Console.WriteLine("No customer with that ID and name"); } 
        }

    }

Probably a dupe of http://stackoverflow.com/questions/324831/breaking-out-of-a-nested-loop — Mike Christensen, Nov 16 '11 at 17:49
Why do you have nested loops to begin with? Find the movie, then find the customer. — dlev, Nov 16 '11 at 17:52

score 4 · Answer 1 · answered Nov 16 '11 at 17:54

It strikes me that actually you don't need nested loops at all - you're not changing movies based on customer anyway. Additionally, I'd use LINQ. So:

var customer = customers.FirstOrDefault(c => c.Name == customerName && 
                                             c.CustomerId == customerId);
if (customer == null)
{
    Console.WriteLine("No customer with that ID and name");
    return;
}

var movie = movies.FirstOrDefault(m => m.Name == movieName && 
                                  m.MovieId == movieId);

if (movie == null)
{
    Console.WriteLine("No movie with that ID and name");
    return;
}

movie.rented = true;
string rentedMovie = string.Format("{0}  ID: {1}", movie.Titel, movie.MovieID);
customer.customerRentedMovies.Add(rentedMovie);

(I'd probably actually change what's returned or throw an exception if the customer or movie couldn't be found, but that's another matter.)

The important point is that now there are no explicit loops - we say what we're trying to find declaratively and act accordingly. Likewise there's no nesting, which separates the two concerns (finding a movie and finding a customer). We could now easily extract each of those parts into a separate method - particularly if we were using exceptions instead of logging and returning. It would then be:

Customer customer = FindCustomer(customerId, customerName);
Movie movie = FindMovie(movieId, movieName);

movie.rented = true;
string rentedMovie = string.Format("{0}  ID: {1}", movie.Titel, movie.MovieID);
customer.customerRentedMovies.Add(rentedMovie);

Much simpler.

+1: I really wonder how you find the time... I have a mental picture of you typing 400 words a minute, answering questions asynchronously in batches of 4. — James Johnson, Nov 16 '11 at 18:22

score 2 · Accepted Answer · answered Nov 16 '11 at 18:28

Your method is violating the Single Responsibility rule -- each class or method should have one and only one responsibility and reason for change. You have a single method that is responsible for doing 3 different things:

Finding a customer
Finding a movie
renting the movie to the customer.

This makes it

difficult to test
difficult to maintain
hard to understand

This is a Code Smell.

You should refactor your method something like this, delegating the responsibility for finding a customer and finding a movie to their own methods:

public void RentMovie( string titel , int movieID , string name , int customerID )
{
  Customer  customer = FindCustomer( customerID , name ) ;
  MovieInfo movie    = FindMovie( movieID , titel ) ;

  if ( customer == null )
  {
    Console.WriteLine("No customer with that ID and name");
  }

  if ( movie == null )
  {
    Console.WriteLine("No movie with that titel and ID!") ;
  }

  if ( customer != null && movie != null )
  {
    string rentedMovie = string.Format( "{0}  ID: {1}" , movie.Titel , movie.MovieID );
    movie.rented = true;
    customer.customerRentedMovies.Add( rentedMovie );
  }

  return ;
}

If you aren't using Linq, the FindCustomer() and FindMovie methods might look like this:

private MovieInfo FindMovie( int movieID , string titel )
{
  MovieInfo instance = null ;
  foreach( MovieInfo movie in movies )
  {
    if ( movie.MovieID == movieID && movie.Titel == titel )
    {
      instance = movie ;
      break ;
    }
  }
  return instance ;
}

private Customer FindCustomer( int customerID , string name )
{
  Customer instance = null ;
  foreach( Customer customer in customers )
  {
    if ( customer.CustomerID == customerID && customer.Name == name )
    {
      instance = customer ;
      break ;
    }
  }
  return instance ;
}

If you're using Ling, the same methods might be:

private MovieInfo FindMovie( int movieID , string titel )
{
  return movies.Where( x => x.MovieID == movieID && x.Titel == titel ).SingleOrDefault() ;
}

private Customer FindCustomer( int customerID , string name )
{
  return customers.Where( x => x.Name == name && x.CustomerID == customerID ).SingleOrDefault() ;
}

The code is now much simpler, easier to understand and self-describing. When it's time to make changes, the changes will be easier to make as well.

Thanks a lot for all the answers. Just need to change the code now, as I said before not sure I'm allowed to use LINQ. It is for school and they can't seem to understand that newer "versions" of C# are better... anyways think I can make an exception, can't see them failing me since this does look a lot simpler :) — user1040281, Nov 16 '11 at 18:35

score 1 · Answer 3 · answered Nov 16 '11 at 17:50

If you want to break out of both loops, just use return.

foreach (Customer customer in customers)   
{ 
    if (name == customer.Name && customerID == customer.CustomerID)   
    { 
        foreach (MovieInfo movie in movies) 
        { 
            if (titel == movie.Titel && movieID == movie.MovieID) 
            { 
                movie.rented = true; 
                string rentedMovie = string.Format("{0}  ID: {1}", movie.Titel, movie.MovieID); 
                customer.customerRentedMovies.Add(rentedMovie);  

                return; //break out of both loops  
            } 
            else { Console.WriteLine("No movie with that titel and ID!"); }       
        }                         
    }                 
    else { Console.WriteLine("No customer with that ID and name"); }  
}

score 1 · Answer 4 · answered Nov 16 '11 at 17:50

1

Since you're doing nothing after your loop you can just call return; where you have your break.

answered Nov 16 '11 at 17:50

Jay Riggs

53,046
9
139
151

score 0 · Answer 5 · answered Nov 16 '11 at 17:55

0

If you're able to use LINQ, your code can become much cleaner:

var customer =
    customers.FirstOrDefault(x => x.Name == name && x.CustomerID == customerID);
if (customer == null) { // Error }

var movie = movies.FirstOrDefault(x => x.Title == title && x.MovieID == movieID);
if (movie == null) { // Error }

// Rental logic here

Also, shouldn't ID be enough to uniquely IDentify the objects?

answered Nov 16 '11 at 17:55

dlev

48,024
5
125
132

Yeah I know LINQ is a lot better but this is for school and they seem to have missed the fact that C# actually got better over the years, since we are using arraylists instead of List etc. The ID was a simple example. Thanks anyways :) – user1040281 Nov 16 '11 at 18:02
@user1040281 Fair enough. Nonetheless, the general structure should still be doable: write a method that finds customers, write a method that finds movies, then call those with the appropriate parameters. – dlev Nov 16 '11 at 18:03

score 0 · Answer 6 · answered Nov 16 '11 at 17:58

If you want to show the error only when the "for" loop finishes, You'll need to setup some kind of flag, indicating that you couldn't find a movie or something. I've changed your code:

public void RentMovie(string titel, int movieID, string name, int customerID) 
    {
        bool hasCustomer, hasMovie;

        hasCustomer = false;
        hasMovie = false;

        foreach (Customer customer in customers)  
        {
            if (name == customer.Name && customerID == customer.CustomerID)  
            {
                hasCustomer = true;
                foreach (MovieInfo movie in movies)
                {
                    if (titel == movie.Titel && movieID == movie.MovieID)
                    {
                        hasMovie = true;
                        movie.rented = true;
                        string rentedMovie = string.Format("{0}  ID: {1}", movie.Titel, movie.MovieID);
                        customer.customerRentedMovies.Add(rentedMovie); 

                        break; 
                    }

                }
                if (hasMovie == false) {
                    Console.WriteLine("No movie with that titel and ID!");
                }
             break;      
            }
        }

        if (hasCustomer == false) {
            Console.WriteLine("No customer with that ID and name");
        }
    }

score 0 · Answer 7 · answered Nov 16 '11 at 18:02

The If/Else in the first foreach is going to get hit for every customer if you have 25 customers then it's going to hit the else statement 10 times before it finds the right customer (assuming the right customer is the 11th one) why don't you use linq to get and set the values

    Customer customer = customers.FirstOrDefault(x => x.Name = name && x.CustomerID = customerID);

    if(customer != null) //will be null if customer isn't found
    {
        //DO the same thing here for the movies and once you find the 
        //movie set the properties and add to the collection
    }

Problems breaking out of nested loops

7 Answers7