[rac] Validate an address.

third_party/libaddressinput/chromium/cpp/src/address_validator.cc

Index: third_party/libaddressinput/chromium/cpp/src/address_validator.cc
diff --git a/third_party/libaddressinput/chromium/cpp/src/address_validator.cc b/third_party/libaddressinput/chromium/cpp/src/address_validator.cc
index 6300ef2741888da0bd62e69bf2463ce55b26e968..98a5c094b9fb7c3f4082a47cea2de2416c44a8f2 100644
--- a/third_party/libaddressinput/chromium/cpp/src/address_validator.cc
+++ b/third_party/libaddressinput/chromium/cpp/src/address_validator.cc
@@ -14,6 +14,7 @@
 
 #include <libaddressinput/address_validator.h>
 
+#include <libaddressinput/address_data.h>
 #include <libaddressinput/downloader.h>
 #include <libaddressinput/load_rules_delegate.h>
 #include <libaddressinput/localization.h>
@@ -21,7 +22,9 @@
 #include <libaddressinput/util/basictypes.h>
 #include <libaddressinput/util/scoped_ptr.h>
 
+#include <algorithm>
 #include <cassert>
+#include <cstddef>
 #include <map>
 #include <string>
 #include <utility>
@@ -29,6 +32,7 @@
 #include "country_rules_retriever.h"
 #include "lookup_key.h"
 #include "retriever.h"
+#include "rule.h"
 #include "rule_retriever.h"
 #include "util/stl_util.h"
 #include "validating_storage.h"
@@ -38,6 +42,59 @@ namespace addressinput {
 
 namespace {
 
+// Returns the next smaller size field type for |field|. For example, returns
+// ADMIN_AREA for COUNTRY. The |field| can only be COUNTRY, ADMIN_AREA, and
+// LOCALITY.
+AddressField GetSubField(AddressField field) {
+  switch (field) {
+    case COUNTRY:
+      return ADMIN_AREA;

[Evan Stade, 2013/12/19 02:12:38]

you've now encoded this ordering twice in one CL.

[please use gerrit instead, 2014/01/08 00:55:52]

Fixed.

+    case ADMIN_AREA:
+      return LOCALITY;
+    case LOCALITY:
+      return DEPENDENT_LOCALITY;
+    default:
+      assert(false);
+      return field;
+  }
+}
+
+// Returns the |field| value from |address|. The |field| can be only ADMIN_AREA,
+// LOCALITY, and DEPENDENT_LOCALITY.
+const std::string& GetValue(const AddressData& address, AddressField field) {

[Evan Stade, 2013/12/19 02:12:38]

didn't I read this function elsewhere in this CL?

[please use gerrit instead, 2014/01/08 00:55:52]

Moved to AddressData.

+  switch (field) {
+    case ADMIN_AREA:
+      return address.administrative_area;
+    case LOCALITY:
+      return address.locality;
+    case DEPENDENT_LOCALITY:
+      return address.dependent_locality;
+    default:
+      assert(false);
+      static const std::string kEmpty;
+      return kEmpty;
+  }
+}
+
+// Returns true if |filter| allows the |field| to have a |problem_type|.
+bool FilterAllows(const AddressProblemFilter& filter,
+                  AddressField field,
+                  AddressProblem::Type problem_type) {
+  if (filter.empty()) {
+    return true;
+  }
+  std::pair<AddressProblemFilter::const_iterator,
+            AddressProblemFilter::const_iterator> range =
+                filter.equal_range(field);

[Evan Stade, 2013/12/19 02:12:38]

while this is interesting, I think the following is easier to read because it's
more idiomatic (and has no worse time complexity):

for (AddressProblemFilter::const_iterator it = filter.begin();
     it != filter.end(); ++it) {
  if (it->first == field && it->second == problem_type) {
    return true;
  }
}

[please use gerrit instead, 2014/01/08 00:55:52]

Done.

+  for (AddressProblemFilter::const_iterator it = range.first;
+       it != range.second; ++it) {
+    if (it->second == problem_type) {
+      return true;
+    }
+  }
+  return false;
+}
+
 // Contains all rules loaded for a country code.
 struct CountryRules {
   CountryRules(const std::string& country_code,
@@ -107,7 +164,39 @@ class AddressValidatorImpl : public AddressValidator {
       const AddressProblemFilter& filter,
       const Localization& localization,
       AddressProblems* problems) const {
-    return RULES_UNAVAILABLE;
+    CountryRulesMap::const_iterator country_rule_it =
+        rules_.find(address.country_code);
+    if (country_rule_it == rules_.end()) {
+      return RULES_UNAVAILABLE;
+    }
+    assert(country_rule_it->second != NULL);
+    if (country_rule_it->second->status != SUCCESS) {
+      return country_rule_it->second->status;
+    }
+    std::map<AddressField, const Rule*> field_rule_map =

[Evan Stade, 2013/12/19 02:12:38]

this function nee4ds some vertical whitespace somewhere.

[please use gerrit instead, 2014/01/08 00:55:52]

Done.

+        country_rule_it->second->root.BuildFieldRuleMap(address);
+    for (std::map<AddressField, const Rule*>::const_iterator
+         field_rule_it = field_rule_map.begin();

[Evan Stade, 2013/12/19 02:12:38]

indent by 4 more

[please use gerrit instead, 2014/01/08 00:55:52]

The rest of libaddressinput uses this format when "it = collection.begin()" does
not fit on the first line of the for-loop:

for (CollectionType::const_iterator
     it = collection.begin();
     it != collection.end();
     ++it) {
  ...
}

I think this makes the loop easier to reed. WYT?

[Evan Stade, 2014/01/08 21:29:34]

Why do you think it makes it easier to read? I think it makes it harder because
it's less obvious that the second line is a continuation of the first.

[please use gerrit instead, 2014/01/09 19:42:00]

Done.

+         field_rule_it != field_rule_map.end();
+         ++field_rule_it) {
+      AddressField field = field_rule_it->first;
+      const Rule& rule = *field_rule_it->second;
+      if (field < DEPENDENT_LOCALITY) {

[Evan Stade, 2013/12/19 02:12:38]

nit: invert this check and continue;, that keeps the total indentation down for
the rest of the function (indentation roughly aligns with how much context one
has to remember when reading code)

[please use gerrit instead, 2014/01/08 00:55:52]

No longer applicable.

+        AddressField sub_field = GetSubField(field);
+        const std::string& sub_value = GetValue(address, sub_field);
+        if (!rule.GetSubKeys().empty() && !sub_value.empty() &&

[Evan Stade, 2013/12/19 02:12:38]

nit: easier to read with one condition per line (when you are wrapping any
conditions, wrap all conditions)

[please use gerrit instead, 2014/01/08 00:55:52]

Done.

+            std::find(rule.GetSubKeys().begin(),
+                      rule.GetSubKeys().end(),
+                      sub_value) == rule.GetSubKeys().end() &&
+            FilterAllows(filter, sub_field, AddressProblem::UNKNOWN_VALUE)) {
+          AddressProblem problem = {sub_field,
+                                    AddressProblem::UNKNOWN_VALUE,
+                                    std::string()};
+          problems->push_back(problem);
+        }
+      }
+    }
+    return SUCCESS;
   }
 
  private: