2
\$\begingroup\$

Questions:

  • Is there a better way other than assigning IDs manually and creating separate lists for each model the way I have? (for this particular scenario).
  • If so, how? (Still taking the conditions into account)

The conditions are:

Through an external source get a list of unique book names with their corresponding author name.

  • book and author might exist in the db. If so we dont want to add them again.
  • If the book doesnt exist the author might still exist. If so we only want to add the book with the existing authors id as FK.
  • If neither exist we will add both. The books gets the authors id as FK.
  • If the book exists the author will exist too.

Through another external source retrieve extended book info (ie Topic and book description) to all books that does not exist in the db.

  • Always add the retrieved description to the book.
  • Topic might exist in the db. If so assign the existing topics id as FK to the book.
  • If Topic doesnt exist in the db then save it and assign its id as FK to the book.

Start here (might be all you need to see depending on your opinion):

[HttpPost]
public ActionResult GetNewData(FormCollection collection)
{
    Tuple<List<Models.Author>, List<Models.Book>, List<Models.Topic>> newData = GetAuthorBookTopic();

    foreach (Models.Author newAuthor in newData.Item1)
    {
        context.Authors.Add(newAuthor);
        context.SaveChanges();
    }
    foreach (Models.Book newBook in newData.Item2)
    {
        context.Books.Add(newBook);
        context.SaveChanges();
    }
    foreach (Models.Topic newTopic in newData.Item3)
    {
        context.Topics.Add(newTopic);
        context.SaveChanges();
    }
    return RedirectToAction("Index");
}

How I solved it:

static Tuple<List<Models.Author>, List<Models.Book>, List<Models.Topic>> GetAuthorBookTopic()
{
    DateTime? latestExistingBook = DB.BookLatestDate();
    int? highestBookID = DB.BookHighestID();
    int? highestAuthorID = DB.AuthorHighestID();

    List<Author> listOfNewAuthors = new List<Author>();
    List<Book> listOfNewBooks = new List<Book>();
    List<Topic> listOfNewTopics = new List<Topic>();


    var newData = externalSource.GetAuthorsAndBooks(); //Gets a list of unique book names together with the publication date and the authors name

    foreach (var newDataNode in newData)
    {
        DateTime newDataNodePublicationDate = newDataNode.publDate;

        if ( newDataNodePublicationDate > latestExistingBook )
        {
            Models.Author newAuthor = new Models.Author();
            Models.Book newBook = new Models.Book();

            // -- Prepare the Author -- //
            var authorName = newDataNode.authName;
            int AuthorListIndex = listOfNewAuthors.FindIndex(Author => Author.AuthorName.Equals(authorName)); //Find out if the Author exists in the list of Authors not yet saved to the db

            if (AuthorListIndex < 0) // We have not added the author to the list before.
            {
                int? existingAuthorDBId = DB.AuthorExists(authorName); //Find out if the Author exists in the db

                if (existingAuthorDBId == -1) //Author doesn't exist in the db so we will add him to the list
                {
                    highestAuthorID += 1;
                    newAuthor.AuthorName = authorName;
                    newAuthor.AuthorId = (int)highestAuthorID;
                    listOfNewAuthors.Add(newAuthor);
                }
                else
                    newAuthor.AuthorId = (int)existingAuthorDBId;
            }
            else
                newAuthor.AuthorId = listOfNewAuthors[AuthorListIndex].AuthorId;

            // -- Prepare the Book -- //
            var bookName = newDataNode.bookName;
            int bookInDB = DB.BookExists(bookName);

            if (bookInDB == -1) //Book does not exist in the db
            {
                highestBookID += 1;         
                newBook.BookId = (int)highestBookID;
                newBook.DateOfPublication = newDataNodePublicationDate;
                newBook.Name = bookName;
                newBook.AuthorId = newAuthor.AuthorId;
                listOfNewBooks.Add(newBook);

                GetExtendedBookInfo(newBook, listOfNewTopics); // Assign book description and see if Topic should be added.             
            }
        }
        i += 1;
    }
    return new Tuple<List<Author>, List<Book>, List<Topic>>(listOfNewAuthors, listOfNewBooks, listOfNewTopics);
}

