Good practice for validating items in class.

Question

I actually found giving this thread a title quite hard. Im currently taking a dot Net programming course in C#. We've been given the task of building a simple library using windows forms and Entity Frameworks using a repository service pattern.

I have a form where I'm adding a Book entity to the database aka adding a new book to the library. Things I am doing in this class are checking the fields to make sure that the user actually entered some text, the ISBN number is the right format, the book doesn't already exist.... you get the point. What I've been trying to decide is how to structure the way the processes are carried out. When I click to submit the new book I originally had a bunch of if statements in the on_click method that do my validation.

private void btn_bookSubmit_Click(object sender, EventArgs e)
    {
        string  newBookName    = this.tb_bookTitle.Text;
        string  newBookAuthor  = this.tb_bookAuthor.Text;
        string  newBookISBN    = this.tb_bookISBN.Text;
        string  description    = this.tb_bookDesc.Text;



        if (bookIsNotValid(newBookISBN, newBookName, newBookAuthor))
        {
            MessageBox.Show(this.validationError);
            return;
        }
        if (bookService.BookTitleExists(newBookName))
        {
            MessageBox.Show("A book by this title already exists in the library");
            return;
        }
        if (bookService.ISBNExists(newBookISBN))
        {
            MessageBox.Show("This ISBN belongs to another book in the library.  Please double check the ISBN number and try again");
            return;
        }
        if (this.authorService.doesAuthorExistByName(newBookAuthor))
        {
            DialogResult result = MessageBox.Show
                ("This author does not exist in the database.  Do you want to add this author?",
                "Author Does not Exist", MessageBoxButtons.YesNo);

            if (result == DialogResult.Yes) this.authorService.addAuthor(newBookAuthor);
            if (result == DialogResult.No)
            {
                MessageBox.Show("New book entry cancled.  In order to enter a new book into the system a valid Author must be specified");
                return ;
            }
        }


        bookService.addBook(newBookISBN, newBookName, newBookAuthor, description);


        MessageBox.Show(
            string.Format("{0} succesfully added to the library", newBookName), 
            string.Format("{0} added Successfully", newBookName), 
            MessageBoxButtons.OK);

        this.clearFields(); 
    }

And I thought to myself; Thats quite a lot of code for one method. So I split it up into more private functions in the form class and ended up with a method that looked like this instead:

private void btn_bookSubmit_Click(object sender, EventArgs e)
    {
        string  newBookName    = this.tb_bookTitle.Text;
        string  newBookAuthor  = this.tb_bookAuthor.Text;
        string  newBookISBN    = this.tb_bookISBN.Text;
        string  description    = this.tb_bookDesc.Text;

        if (!isBookValid(newBookISBN, newBookName, newBookAuthor))  return;
        if (!isTitleValid(newBookName))                             return;
        if (!isISBNvalid(newBookISBN))                              return;
        if (!isAuthorNew(newBookAuthor))                            return;

        bookService.addBook(newBookISBN, newBookName, newBookAuthor, description);


        MessageBox.Show(
            string.Format("{0} succesfully added to the library", newBookName), 
            string.Format("{0} added Successfully", newBookName), 
            MessageBoxButtons.OK);

        this.clearFields(); 
    }

Now I have quite a few methods in my class. Is that good practice? It looks a lot cleaner to me, but maybe its harder to sift through methods when going over my class than it is to see all of it happening within the one function. The other thing I thought about was to move all of my validation into one function instead of many, but then I'd have to handle the boolean return and how to stop my function differently.

I've been studying my program for 2 years now, have tried out, javascript, php, html5, c++, C, now c# trying to figure out which I enjoy the most. What I've taken out the most from all the programming is that I love beautiful and efficient code. I may not be able to do that yet, but Im trying my damndest to learn it. So any other shitty practices you might notice please let me know. So far everything works fine in the class and my real question is, is the way I'm implementing my validation process ok? good? shitty? slow?

Answer 1

Hmm, does your book service not take a book object?

Rather than your form doing validation of what's a valid book, it seems like it should maybe book the responsibility of a Book class to determine whether it's valid.

Some validation, like a required field being left blank can be a form level validation concern... but a lot of this sounds like the domain of book service and/or a book class.