Many to many crud using entity framework 4.1

Question

My view submits data to the controller using Json objects which contains child objects.It allows users to add/remove/modify the relationship with child entities(authors,categories,LIbraryBookCopy). Users can only select authors and categories from list but can update/add LibraryBookCopy object.

This is my first MVC project and I have done this so far . Am I doing too much manipulation on the data? Is this the right way of doing this?

[HttpPost]
    public ActionResult Edit(BookViewModel bookv)
    {
        bool found=false;
        Mapper.CreateMap<AuthorViewModel, Author>();
        Mapper.CreateMap<CategoryViewModel, Category>();
        Mapper.CreateMap<LibraryBookCopyViewModel, LibraryBookCopy>();
        List<Author> authors = Mapper.Map<List<AuthorViewModel>, List<Author>>(bookv.Authors.ToList());
        List<Category> categories = Mapper.Map<List<CategoryViewModel>, List<Category>>(bookv.Categories.ToList());
        bookv.Authors.Clear();
        bookv.Categories.Clear();
        Mapper.CreateMap< BookViewModel,Book>();
        Book  book = Mapper.Map<BookViewModel,Book>(bookv);
        List<LibraryBookCopy> toBeDeletedLibraryBookCopies = new List<LibraryBookCopy>();
        List<LibraryBookCopy> toBeAddedLibraryBookCopies = new List<LibraryBookCopy>();
        List<LibraryBookCopy> toBeUpdatedLibraryBookCopies = new List<LibraryBookCopy>();

        List<LibraryBookCopy> libraryBookCopiesFromDatabase = new List<LibraryBookCopy>();
       db.Books.Attach(book);

        //Assign categories to book
       foreach (Category c in categories)    { db.Categories.Attach(c); }

       book.Categories.Clear();

       foreach (Category c in categories)    { book.Categories.Add(c); }

     //Assign authors to book
     foreach (Author a in authors)           { db.Authors.Attach(a);   }

      book.Authors.Clear();

            foreach (Author a in authors)    {book.Authors.Add(a); }

            if (bookv.LibraryBookCopies != null)
            {
                List<LibraryBookCopy> bookCopiesFromView = Mapper.Map<List<LibraryBookCopyViewModel>, List<LibraryBookCopy>>(bookv.LibraryBookCopies);

                libraryBookCopiesFromDatabase = db.LibraryBookCopies.Where(lbc=>lbc.BookId==book.BookId).ToList();

                foreach (LibraryBookCopy bc in libraryBookCopiesFromDatabase)
                {
                    foreach (LibraryBookCopy bcv in bookCopiesFromView)
                    {
                        if ((bc.LibraryId == bcv.LibraryId) )
                        {
                            found = true;
                            toBeUpdatedLibraryBookCopies.Add(bcv);
                            break;
                        }

                    }

                    if (!found)
                    {
                       toBeDeletedLibraryBookCopies.Add(bc);
                    }
                    found = false;
                }

                //remove objects moved to toBeDelted

                foreach (LibraryBookCopy lbc in toBeDeletedLibraryBookCopies )
                {

                    libraryBookCopiesFromDatabase.Remove(lbc);
                }

                //NOW FIND NEW BOOK COPIES TO BE ADDED
                foreach (LibraryBookCopy bcv in bookCopiesFromView)
                {
                    if (libraryBookCopiesFromDatabase.Count == 0)
                    {
                        toBeAddedLibraryBookCopies.Add(bcv);
                    }

                    else
                    {
                        foreach (LibraryBookCopy bc in libraryBookCopiesFromDatabase)
                        {
                            if ((bc.LibraryId != bcv.LibraryId))
                            {

                                toBeAddedLibraryBookCopies.Add(bcv);
                            }
                        }
                    }
                }


            }
            else
            {
                book.LibraryBookCopies.Clear();
            }

        //mark copies as deleted
        foreach (LibraryBookCopy lbcv in toBeDeletedLibraryBookCopies)
        {
          //  db.LibraryBookCopies.Attach(lbcv);
           db.LibraryBookCopies.DeleteObject(lbcv);
        }

        //Add library ref to each libraryBookCopies object in Book
        foreach (LibraryBookCopy lBC in toBeAddedLibraryBookCopies)
        {
            db.LibraryBookCopies.AddObject(lBC);
        }
        db.ObjectStateManager.ChangeObjectState(book, EntityState.Modified);
        db.SaveChanges();
        return RedirectToAction("Index");

    }

Answer 1

Doing too much on the Controller is subjective. If you do not have a Data Access Layer, I guess this is where you have to write this manipulation logic. However, you can consider followings;

• Pick meaningful names which are intention-revealing and pronounceable instead of encoding (such as bookv, c, a, bc, bcv, lBC).
• Split methods so they are small enough to one thing and give descriptive names. In this way you obey to do 'method should do one and one only thing' and not to do anything that is not useful for the program.

You can start doing these by;

1. Rename variables for meaningful names.
2. Move mapping logic to a seperate method.
3. Move each FOREACH/IF statements to separate methods.
4. Encapsulate IF conditions from methods.
5. Encapsulate body content of FOREACH/IF statements to separate methods.
6. Encapsulate LINQ queries using filters.
7. Encapsulate object's state change from a method.

However, doing all these could be over engineering so you might need to find the balance.

Answer 2

I don't code in C#, so just two generic notes:

1. The following comments are good candidates for method names:

   • //Assign categories to book: assignCategories, assignCategoriesToBook
   • //Assign authors to book: assignAuthors, assignAuthorsToBook
   • //remove objects moved to toBeDelted: removeObjects
   • //NOW FIND NEW BOOK COPIES TO BE ADDED: findNewBooks
   • //mark copies as deleted: markDeleted
   • //Add library ref to each libraryBookCopies object in Book: addLibraryReference

2. Short variable names (like lBC, lbcv, bcv, bookv) are not too readable. I suppose you have autocomplete, so using longer names does not mean more typing but I'd help readers and maintainers a lot since they don't have to remember the purpose of each variable - the name would express the programmers intent.