static void GetExtendedBookInfo(Book book, List<Topic> listOfNewTopics)
{
    int highestTopicID = DB.HighestTopicID();

    Models.Topic newTopic = new Models.Topic();

    var extendedBookInfo = externalSource.GetExtendedBookInfo(book.Name);
    book.Description = extendedBookInfo.BookDescr; //add new info to book
    var topicName = extendedBookInfo.TopicName;

    //--- Determine if Topic should be added  --- //
    int topicListIndex = listOfNewTopics.FindIndex(t => t.Name.Equals(topicName)); //Find out if the topic exists in the list

    if (topicListIndex < 0) // We have not added the topic before.
    {
        int existingTopic = DB.TopicExists(topicName); //Find out if the topic exists in the db

        if (existingTopic != -1) //topic exists in the db
            newTopic.TopicId = existingTopic; // assign the id from the db
        else
        {
            highestTopicID += 1;
            newTopic.TopicId = highestTopicID;
            newTopic.Name = topicName;
            listOfNewTopics.Add(booksTopic);
        }
    }
    else
        newTopic.TopicId = listOfNewTopics[topicListIndex].TopicId; //assign the id from the topic in the list

    book.TopicId = newTopic.TopicId;        
}

Models:

public class Author
{
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    public int AuthorId { get; set; }
    [Required]
    public string AuthorName { get; set; }
    public virtual List<Book> Books { get; set; }
}

public class Book
{
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    public int BookId { get; set; }
    [Required]
    public DateTime DateOfPublication { get; set; }
    public string Name { get; set; }
    public string Description { get; set; }

    public int AuthorId { get; set; }
    [ForeignKey("AuthorId")]
    public virtual Models.Author Author { get; set; }

    public int TopicId { get; set; }
    [ForeignKey("TopicId")]
    public virtual Models.Topic Topic { get; set; }
}

