C++ Readability review for holte

components/rappor/rappor_service.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Change for readability
+
 #include "components/rappor/rappor_service.h"
 
 #include "base/base64.h"
 #include "base/metrics/field_trial.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "components/metrics/metrics_hashes.h"
 #include "components/rappor/proto/rappor_metric.pb.h"
@@ skipping 20 matching lines @@
 // Constant for the finch parameter name for the server URL
 const char kRapporRolloutServerUrlParam[] = "ServerUrl";
 
 GURL GetServerUrl() {
   return GURL(chrome_variations::GetVariationParamValue(
       kRapporRolloutFieldTrialName,
       kRapporRolloutServerUrlParam));
 }
 
 const RapporParameters kRapporParametersForType[NUM_RAPPOR_TYPES] = {
-    {  // ETLD_PLUS_ONE_RAPPOR_TYPE
-        16 /* Bloom filter size bytes */,
-        2 /* Bloom filter hash count */,
-        rappor::PROBABILITY_75 /* Fake data probability */,
-        rappor::PROBABILITY_50 /* Fake one probability */,
-        rappor::PROBABILITY_75 /* One coin probability */,
-        rappor::PROBABILITY_50 /* Zero coin probability */
+    {// ETLD_PLUS_ONE_RAPPOR_TYPE
+     16 /* Bloom filter size bytes */,
+     2 /* Bloom filter hash count */,
+     rappor::PROBABILITY_75 /* Fake data probability */,
+     rappor::PROBABILITY_50 /* Fake one probability */,
+     rappor::PROBABILITY_75 /* One coin probability */,
+     rappor::PROBABILITY_50 /* Zero coin probability */
     },
 };
 
 }  // namespace
 
 RapporService::RapporService() : cohort_(-1) {}
 
 RapporService::~RapporService() {
   STLDeleteValues(&metrics_map_);
 }
@@ skipping 29 matching lines @@
                             &RapporService::OnLogInterval);
 }
 
 // static
 void RapporService::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterStringPref(prefs::kRapporSecret, std::string());
   registry->RegisterIntegerPref(prefs::kRapporCohort, -1);
 }
 
 void RapporService::LoadCohort(PrefService* pref_service) {
-  DCHECK_EQ(cohort_, -1);
+  DCHECK(!IsInitialized());
   cohort_ = pref_service->GetInteger(prefs::kRapporCohort);
+  // If the user is already assigned to a valid cohort, we're done.
   if (cohort_ >= 0 && cohort_ < kNumCohorts)
     return;
 
+  // This is the first time the client has started the service (or thier

[ktl, 2014/03/18 08:35:01]

Spelling: their

[Steven Holte, 2014/03/19 23:46:55]

Done.

+  // preferences were corrupted).  Randomly assign them to a cohort.
   cohort_ = base::RandGenerator(kNumCohorts);
   pref_service->SetInteger(prefs::kRapporCohort, cohort_);
 }
 
 void RapporService::LoadSecret(PrefService* pref_service) {
   DCHECK(secret_.empty());
   std::string secret_base64 =

[ktl, 2014/03/18 08:35:01]

This fits on one line.

[Steven Holte, 2014/03/19 23:46:55]

Done.

       pref_service->GetString(prefs::kRapporSecret);
   if (!secret_base64.empty()) {
     bool decoded = base::Base64Decode(secret_base64, &secret_);
     if (decoded && secret_.size() == HmacByteVectorGenerator::kEntropyInputSize)
       return;
     // If the preference fails to decode, or is the wrong size, it must be
     // corrupt, so continue as though it didn't exist yet and generate a new
     // one.
   }
 
   secret_ = HmacByteVectorGenerator::GenerateEntropyInput();
   base::Base64Encode(secret_, &secret_base64);
   pref_service->SetString(prefs::kRapporSecret, secret_base64);
 }
 
 bool RapporService::ExportMetrics(RapporReports* reports) {
   if (metrics_map_.empty())
     return false;
 
   DCHECK_GE(cohort_, 0);
   reports->set_cohort(cohort_);
 
-  for (std::map<std::string, RapporMetric*>::iterator it = metrics_map_.begin();
+  for (std::map<std::string, RapporMetric*>::const_iterator it =
+          metrics_map_.begin();

[ktl, 2014/03/18 08:35:01]

This would need one more space; the four space indentation is counted from the
left parenthesis. (Clang-format also does this.)

[Steven Holte, 2014/03/19 23:46:55]

Done.

        metrics_map_.end() != it;

[ktl, 2014/03/18 08:35:01]

I've not seen this style (putting the variable iterator to the right in
comparison) before, is this also project-specific?

Note that Chromium style guide prefers foo == 0 to 0 == foo.

[Steven Holte, 2014/03/19 23:46:55]

base/metrics/... seems to use that style in a few places, but I think that is
not the norm in chrome code.  Fixed.

        ++it) {
     const RapporMetric* metric = it->second;
     RapporReports::Report* report = reports->add_report();
     report->set_name_hash(metrics::HashMetricName(it->first));
     ByteVector bytes = metric->GetReport(secret_);
     report->set_bits(std::string(bytes.begin(), bytes.end()));
   }
   STLDeleteValues(&metrics_map_);
   return true;
 }
 
 bool RapporService::IsInitialized() const {
   return cohort_ >= 0;
 }
 
 void RapporService::RecordSample(const std::string& metric_name,
                                  RapporType type,
                                  const std::string& sample) {
   // Ignore the sample if the service hasn't started yet.
   if (!IsInitialized())
     return;
   DCHECK_LT(type, NUM_RAPPOR_TYPES);
   RecordSampleInternal(metric_name, kRapporParametersForType[type], sample);
 }
 
 void RapporService::RecordSampleInternal(const std::string& metric_name,
                                          const RapporParameters& parameters,
                                          const std::string& sample) {
   DCHECK(IsInitialized());
-
   RapporMetric* metric = LookUpMetric(metric_name, parameters);
   metric->AddSample(sample);
 }
 
 RapporMetric* RapporService::LookUpMetric(const std::string& metric_name,
                                           const RapporParameters& parameters) {
   DCHECK(IsInitialized());
-  std::map<std::string, RapporMetric*>::iterator it =
+  std::map<std::string, RapporMetric*>::const_iterator it =
       metrics_map_.find(metric_name);
   if (metrics_map_.end() != it) {
     RapporMetric* metric = it->second;
     DCHECK_EQ(parameters.ToString(), metric->parameters().ToString());
     return metric;
   }
 
   RapporMetric* new_metric = new RapporMetric(metric_name, parameters, cohort_);
   metrics_map_[metric_name] = new_metric;
   return new_metric;
 }
 
 }  // namespace rappor