ENH/BUG: Code Clean Up, Optimization, Documentation

avogadro/calc/chargemanager.cpp

@@ -7,9 +7,6 @@
 #include "chargemodel.h"
 #include "defaultmodel.h"
 
-#include <algorithm>
-#include <memory>
-
 namespace Avogadro::Calc {
 
 ChargeManager& ChargeManager::instance()
@@ -100,7 +97,7 @@ std::set<std::string> ChargeManager::identifiersForMolecule(
   std::set<std::string> identifiers = molecule.partialChargeTypes();
 
   // check our models for compatibility
-  for (auto m_model : m_models) {
+  for (auto* m_model : m_models) {
     // We check that every element in the molecule
     // is handled by the model
     auto mask = m_model->elements() & molecule.elements();

avogadro/calc/chargemanager.h

@@ -124,7 +124,7 @@ class AVOGADROCALC_EXPORT ChargeManager
   std::string error() const;
 
 private:
-  typedef std::map<std::string, size_t> ChargeIdMap;
+  using ChargeIdMap = std::map<std::string, size_t>;
 
   ChargeManager();
   ~ChargeManager();

avogadro/calc/chargemodel.cpp

@@ -19,10 +19,6 @@ namespace Calc {
 constexpr double M_PI = 3.14159265358979323846;
 #endif
 
-ChargeModel::ChargeModel() : m_dielectric(1.0) {}
-
-ChargeModel::~ChargeModel() {}
-
 double ChargeModel::potential(Molecule& mol, const Vector3& point) const
 {
   // default is to get the set of partial atomic charges

avogadro/calc/chargemodel.h

@@ -14,11 +14,8 @@
 #include <avogadro/core/vector.h>
 
 #include <string>
-#include <vector>
 
-namespace Avogadro {
-
-namespace Calc {
+namespace Avogadro::Calc {
 
 /**
  * @class ChargeModel chargemodel.h <avogadro/calc/chargemodel.h>
@@ -41,8 +38,8 @@ namespace Calc {
 class AVOGADROCALC_EXPORT ChargeModel
 {
 public:
-  ChargeModel();
-  virtual ~ChargeModel();
+  ChargeModel() = default;
+  virtual ~ChargeModel() = default;
 
   /**
    * Create a new instance of the model. Ownership passes to the
@@ -73,7 +70,7 @@ class AVOGADROCALC_EXPORT ChargeModel
    * Set the dielectric constant for the model.
    * @param dielectric constant.
    */
-  virtual void setDielectric(double dielectric) { m_dielectric = dielectric; };
+  virtual void setDielectric(float dielectric) { m_dielectric = dielectric; };
 
   /**
    * @return The dielectric constant.
@@ -114,10 +111,9 @@ class AVOGADROCALC_EXPORT ChargeModel
 private:
   mutable std::string m_error;
 
-  float m_dielectric;
+  float m_dielectric = 1.0f;
 };
 
-} // namespace Calc
-} // namespace Avogadro
+} // namespace Avogadro::Calc
 
 #endif // AVOGADRO_CALC_CHARGEMODEL_H

avogadro/calc/defaultmodel.cpp

@@ -8,27 +8,20 @@
 #include <avogadro/core/array.h>
 #include <avogadro/core/molecule.h>
 
-namespace Avogadro {
+namespace Avogadro::Calc {
 
-using Core::Molecule;
-
-namespace Calc {
-
-DefaultModel::DefaultModel(const std::string& id)
-  : ChargeModel(), m_identifier(id)
+// Base class constructors are called automatically
+DefaultModel::DefaultModel(const std::string& id) : m_identifier(id)
 {
   // we don't know which elements are in the molecule
   // but we can just say all of them are okay
   // (because this method should work for any molecule)
   m_elements.set();
 }
 
-DefaultModel::~DefaultModel() {}
-
 MatrixX DefaultModel::partialCharges(Core::Molecule& mol) const
 {
   return mol.partialCharges(m_identifier);
 }
 
-} // namespace Calc
-} // namespace Avogadro
+} // namespace Avogadro::Calc

avogadro/calc/defaultmodel.h

@@ -33,7 +33,7 @@ class AVOGADROCALC_EXPORT DefaultModel : public ChargeModel
 {
 public:
   DefaultModel(const std::string& identifier = "");
-  virtual ~DefaultModel();
+  virtual ~DefaultModel() = default;
 
   /**
    * Create a new instance of the file format class. Ownership passes to the

avogadro/calc/energycalculator.h

@@ -24,8 +24,8 @@ namespace Calc {
 class AVOGADROCALC_EXPORT EnergyCalculator : public cppoptlib::Problem<Real>
 {
 public:
-  EnergyCalculator() {}
-  ~EnergyCalculator() {}
+  EnergyCalculator() = default;
+  ~EnergyCalculator() = default;
 
   /**
    * Create a new instance of the model. Ownership passes to the

avogadro/calc/energymanager.cpp

@@ -7,9 +7,6 @@
 #include "energycalculator.h"
 #include "lennardjones.h"
 
-#include <algorithm>
-#include <memory>
-
 namespace Avogadro::Calc {
 
 EnergyManager& EnergyManager::instance()
@@ -112,7 +109,7 @@ std::set<std::string> EnergyManager::identifiersForMolecule(
   std::set<std::string> identifiers;
 
   // check our models for compatibility
-  for (auto m_model : m_models) {
+  for (auto* m_model : m_models) {
     if (m_model == nullptr)
       continue;
 

avogadro/calc/energymanager.h

@@ -116,7 +116,7 @@ class AVOGADROCALC_EXPORT EnergyManager
   std::string error() const;
 
 private:
-  typedef std::map<std::string, size_t> ModelIdMap;
+  using ModelIdMap = std::map<std::string, size_t>;
 
   EnergyManager();
   ~EnergyManager();

avogadro/calc/lennardjones.cpp

@@ -21,8 +21,6 @@ LennardJones::LennardJones()
   }
 }
 
-LennardJones::~LennardJones() {}
-
 void LennardJones::setMolecule(Core::Molecule* mol)
 {
   m_molecule = mol;

avogadro/calc/lennardjones.h

@@ -22,7 +22,7 @@ class AVOGADROCALC_EXPORT LennardJones : public EnergyCalculator
 {
 public:
   LennardJones();
-  ~LennardJones();
+  ~LennardJones() = default;
 
   LennardJones* newInstance() const override { return new LennardJones; }
 

avogadro/core/angleiterator.cpp

@@ -8,12 +8,8 @@
 #include <avogadro/core/graph.h>
 #include <avogadro/core/molecule.h>
 
-#include <iostream>
-
 namespace Avogadro::Core {
 
-using namespace std;
-
 AngleIterator::AngleIterator(const Molecule* mol)
   : m_current(0, 0, 0), m_mol(mol)
 {}
@@ -26,7 +22,7 @@ Angle AngleIterator::begin()
 Angle AngleIterator::operator++()
 {
   if (m_mol == nullptr)
-    return make_tuple(MaxIndex, MaxIndex, MaxIndex);
+    return std::make_tuple(MaxIndex, MaxIndex, MaxIndex);
 
   Index a, b, c;
   std::tie(a, b, c) = m_current;
@@ -45,7 +41,7 @@ Angle AngleIterator::operator++()
         // we have a valid current angle, try to find a new edge
         for (const auto maybeC : graph.neighbors(b)) {
           if (maybeC != a && maybeC > c) {
-            m_current = make_tuple(a, b, maybeC);
+            m_current = std::make_tuple(a, b, maybeC);
             return m_current;
           }
 
@@ -80,7 +76,7 @@ Angle AngleIterator::operator++()
   } while (valid && b < count);
 
   // can't find anything
-  return make_tuple(MaxIndex, MaxIndex, MaxIndex);
+  return std::make_tuple(MaxIndex, MaxIndex, MaxIndex);
 } // end ++ operator
 
 } // namespace Avogadro

avogadro/core/angleiterator.h

@@ -10,15 +10,13 @@
 
 #include "avogadrocore.h"
 
-#include <vector>
 #include <tuple>
 
-namespace Avogadro {
-namespace Core {
+namespace Avogadro::Core {
 
 class Molecule;
 
-typedef std::tuple<Index, Index, Index> Angle;
+using Angle = std::tuple<Index, Index, Index>;
 
 class AVOGADROCORE_EXPORT AngleIterator
 {
@@ -37,7 +35,8 @@ class AVOGADROCORE_EXPORT AngleIterator
   Angle begin();
 
   Angle end() const {
-    return std::make_tuple(MaxIndex, MaxIndex, MaxIndex);
+    return std::make_tuple(Avogadro::MaxIndex, Avogadro::MaxIndex,
+                           Avogadro::MaxIndex);
   }
 
   Angle operator++();
@@ -47,11 +46,10 @@ class AVOGADROCORE_EXPORT AngleIterator
   }
 
 private:
-  Angle     m_current;
+  Angle m_current;
   const Molecule* m_mol;
 };
 
-} // namespace Core
-} // namespace Avogadro
+} // namespace Avogadro::Core
 
 #endif // AVOGADRO_CORE_ANGLEITERATOR_H

avogadro/core/array.h

@@ -11,8 +11,7 @@
 #include <algorithm>
 #include <vector>
 
-namespace Avogadro {
-namespace Core {
+namespace Avogadro::Core {
 
 using std::size_t;
 
@@ -22,22 +21,22 @@ template <typename T>
 class ArrayRefContainer
 {
 public:
-  typedef T ValueType;
-  typedef std::vector<T> Parent; // The parent container for iterators etc
+  using ValueType = T;
+  using Parent = std::vector<T>; // The parent container for iterators etc
 
   // STL compatibility, forward typedefs from std::vector:
-  typedef typename Parent::value_type value_type;
-  typedef typename Parent::allocator_type allocator_type;
-  typedef typename Parent::reference reference;
-  typedef typename Parent::const_reference const_reference;
-  typedef typename Parent::pointer pointer;
-  typedef typename Parent::const_pointer const_pointer;
-  typedef typename Parent::iterator iterator;
-  typedef typename Parent::const_iterator const_iterator;
-  typedef typename Parent::reverse_iterator reverse_iterator;
-  typedef typename Parent::const_reverse_iterator const_reverse_iterator;
-  typedef typename Parent::difference_type difference_type;
-  typedef typename Parent::size_type size_type;
+  using value_type = typename Parent::value_type;
+  using allocator_type = typename Parent::allocator_type;
+  using reference = typename Parent::reference;
+  using const_reference = typename Parent::const_reference;
+  using pointer = typename Parent::pointer;
+  using const_pointer = typename Parent::const_pointer;
+  using iterator = typename Parent::iterator;
+  using const_iterator = typename Parent::const_iterator;
+  using reverse_iterator = typename Parent::reverse_iterator;
+  using const_reverse_iterator = typename Parent::const_reverse_iterator;
+  using difference_type = typename Parent::difference_type;
+  using size_type = typename Parent::size_type;
 
   ArrayRefContainer() : m_ref(1), data() {}
 
@@ -93,24 +92,22 @@ template <typename T>
 class Array
 {
 public:
-  typedef internal::ArrayRefContainer<T> Container;
-
-public:
-  typedef T ValueType;
+  using Container = internal::ArrayRefContainer<T>;
+  using ValueType = T;
 
   /** Typedefs for STL compatibility @{ */
-  typedef typename Container::value_type value_type;
-  typedef typename Container::allocator_type allocator_type;
-  typedef typename Container::reference reference;
-  typedef typename Container::const_reference const_reference;
-  typedef typename Container::pointer pointer;
-  typedef typename Container::const_pointer const_pointer;
-  typedef typename Container::iterator iterator;
-  typedef typename Container::const_iterator const_iterator;
-  typedef typename Container::reverse_iterator reverse_iterator;
-  typedef typename Container::const_reverse_iterator const_reverse_iterator;
-  typedef typename Container::difference_type difference_type;
-  typedef typename Container::size_type size_type;
+  using value_type = typename Container::value_type;
+  using allocator_type = typename Container::allocator_type;
+  using reference = typename Container::reference;
+  using const_reference = typename Container::const_reference;
+  using pointer = typename Container::pointer;
+  using const_pointer = typename Container::const_pointer;
+  using iterator = typename Container::iterator;
+  using const_iterator = typename Container::const_iterator;
+  using reverse_iterator = typename Container::reverse_iterator;
+  using const_reverse_iterator = typename Container::const_reverse_iterator;
+  using difference_type = typename Container::difference_type;
+  using size_type = typename Container::size_type;
   /** @} */
 
   /** Constructors for new containers. */
@@ -362,7 +359,7 @@ template <typename T>
 inline void Array<T>::detachWithCopy()
 {
   if (d && d->ref() != 1) {
-    Container* o = new Container(*d);
+    auto* o = new Container(*d);
     d->deref();
     d = o;
   }
@@ -421,7 +418,6 @@ inline void swap(Array<T>& lhs, Array<T>& rhs)
   lhs.swap(rhs);
 }
 
-} // namespace Core
-} // namespace Avogadro
+} // namespace Avogadro::Core
 
 #endif // AVOGADRO_CORE_ARRAY_H

avogadro/core/atom.h

@@ -9,8 +9,7 @@
 #include "avogadrocore.h"
 #include "vector.h"
 
-namespace Avogadro {
-namespace Core {
+namespace Avogadro::Core {
 
 enum AtomHybridization
 {
@@ -47,7 +46,7 @@ template <class Molecule_T>
 class AtomTemplate
 {
 public:
-  typedef Molecule_T MoleculeType;
+  using MoleculeType = Molecule_T;
 
   /** Creates a new, invalid atom object. */
   AtomTemplate();
@@ -389,7 +388,6 @@ std::string AtomTemplate<Molecule_T>::label() const
   return m_molecule->atomLabel(m_index);
 }
 
-} // namespace Core
-} // namespace Avogadro
+} // namespace Avogadro::Core
 
 #endif // AVOGADRO_CORE_ATOM_H