public class Topic
{
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    public int TopicId { get; set; }
    [Required]
    public string Name { get; set; }
    public virtual List<Book> Books { get; set; }
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

There are definitely some things to say about this. As far as I can tell you're making things harder than they should be and by changing some things you can make your code a lot more readable. I'll work through your code first and look at your approach afterwards (mainly because I haven't read through it carefully yet).


public ActionResult GetNewData(FormCollection collection)

You're not actually using the form so can't it be omitted?


Tuple<List<Models.Author>, List<Models.Book>, List<Models.Topic>> newData = GetAuthorBookTopic();

99% of the time you think you need a Tuple, you don't. First look into whether this can be turned into 3 methods. If that isn't possible, return a simple object that holds these 3 collections as properties.


Models.Author

It doesn't read very fluently if you specify the namespace everywhere. I would suggest removing this for brevity.


foreach (Models.Author newAuthor in newData.Item1)
{
    context.Authors.Add(newAuthor);
    context.SaveChanges();
}
foreach (Models.Book newBook in newData.Item2)
{
    context.Books.Add(newBook);
    context.SaveChanges();
}

You don't have to call SaveChanges() after each insertion or each type of model. Simply call SaveChanges() once at the end of your method.


static Tuple<List<Models.Author>, List<Models.Book>, List<Models.Topic>> GetAuthorBookTopic()

This method shouldn't be called from a static context since all communication with this method should go through the controller in the first place.


DateTime? latestExistingBook = DB.BookLatestDate();

I assume DB is defined on instance level? Naming conventions for private instance level fields are either db or _db with the latter version the kind I prefer myself and more common.

Aside from this I would also pay attention to your naming: methods should describe the action being performed in it. This means there is always a verb in the name. In your case this would become GetLatestBook(). Notice how I left out "Date" because that is already specified by the return type.


I will note this right now: your code used a self-defined ID ([DatabaseGenerated(DatabaseGeneratedOption.None)]) only to mimic DatabaseGeneratedOption.Identity: you retrieve the highest ID and increment it for each entry. Get rid of all this and simply use the Identity generated option.

This goes for books, topics and authors.


List<Author> listOfNewAuthors = new List<Author>();

You don't have to specifically create intermediate collections once you dropped the static: you can just insert them inside this method.

Note that even if you go this route, at least define them as IList<T> to loosely couple your code a bit more.

Don't repeat the "list" part either: we already know it's a list.


GetAuthorsAndBooks()

Ideally this should be returned in two different methods, if possible. This follows the Single Responsibility Principle.


newDataNode

I would call it simply node since being part of newData is implied and you don't have to distinguish from other nodes.


DateTime newDataNodePublicationDate = newDataNode.publDate;
  1. Don't shorten names: it's PublicationDate
  2. Public properties are UpperCamelCase
  3. No need to spell out DateTime since the type is already obvious from the name
  4. You only use this field once so there's no point in assigning it to an intermediate variable

if ( newDataNodePublicationDate > latestExistingBook )

I assume this is a shortcut? This is a good place for a comment to specify why this is.


int AuthorListIndex = listOfNewAuthors.FindIndex();

This is a convoluted way of finding out whether something exists in a collection. Use Contains() instead to find out if a collection contains an item. If you also need to retrieve the item, use FirstOrDefault(). If you are using a query which should only return one item (which it should in this case) then use SingleOrDefault().


Author => Author.AuthorName.Equals(authorName)

The inner variable that represents each item in the collection (I'm sure there's an appropriate name for this but I can't recall it right now) follows the same convention as any other local field: lowerCamelCase.

Using == instead of .Equals() reads easier although you might want to consider using the proper overload taking a culture and compare options in account. Certainly with authors that have names from a non-latin character set this is a recommendation.


if (AuthorListIndex < 0)

This would then be replaced with a if(author == null) which more clearly conveys what you're trying to do.


int? existingAuthorDBId = DB.AuthorExists(authorName)

Boolean queries return booleans, not integers! You are trying to do two things here: see if the author exists and return the ID. I am okay with you putting that in the same method but you're putting the focus on the wrong task.

Instead I would name it int? GetAuthorId(string name) and return null if no record was found. Although I might just skip this altogether and use Author GetAuthor(string name).


newAuthor.AuthorName = authorName;
newAuthor.AuthorId = (int)highestAuthorID;

Can you see yourself busy when your author has 20 fields? I would suggest using a mapper tool like AutoMapper to create a new Author object or at least move the responsibility of this to a void Update(Author otherAuthor) method on your Author class.


newAuthor.AuthorId = (int)highestAuthorID;

The ID should always be returned in the correct type.


else
    newAuthor.AuthorId = (int)existingAuthorDBId;

Always use braces. Omitting them is prone to making mistakes when you're just a tiny bit inattentive.


You can apply the above commentary to books and topics as well. Note that this might indicate appropriateness for a generic method! It will take some setting up but in the end, each of these 3 aspects do the same things so it should be doable.

Your approach is the right one (only one?) so there's no remark on that. Once all these changes are implemented I think you'll find your code to be very easily readable and extendable.


Update

In response to your questions about retrieving the ID I made a little sample project to confirm my suspicions and it's like I thought: the original object's ID is being updated when inserting it into the database.

Take a look at the following code:

public class Author
{
    public int Id { get; set; }
    public string Name { get; set; }
    public DateTime DateOfBirth { get; set; }
}

public class LibraryContext : DbContext
{
    public LibraryContext()
    {
    }

    public LibraryContext(DbConnection connection) : base(connection, true)
    {
    }

    public DbSet<Author> Authors { get; set; }

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Author>().ToTable("Authors");
        modelBuilder.Entity<Author>().HasKey(x => x.Id);
        modelBuilder.Entity<Author>().Property(x => x.Id).HasDatabaseGeneratedOption(DatabaseGeneratedOption.Identity);
    }
}

[TestClass]
public class UnitTests
{
    private LibraryContext _context;

    [TestInitialize]
    public void Initialize()
    {
        _context = new LibraryContext(DbConnectionFactory.CreateTransient());
    }

    [TestMethod]
    public void InsertAuthor_WithoutId_AdjustsIdInExistingObject()
    {
        // Arrange
        var author = new Author
        {
            Name = "Dan Brown",
            DateOfBirth = new DateTime(1964, 06, 22)
        };

        // Act
        _context.Authors.Add(author);
        _context.SaveChanges();

        // Assert
        author.Id.Should().Be(1);
    }

    [TestMethod]
    public void InsertAuthors_WithoutIds_AdjustsIdsInExistingObjects()
    {
        // Arrange
        var firstAuthor = new Author
        {
            Name = "Dan Brown",
            DateOfBirth = new DateTime(1964, 06, 22)
        };

        var secondAuthor = new Author
        {
            Name = "Roald Dahl",
            DateOfBirth = new DateTime(1916, 08, 13)
        };

        // Act
        _context.Authors.Add(firstAuthor);
        _context.Authors.Add(secondAuthor);
        _context.SaveChanges();

        // Assert
        firstAuthor.Id.Should().Be(1);
        secondAuthor.Id.Should().Be(2);
        firstAuthor.Id.Should().NotBe(secondAuthor.Id);
    }
}

The tests speak for themselves but just to summarize: when I don't specify their ID (which is your usecase), add them to the DbContext and flush it to the database by calling SaveChanges() (the flushing is important since that's when the IDs get generated), the original objects have their ID updated to represent what is in the database.

This means that you won't have to query the database again just to get the generated ID; you can simply use the object you already had.

\$\endgroup\$
7
  • \$\begingroup\$ Thank you for your time. I will make most of the suggested changes. A few of them were unclear to me though: Regarding Tuple-No Tuple: Why is it be better to use an object instead? Regarding the Identity generated option: Potential scenario: I have added an author(A) to the listOfNewAuthors and later find that a new books(B) author is the same as A. Then I must have assigned an ID to A in order to let Bs FK point to A. If I let the ID autoincrement A wouldn't have an ID until it's saved in the DB. \$\endgroup\$
    – mdc
    Mar 23, 2015 at 12:21
  • \$\begingroup\$ Regarding: listOfNewAuthors.FindIndex(): I didn't actually need the full object, just its index if it exists. I assumed it would be more efficient. Regarding: int? existingAuthorDBId = DB.AuthorExists(authorName): I agree with renaming it but I think it would be unnecessary to retreieve a complete Author when all I need is the Id. Should be more efficient? \$\endgroup\$
    – mdc
    Mar 23, 2015 at 12:27
  • \$\begingroup\$ Tuples: if you need to return multiple distinct fields then you should group them in an object because a tuple's Item1, Item2, etc do not properly convey what they contain. It takes away a lot of clarity for no gain, really. Identity: I see. In that case I would entertain the idea of inserting the object in the database and flushing it (db.SaveChanges()) so you can retrieve the inserted object afterwards. You don't have to query it again I think, the Id field will have its value changed and otherwise you can just retrieve it from the local attached context. \$\endgroup\$ Mar 23, 2015 at 14:50
  • \$\begingroup\$ FindIndex(): Fair enough, although I don't think there's any performance impact (you're simply returning a reference to an existing object) and thus I would find that easier to use. But that might be more subjective. AuthorExists: once again you're just returning a reference to an existing object in the attached context so you're not creating anything. It's true that the change is not needed, I personally would just prefer to have that API exposed because it allows me to do more with it later if I would decide to want that. \$\endgroup\$ Mar 23, 2015 at 14:53
  • \$\begingroup\$ Identity: Hmm I'm unsure of how to implement this in a good way (it's basically my original question). If I save the object to the db then how would I retrieve it easily without querying the db when I need its ID? I guess I didn't to this in fear of having to query the DB every time I insert a new object (to get the id). On the other hand I could get rid of all the lists. Would you mind posting some code how you would do it? \$\endgroup\$
    – mdc
    Mar 23, 2015 at 15:44

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.