QSqlTM/QSqlRTM: improve style and readability

General changes:
const, scope, braces, hash[] for clarity, comment wording and
spelling.

QSqlRelationalTableModel::selectStatement() readability:

Renamed private method.

QVector<Class>.value() already defaults to null object value, so there
is no point in handling this case explicitly.

Alias rec for d->rec added more noise than clarity.

Using "tables" list only adds an extra step. Simple concatenation does
the trick.

Deduplicate code for building table expression and JOIN condition.

Change-Id: Ia52afaf3c3937a26595d5ae867982664002562d8
Reviewed-by: Honglei Zhang <honglei.zhang@nokia.com>

diff --git a/src/sql/models/qsqlrelationaltablemodel.cpp b/src/sql/models/qsqlrelationaltablemodel.cpp
index 6744e9d..b5d98d5 100644 (file)
--- a/src/sql/models/qsqlrelationaltablemodel.cpp
+++ b/src/sql/models/qsqlrelationaltablemodel.cpp
@@ -259,7 +259,7 @@ public:
         : QSqlTableModelPrivate(),
         joinMode( QSqlRelationalTableModel::InnerJoin )
     {}
-    QString relationField(const QString &tableName, const QString &fieldName) const;
+    QString fullyQualifiedFieldName(const QString &tableName, const QString &fieldName) const;
 
     int nameToIndex(const QString &name) const;
     mutable QVector<QRelation> relations;
