How to validate a form?

[homerun4711]

Hello!

I am looking for a good way to validate a form.

The form contains:

- line edits: customer name and address, fetched from a qdialog
- qtableview: invoice positions

The approach I am using so far is to set a private member variable

int CustomerKey

int CustomerKey

To copy to clipboard, switch view to plain text mode 

if a customer was chosen from the list and inserted into the form and validate like this:

if (!CustomerKey)
        {
        QMessageBox::warning(0, QObject::tr("Problem"),"No customer selected");
        return 1;
        }

if (!CustomerKey)
        {
        QMessageBox::warning(0, QObject::tr("Problem"),"No customer selected");
        return 1;
        }

To copy to clipboard, switch view to plain text mode 

To check if invoice positions are present I use the following code:

QSqlQuery query;
 
query.exec("SELECT count(*) FROM tempinvoiceentries");
 
     if (query.next())
        {
        if (query.record().value(0).toInt() == 0) //improve check !!
        {
        QMessageBox::warning(0, QObject::tr("Problem"),"No invoice entries");
        return 1;
        }
        else
        {
            //valid entries present...    
            return 0;
        }
        }

QSqlQuery query;

query.exec("SELECT count(*) FROM tempinvoiceentries");

     if (query.next())
        {
        if (query.record().value(0).toInt() == 0) //improve check !!
        {
        QMessageBox::warning(0, QObject::tr("Problem"),"No invoice entries");
        return 1;
        }
        else
        {
            //valid entries present...    
            return 0;
        }
        }

To copy to clipboard, switch view to plain text mode 

This methods work so far, but I am not sure if they are sufficient. Do you have ideas how to improve the validation?

Kind regards,
HomeR

[BalaQT]

Hi,
Just a hint, First i will do the valid statements then error case.

QSqlQuery query;
 
query.exec("SELECT count(*) FROM tempinvoiceentries");
 
     if (query.next())
        {
        if (query.record().value(0).toInt() >0) //improve check !!
        {
            //valid entries present...    
            return 0;
        }
        else
        {
        QMessageBox::warning(0, QObject::tr("Problem"),"No invoice entries");
        return 1;
        }
        }

QSqlQuery query;

query.exec("SELECT count(*) FROM tempinvoiceentries");

     if (query.next())
        {
        if (query.record().value(0).toInt() >0) //improve check !!
        {
            //valid entries present...    
            return 0;
        }
        else
        {
        QMessageBox::warning(0, QObject::tr("Problem"),"No invoice entries");
        return 1;
        }
        }

To copy to clipboard, switch view to plain text mode 

[homerun4711]

Hi!

Thanks for your answer. The only problem is that there are more checks than I mentioned, so if I order the statements like you said, the first valid statement will return 0 and the other ones will not be executed. Do you know a good solution to this?

[squidge]

Personally, I like to put the actual data fetch and checks into a stored procedure and just call that from the code and check the result. Then whenever your software wants such data, it is automatically checked, you don't have to copy-paste code and so not going to miss something important. It also means modifying the database later on is much easier, as there is less dependance on the actual structure.

[homerun4711]

This sounds like good idea, but I don't completly unterstand what you mean.

In my case, the code I posted above is from a separate function validateData(), which is called before the data from a form is inserted into a database. If the validation fails, message boxes pop up or a border around Qlineedits are drawn to signal where the problem lies. Do you mean something like this?

[squidge]

Really, you have posted insufficient information. We are commenting on the code that you have posted, but then you say you do more checks further down the line, so our comments become useless. We don't know your form nor your database, so we are basically answering blind with generic information, so to speak.

I would change your return value too, returning 0 (FALSE) for OK seems a little strange. Imagine the code calling your validation - if (!ValidateForm()) { // Form is OK } looks odd and is not self-documenting. Personally, I would probably create an enum and return something like VALIDATE_SUCCESS or something like that.

Secondly, I would change the sql to a stored procedure, something like:

CREATE PROCEDURE isInvoiceEntriesPresent
AS
SELECT COUNT(1) FROM tempinvoiceentries;

CREATE PROCEDURE isInvoiceEntriesPresent
AS
SELECT COUNT(1) FROM tempinvoiceentries;

To copy to clipboard, switch view to plain text mode 

and then your query becomes more self-explanatory:

query.exec("{call isInvoiceEntriesPresent}");

query.exec("{call isInvoiceEntriesPresent}");

To copy to clipboard, switch view to plain text mode 

This also means you can change your method without having to recompile your code. Assume you are on a customers site with a problem, altering SQL is easier than having to do a full recompile and redeployment.

Unless your interested in the content of the data fields (ie, non-null), use COUNT(1) as above instead of COUNT(*). The former will give you more performance as it doesn't have to retrieve all the database data. Another reason for stored procedures is that they are typically pre-compiled (depending on the underlying implementation), so don't need to be parsed and offer even more performance.

etc...

[homerun4711]

Thanks for your comment. I have not posted all code because the other checks that are performed are similar to the two types that I mentioned above.

Some more information:
The form consists of
- editable QLineEdits (easy to verify)
- a QTableView containing a QStandardItemModel called "tempinvoiceentries" (see code)
- some non-editable QLineEdits that are filled with customer data after a customer is selected from a popup with a QTableView containing a QSqlTableModel with the customer data (after the call the variable CustomerKey is set, see code)

I would change your return value too, returning 0 (FALSE) for OK seems a little strange.

I am confused...I thought all applications return 0 on success, why should this be different while using functions?

Thanks for the hint to use

CREATE PROCEDURE isInvoiceEntriesPresent

CREATE PROCEDURE isInvoiceEntriesPresent

To copy to clipboard, switch view to plain text mode 

This is indeed a good possibility to improve the code and will make things easier, I was not aware of this. Also thanks for the hint to create an enum, so much to learn

What exactly is the difference between COUNT(*) and COUNT(1) ?

I read the sqlite docs on this

count(X)
The count(X) function returns a count of the number of times that X is not NULL in a group.

but I don't unterstand what this exactly means.

[squidge]

Applications return an error code in which 0 is 'The operation completed successfully' and other values are pre-defined error codes. If your function only returns SUCCESS or FAILURE (like the code snippet), then the closest match to these would be TRUE and FALSE. You could typedef these values as follows:

typedef enum {FAILURE = 0, SUCCESS  = !FAILURE} ErrorStatus;

typedef enum {FAILURE = 0, SUCCESS  = !FAILURE} ErrorStatus;

To copy to clipboard, switch view to plain text mode 

COUNT(*) will retrieve all rows matching your condition (in your case, all rows, as you have no limits placed, so it will become slower over time and not scale well). It will then validate that the rows actually contain data. I'm sure when you created your schema that some fields you used the "NOT NULL" restriction to ensure they had data.

COUNT(1) doesn't care if the rows contain data or not, only that they exist.

Since your code only requires that at least one row exists, you could take it further:

SELECT COUNT(1) FROM tempinvoiceentries LIMIT 1

SELECT COUNT(1) FROM tempinvoiceentries LIMIT 1

To copy to clipboard, switch view to plain text mode 

This will always take the same amount of time to execute regardless of how many entries are in your table.

[homerun4711]

Thanks for the good information, I will rewrite some stuff now. I have a feeling that CREATE PROCEDURE will become a good friend of mine