Fix review findings

Author: wichern

libs/s25main/ai/aijh/AIConstruction.cpp

@@ -466,7 +466,7 @@ helpers::OptionalEnum<BuildingType> AIConstruction::ChooseMilitaryBuilding(const
     const BuildingType biggestBld = GetBiggestAllowedMilBuilding().value();
 
     const Inventory& inventory = aii.GetInventory();
-    if((inventory.people[Job::Private] < 15 || AI::random())
+    if((inventory.people[Job::Private] < 15 || AI::random(3))
        && (inventory.goods[GoodType::Stones] > 6 || bldPlanner.GetNumBuildings(BuildingType::Quarry) > 0))
         bld = BuildingType::Guardhouse;
     if(aijh.getAIInterface().isHarborPosClose(pt, 19) && !AI::random(9) && aijh.ggs.isEnabled(AddonId::SEA_ATTACK))

libs/s25main/ai/aijh/AIPlayerJH.cpp

@@ -344,9 +344,8 @@ void AIPlayerJH::PlanNewBuildings(const unsigned gf)
         DistributeGoodsByBlocking(GoodType::Boards, 30);
         DistributeGoodsByBlocking(GoodType::Stones, 50);
         // go to the picked random warehouse and try to build around it
-        auto it = storehouses.begin();
-        std::advance(it, AI::randomIndex(storehouses));
-        const MapPoint whPos = (*it)->GetPos();
+        const auto* storehouse = AI::randomElement(storehouses);
+        const MapPoint whPos = storehouse->GetPos();
         UpdateNodesAround(whPos, 15); // update the area we want to build in first
         for(const BuildingType i : bldToTest)
         {
@@ -1532,10 +1531,10 @@ void AIPlayerJH::TryToAttack()
     constexpr unsigned limit = 40;
     for(const nobMilitary* milBld : militaryBuildings)
     {
-        // We skip the current building with a probability of limit/numMilBlds
+        // We handle the current building with a probability of limit/numMilBlds
         // -> For twice the number of blds as the limit we will most likely skip every 2nd building
         // This way we check roughly (at most) limit buildings but avoid any preference for one building over an other
-        if(AI::random(numMilBlds - 1u, limit))
+        if(!AI::random(numMilBlds, limit))
             continue;
 
         if(milBld->GetFrontierDistance() == FrontierDistance::Far) // inland building? -> skip it

libs/s25main/ai/random.h

@@ -11,6 +11,7 @@ namespace AI {
 
 std::minstd_rand& getRandomGenerator();
 
+// Return a random value (min and max are included)
 template<typename T>
 T randomValue(T min = std::numeric_limits<T>::min(), T max = std::numeric_limits<T>::max())
 {
@@ -20,22 +21,28 @@ T randomValue(T min = std::numeric_limits<T>::min(), T max = std::numeric_limits
 // Return a random bool:
 //   random()           ... will return true|false with 50% chance each
 //   random(15)         ... will return true in 1/15 of the cases
-inline bool random(unsigned total = 1u)
+//   random(20, 5)      ... will return true in 5 out of 20 cases, i.e. a probability of 25%. Sames as random(4, 1)
+inline bool random(unsigned total = 2u, unsigned chance = 1u)
 {
-    return helpers::randomValue(getRandomGenerator(), 0u, total) == 0u;
+    RTTR_Assert(total > 0u);
+    return (chance >= total) || randomValue(1u, total) <= chance;
 }
 
-//   random(20, 5)      ... will return true in ~3/4 of the cases (random(20) > 5)
-inline bool random(unsigned total, unsigned chance)
+template<typename ContainerT>
+inline unsigned randomIndex(const ContainerT& container)
 {
-    return helpers::randomValue(getRandomGenerator(), 0u, total) > chance;
+    RTTR_Assert(!container.empty());
+    return randomValue(0u, static_cast<unsigned>(container.size()) - 1u);
 }
 
 template<typename ContainerT>
-inline unsigned randomIndex(const ContainerT& container)
+inline auto randomElement(const ContainerT& container)
 {
     RTTR_Assert(!container.empty());
-    return helpers::randomValue(getRandomGenerator(), 0u, static_cast<unsigned>(container.size()) - 1u);
+    auto it = container.begin();
+    if(container.size() > 1u)
+        std::advance(it, randomIndex(container));
+    return *it;
 }
 
 } // namespace AI