@@ -299,7 +299,7 @@ void QSqlRelationalTableModelPrivate::revertCachedRow(int row)
 
 int QSqlRelationalTableModelPrivate::nameToIndex(const QString &name) const
 {
-    QString fieldname = strippedFieldName(name);
+    const QString fieldname = strippedFieldName(name);
     int idx = baseRec.indexOf(fieldname);
     if (idx == -1) {
         // If the name is an alias we can find it here.
@@ -520,8 +520,8 @@ QSqlRelation QSqlRelationalTableModel::relation(int column) const
     return d->relations.value(column).rel;
 }
 
-QString QSqlRelationalTableModelPrivate::relationField(const QString &tableName,
-                        const QString &fieldName) const
+QString QSqlRelationalTableModelPrivate::fullyQualifiedFieldName(const QString &tableName,
+                                                                 const QString &fieldName) const
 {
     QString ret;
     ret.reserve(tableName.size() + fieldName.size() + 1);
@@ -536,35 +536,25 @@ QString QSqlRelationalTableModelPrivate::relationField(const QString &tableName,
 QString QSqlRelationalTableModel::selectStatement() const
 {
     Q_D(const QSqlRelationalTableModel);
-    QString query;
 
     if (tableName().isEmpty())
-        return query;
+        return QString();
     if (d->relations.isEmpty())
         return QSqlTableModel::selectStatement();
 
-    QString tList;
-    QString fList;
-    QString where;
-
-    QSqlRecord rec = d->baseRec;
-    QStringList tables;
-    const QRelation nullRelation;
-
     // Count how many times each field name occurs in the record
     QHash<QString, int> fieldNames;
     QStringList fieldList;
-    for (int i = 0; i < rec.count(); ++i) {
-        QSqlRelation relation = d->relations.value(i, nullRelation).rel;
+    for (int i = 0; i < d->baseRec.count(); ++i) {
+        QSqlRelation relation = d->relations.value(i).rel;
         QString name;
-        if (relation.isValid())
-        {
+        if (relation.isValid()) {
             // Count the display column name, not the original foreign key
             name = relation.displayColumn();
             if (d->db.driver()->isIdentifierEscaped(name, QSqlDriver::FieldName))
                 name = d->db.driver()->stripDelimiters(name, QSqlDriver::FieldName);
 
-            QSqlRecord rec = database().record(relation.tableName());
+            const QSqlRecord rec = database().record(relation.tableName());
             for (int i = 0; i < rec.count(); ++i) {
                 if (name.compare(rec.fieldName(i), Qt::CaseInsensitive) == 0) {
                     name = rec.fieldName(i);
@@ -572,21 +562,24 @@ QString QSqlRelationalTableModel::selectStatement() const
                 }
             }
         }
-        else
-            name = rec.fieldName(i);
-        fieldNames.insert(name, fieldNames.value(name, 0) + 1);
+        else {
+            name = d->baseRec.fieldName(i);
+        }
+        fieldNames[name] = fieldNames.value(name, 0) + 1;
         fieldList.append(name);
     }
 
-    for (int i = 0; i < rec.count(); ++i) {
-        QSqlRelation relation = d->relations.value(i, nullRelation).rel;
+    QString fList;
+    QString conditions;
+    QString from = tableName();
+    for (int i = 0; i < d->baseRec.count(); ++i) {
+        QSqlRelation relation = d->relations.value(i).rel;
+        const QString tableField = d->fullyQualifiedFieldName(tableName(), d->db.driver()->escapeIdentifier(d->baseRec.fieldName(i), QSqlDriver::FieldName));
         if (relation.isValid()) {
-            QString relTableAlias = QString::fromLatin1("relTblAl_%1").arg(i);
-            if (!fList.isEmpty())
-                fList.append(QLatin1String(", "));
-            fList.append(d->relationField(relTableAlias, relation.displayColumn()));
+            const QString relTableAlias = QString::fromLatin1("relTblAl_%1").arg(i);
+            QString displayTableField = d->fullyQualifiedFieldName(relTableAlias, relation.displayColumn());
 
-            // If there are duplicate field names they must be aliased
+            // Duplicate field names must be aliased
             if (fieldNames.value(fieldList[i]) > 1) {
                 QString relTableName = relation.tableName().section(QChar::fromLatin1('.'), -1, -1);
                 if (d->db.driver()->isIdentifierEscaped(relTableName, QSqlDriver::TableName))
@@ -594,54 +587,44 @@ QString QSqlRelationalTableModel::selectStatement() const
                 QString displayColumn = relation.displayColumn();
                 if (d->db.driver()->isIdentifierEscaped(displayColumn, QSqlDriver::FieldName))
                     displayColumn = d->db.driver()->stripDelimiters(displayColumn, QSqlDriver::FieldName);
-                fList.append(QString::fromLatin1(" AS %1_%2_%3").arg(relTableName).arg(displayColumn).arg(fieldNames.value(fieldList[i])));
-                fieldNames.insert(fieldList[i], fieldNames.value(fieldList[i])-1);
+                displayTableField.append(QString::fromLatin1(" AS %1_%2_%3").arg(relTableName).arg(displayColumn).arg(fieldNames.value(fieldList[i])));
+                --fieldNames[fieldList[i]];
             }
 
+            if (!fList.isEmpty())
+                fList.append(QLatin1String(", "));
+            fList.append(displayTableField);
+
+            // Join related table
+            const QString tblexpr = relation.tableName().append(QLatin1Char(' ')).append(relTableAlias);
+            const QString relTableField = d->fullyQualifiedFieldName(relTableAlias, relation.indexColumn());
+            const QString cond = tableField + QLatin1String(" = ") + relTableField;
             if (d->joinMode == QSqlRelationalTableModel::InnerJoin) {
-                // this needs fixing!! the below if is borken.
-                // Use LeftJoin mode if you want correct behavior
-                tables.append(relation.tableName().append(QLatin1Char(' ')).append(relTableAlias));
-                if (!where.isEmpty())
-                    where.append(QLatin1String(" AND "));
-                where.append(d->relationField(tableName(), d->db.driver()->escapeIdentifier(rec.fieldName(i), QSqlDriver::FieldName)));
-                where.append(QLatin1String(" = "));
-                where.append(d->relationField(relTableAlias, relation.indexColumn()));
+                // FIXME: InnerJoin code is known to be broken.
+                // Use LeftJoin mode if you want correct behavior.
+                from.append(QLatin1String(", ")).append(tblexpr);
+                if (!conditions.isEmpty())
+                    conditions.append(QLatin1String(" AND "));
+                conditions.append(cond);
             } else {
-                tables.append(QLatin1String(" LEFT JOIN"));
-                tables.append(relation.tableName().append(QLatin1Char(' ')).append(relTableAlias));
-                tables.append(QLatin1String("ON"));
-
-                QString clause;
-                clause.append(d->relationField(tableName(), d->db.driver()->escapeIdentifier(rec.fieldName(i), QSqlDriver::FieldName)));
-                clause.append(QLatin1String(" = "));
-                clause.append(d->relationField(relTableAlias, relation.indexColumn()));
-
-                tables.append(clause);
+                from.append(QLatin1String(" LEFT JOIN ")).append(tblexpr);
+                from.append(QLatin1String(" ON ")).append(cond);
             }
         } else {
             if (!fList.isEmpty())
                 fList.append(QLatin1String(", "));
-            fList.append(d->relationField(tableName(), d->db.driver()->escapeIdentifier(rec.fieldName(i), QSqlDriver::FieldName)));
+            fList.append(tableField);
         }
     }
 
-    if (d->joinMode == QSqlRelationalTableModel::InnerJoin && !tables.isEmpty()) {
-        tList.append(tables.join(QLatin1String(", ")));
-        if(!tList.isEmpty())
-            tList.prepend(QLatin1String(", "));
-    } else
-        tList.append(tables.join(QLatin1String(" ")));
-
     if (fList.isEmpty())
-        return query;
+        return QString();
 
-    tList.prepend(tableName());
-    query.append(QLatin1String("SELECT "));
-    query.append(fList).append(QLatin1String(" FROM ")).append(tList);
+    QString query = query.append(QLatin1String("SELECT "));
+    query.append(fList).append(QLatin1String(" FROM ")).append(from);
 
     if (d->joinMode == QSqlRelationalTableModel::InnerJoin) {
-        qAppendWhereClause(query, where, filter());
+        qAppendWhereClause(query, conditions, filter());
     } else if (!filter().isEmpty()) {
         query.append(QLatin1String(" WHERE ("));
         query.append(filter());
@@ -795,8 +778,8 @@ QString QSqlRelationalTableModel::orderByClause() const
         return QSqlTableModel::orderByClause();
 
     QString s = QLatin1String("ORDER BY ");
-    s.append(d->relationField(QLatin1String("relTblAl_") + QString::number(d->sortColumn),
-                    rel.displayColumn()));
+    s.append(d->fullyQualifiedFieldName(QLatin1String("relTblAl_") + QString::number(d->sortColumn),
+                                        rel.displayColumn()));
     s += d->sortOrder == Qt::AscendingOrder ? QLatin1String(" ASC") : QLatin1String(" DESC");
     return s;
 }

diff --git a/src/sql/models/qsqltablemodel.cpp b/src/sql/models/qsqltablemodel.cpp
index d8d691f..0e5eba1 100644 (file)
--- a/src/sql/models/qsqltablemodel.cpp
+++ b/src/sql/models/qsqltablemodel.cpp
@@ -84,12 +84,9 @@ int QSqlTableModelPrivate::insertCount(int maxRow) const
     int cnt = 0;
     CacheMap::ConstIterator i = cache.constBegin();
     const CacheMap::ConstIterator e = cache.constEnd();
-    for (;
-         i != e && (maxRow < 0 || i.key() <= maxRow);
-         ++i) {
+    for ( ; i != e && (maxRow < 0 || i.key() <= maxRow); ++i)
         if (i.value().insert())
             ++cnt;
-    }
 
     return cnt;
 }
@@ -175,14 +172,12 @@ bool QSqlTableModelPrivate::exec(const QString &stmt, bool prepStatement,
             }
         }
         int i;
-        for (i = 0; i < rec.count(); ++i) {
+        for (i = 0; i < rec.count(); ++i)
             if (rec.isGenerated(i))
                 editQuery.addBindValue(rec.value(i));
-        }
-        for (i = 0; i < whereValues.count(); ++i) {
+        for (i = 0; i < whereValues.count(); ++i)
             if (whereValues.isGenerated(i) && !whereValues.isNull(i))
                 editQuery.addBindValue(whereValues.value(i));
-        }
 
         if (!editQuery.exec()) {
             error = editQuery.lastError();
@@ -363,7 +358,7 @@ QString QSqlTableModel::tableName() const
 bool QSqlTableModel::select()
 {
     Q_D(QSqlTableModel);
-    QString query = selectStatement();
+    const QString query = selectStatement();
     if (query.isEmpty())
         return false;
 
@@ -593,11 +588,11 @@ bool QSqlTableModel::updateRowInTable(int row, const QSqlRecord &values)
     emit beforeUpdate(row, rec);
 
     const QSqlRecord whereValues = d->primaryValues(row);
-    bool prepStatement = d->db.driver()->hasFeature(QSqlDriver::PreparedQueries);
+    const bool prepStatement = d->db.driver()->hasFeature(QSqlDriver::PreparedQueries);
     QString stmt = d->db.driver()->sqlStatement(QSqlDriver::UpdateStatement, d->tableName,
                                                 rec, prepStatement);
-    QString where = d->db.driver()->sqlStatement(QSqlDriver::WhereStatement, d->tableName,
-                                                 whereValues, prepStatement);
+    const QString where = d->db.driver()->sqlStatement(QSqlDriver::WhereStatement, d->tableName,
+                                                       whereValues, prepStatement);
 
     if (stmt.isEmpty() || where.isEmpty() || row < 0 || row >= rowCount()) {
         d->error = QSqlError(QLatin1String("No Fields to update"), QString(),
@@ -629,9 +624,9 @@ bool QSqlTableModel::insertRowIntoTable(const QSqlRecord &values)
     QSqlRecord rec = values;
     emit beforeInsert(rec);
 
-    bool prepStatement = d->db.driver()->hasFeature(QSqlDriver::PreparedQueries);
-    QString stmt = d->db.driver()->sqlStatement(QSqlDriver::InsertStatement, d->tableName,
-                                                rec, prepStatement);
+    const bool prepStatement = d->db.driver()->hasFeature(QSqlDriver::PreparedQueries);
+    const QString stmt = d->db.driver()->sqlStatement(QSqlDriver::InsertStatement, d->tableName,
+                                                      rec, prepStatement);
 
     if (stmt.isEmpty()) {
         d->error = QSqlError(QLatin1String("No Fields to update"), QString(),
@@ -660,15 +655,15 @@ bool QSqlTableModel::deleteRowFromTable(int row)
     emit beforeDelete(row);
 
     const QSqlRecord whereValues = d->primaryValues(row);
-    bool prepStatement = d->db.driver()->hasFeature(QSqlDriver::PreparedQueries);
+    const bool prepStatement = d->db.driver()->hasFeature(QSqlDriver::PreparedQueries);
     QString stmt = d->db.driver()->sqlStatement(QSqlDriver::DeleteStatement,
                                                 d->tableName,
                                                 QSqlRecord(),
                                                 prepStatement);
-    QString where = d->db.driver()->sqlStatement(QSqlDriver::WhereStatement,
-                                                 d->tableName,
-                                                 whereValues,
-                                                 prepStatement);
+    const QString where = d->db.driver()->sqlStatement(QSqlDriver::WhereStatement,
+                                                       d->tableName,
+                                                       whereValues,
+                                                       prepStatement);
 
     if (stmt.isEmpty() || where.isEmpty()) {
         d->error = QSqlError(QLatin1String("Unable to delete row"), QString(),
@@ -838,9 +833,8 @@ void QSqlTableModel::revertAll()
     Q_D(QSqlTableModel);
 
     const QList<int> rows(d->cache.keys());
-    for (int i = rows.size() - 1; i >= 0; --i) {
+    for (int i = rows.size() - 1; i >= 0; --i)
         revertRow(rows.value(i));
-    }
 }
 
 /*!