Merge pull request #5748 from IvanSavenko/sonar

Resolve some of Sonar / clang issues in server code

lib/networkPacks/PacksForClient.h

@@ -239,18 +239,23 @@ struct DLL_LINKAGE HeroVisitCastle : public CPackForClient
 {
 	void visitTyped(ICPackVisitor & visitor) override;
 
-	ui8 flags = 0; //1 - start
+	bool startVisit = false;
 	ObjectInstanceID tid;
 	ObjectInstanceID hid;
 
 	bool start() const //if hero is entering castle (if false - leaving)
 	{
-		return flags & 1;
+		return startVisit;
+	}
+
+	bool leave() const //if hero is entering castle (if false - leaving)
+	{
+		return !startVisit;
 	}
 
 	template <typename Handler> void serialize(Handler & h)
 	{
-		h & flags;
+		h & startVisit;
 		h & tid;
 		h & hid;
 	}

server/CGameHandler.cpp

@@ -26,19 +26,13 @@
 #include "../lib/CConfigHandler.h"
 #include "../lib/CCreatureHandler.h"
 #include "../lib/CCreatureSet.h"
-#include "../lib/texts/CGeneralTextHandler.h"
 #include "../lib/CPlayerState.h"
-#include "../lib/CRandomGenerator.h"
 #include "../lib/CSoundBase.h"
-#include "../lib/CSkillHandler.h"
-#include "../lib/CThreadHelper.h"
 #include "../lib/GameConstants.h"
-#include "../lib/UnlockGuard.h"
 #include "../lib/IGameSettings.h"
 #include "../lib/ScriptHandler.h"
 #include "../lib/StartInfo.h"
 #include "../lib/TerrainHandler.h"
-#include "../lib/VCMIDirs.h"
 #include "../lib/GameLibrary.h"
 #include "../lib/int3.h"
 
@@ -99,11 +93,6 @@
 #define COMPLAIN_RET(txt) {complain(txt); return false;}
 #define COMPLAIN_RETF(txt, FORMAT) {complain(boost::str(boost::format(txt) % FORMAT)); return false;}
 
-static inline double distance(int3 a, int3 b)
-{
-	return std::sqrt((double)(a.x-b.x)*(a.x-b.x) + (a.y-b.y)*(a.y-b.y));
-}
-
 template <typename T>
 void callWith(std::vector<T> args, std::function<void(T)> fun, ui32 which)
 {
@@ -202,7 +191,7 @@ void CGameHandler::levelUpCommander (const CCommanderInstance * c, int skill)
 {
 	SetCommanderProperty scp;
 
-	auto hero = dynamic_cast<const CGHeroInstance *>(c->getArmy());
+	const auto * hero = dynamic_cast<const CGHeroInstance *>(c->getArmy());
 	if (hero)
 		scp.heroid = hero->id;
 	else
@@ -220,10 +209,10 @@ void CGameHandler::levelUpCommander (const CCommanderInstance * c, int skill)
 	{
 		scp.which = SetCommanderProperty::BONUS;
 
-		auto difference = [](std::vector< std::vector <ui8> > skillLevels, std::vector <ui8> secondarySkills, int skill)->int
+		auto difference = [](std::vector< std::vector <ui8> > skillLevels, std::vector <ui8> secondarySkills, int skillToTest)->int
 		{
-			int s = std::min (skill, (int)ECommander::SPELL_POWER); //spell power level controls also casts and resistance
-			return skillLevels.at(skill).at(secondarySkills.at(s)) - (secondarySkills.at(s) ? skillLevels.at(skill).at(secondarySkills.at(s)-1) : 0);
+			int s = std::min (skillToTest, static_cast<int>(ECommander::SPELL_POWER)); //spell power level controls also casts and resistance
+			return skillLevels.at(skillToTest).at(secondarySkills.at(s)) - (secondarySkills.at(s) ? skillLevels.at(skillToTest).at(secondarySkills.at(s)-1) : 0);
 		};
 
 		switch (skill)
@@ -292,7 +281,7 @@ void CGameHandler::levelUpCommander(const CCommanderInstance * c)
 	}
 	CommanderLevelUp clu;
 
-	auto hero = dynamic_cast<const CGHeroInstance *>(c->getArmy());
+	const auto * hero = dynamic_cast<const CGHeroInstance *>(c->getArmy());
 	if(hero)
 	{
 		clu.heroId = hero->id;
@@ -320,7 +309,7 @@ void CGameHandler::levelUpCommander(const CCommanderInstance * c)
 			clu.skills.push_back (i);
 		++i;
 	}
-	int skillAmount = static_cast<int>(clu.skills.size());
+	int skillAmount = clu.skills.size();
 
 	if (!skillAmount)
 	{
@@ -427,7 +416,7 @@ void CGameHandler::changeSecSkill(const CGHeroInstance * hero, SecondarySkill wh
 
 }
 
-void CGameHandler::handleClientDisconnection(std::shared_ptr<CConnection> c)
+void CGameHandler::handleClientDisconnection(const std::shared_ptr<CConnection> & c)
 {
 	if(gameLobby().getState() == EServerState::SHUTDOWN || !gameState().getStartInfo())
 	{
@@ -446,18 +435,18 @@ void CGameHandler::handleClientDisconnection(std::shared_ptr<CConnection> c)
 		logGlobal->trace("Player %s disconnected. Notifying remaining players", playerId.toString());
 
 		// this player have left the game - broadcast infowindow to all in-game players
-		for (auto i = gameState().players.cbegin(); i!=gameState().players.cend(); i++)
+		for (const auto & player : gameState().players)
 		{
-			if (i->first == playerId)
+			if (player.first == playerId)
 				continue;
 
-			if (gameInfo().getPlayerState(i->first)->status != EPlayerStatus::INGAME)
+			if (gameInfo().getPlayerState(player.first)->status != EPlayerStatus::INGAME)
 				continue;
 
-			logGlobal->trace("Notifying player %s", i->first);
+			logGlobal->trace("Notifying player %s", player.first);
 
 			InfoWindow out;
-			out.player = i->first;
+			out.player = player.first;
 			out.text.appendTextID("vcmi.server.errors.playerLeft");
 			out.text.replaceName(playerId);
 			out.components.emplace_back(ComponentType::FLAG, playerId);
@@ -511,31 +500,32 @@ CGameHandler::CGameHandler(CVCMIServer * lobby)
 	: lobby(lobby)
 	, heroPool(std::make_unique<HeroPoolProcessor>(this))
 	, battles(std::make_unique<BattleProcessor>(this))
-	, turnOrder(std::make_unique<TurnOrderProcessor>(this))
 	, queries(std::make_unique<QueriesProcessor>())
+	, turnOrder(std::make_unique<TurnOrderProcessor>(this))
+	, turnTimerHandler(std::make_unique<TurnTimerHandler>(*this))
+	, newTurnProcessor(std::make_unique<NewTurnProcessor>(this))
+	, statistics(std::make_unique<StatisticDataSet>())
+	, spellEnv(std::make_unique<ServerSpellCastEnvironment>(this))
 	, playerMessages(std::make_unique<PlayerMessageProcessor>(this))
+	, QID(1)
 	, complainNoCreatures("No creatures to split")
 	, complainNotEnoughCreatures("Cannot split that stack, not enough creatures!")
 	, complainInvalidSlot("Invalid slot accessed!")
-	, turnTimerHandler(std::make_unique<TurnTimerHandler>(*this))
-	, newTurnProcessor(std::make_unique<NewTurnProcessor>(this))
-	, statistics(std::make_unique<StatisticDataSet>())
 {
-	QID = 1;
-
-	spellEnv = new ServerSpellCastEnvironment(this);
 }
 
-CGameHandler::~CGameHandler()
+CGameHandler::~CGameHandler() = default;
+
+ServerCallback * CGameHandler::spellcastEnvironment() const
 {
-	delete spellEnv;
+	return spellEnv.get();
 }
 
 void CGameHandler::reinitScripting()
 {
 	serverEventBus = std::make_unique<events::EventBus>();
 #if SCRIPTING_ENABLED
-	serverScripts.reset(new scripting::PoolImpl(this, spellEnv));
+	serverScripts.reset(new scripting::PoolImpl(this, spellEnv.get()));
 #endif
 }
 
@@ -635,7 +625,7 @@ void CGameHandler::onNewTurn()
 
 	if (firstTurn)
 	{
-		for (auto obj : gameState().getMap().getObjects<CGHeroInstance>())
+		for (const auto * obj : gameState().getMap().getObjects<CGHeroInstance>())
 		{
 			if (obj->ID == Obj::PRISON) //give imprisoned hero 0 exp to level him up. easiest to do at this point
 			{
@@ -654,22 +644,22 @@ void CGameHandler::onNewTurn()
 
 	for (const auto & townID : gameState().getMap().getAllTowns())
 	{
-		auto t = gameState().getTown(townID);
+		const auto * t = gameState().getTown(townID);
 		PlayerColor player = t->tempOwner;
 
+		// Skyship, probably easier to handle same as Veil of darkness
+		// do it every new day before veils
 		if(t->hasBuilt(BuildingID::GRAIL)
+		   && player.isValidPlayer()
 			&& t->getTown()->buildings.at(BuildingID::GRAIL)->height == CBuilding::HEIGHT_SKYSHIP)
 		{
-			// Skyship, probably easier to handle same as Veil of darkness
-			// do it every new day before veils
-			if (player.isValidPlayer())
-				changeFogOfWar(t->getSightCenter(), t->getSightRadius(), player, ETileVisibility::REVEALED);
+			changeFogOfWar(t->getSightCenter(), t->getSightRadius(), player, ETileVisibility::REVEALED);
 		}
 	}
 
 	for (const auto & townID : gameState().getMap().getAllTowns())
 	{
-		auto t = gameState().getTown(townID);
+		const auto * t = gameState().getTown(townID);
 		if(t->hasBonusOfType(BonusType::DARKNESS))
 		{
 			for(const auto & player : gameState().players)
@@ -706,7 +696,7 @@ void CGameHandler::start(bool resume)
 {
 	LOG_TRACE_PARAMS(logGlobal, "resume=%d", resume);
 
-	for (auto cc : gameLobby().activeConnections)
+	for (const auto & cc : gameLobby().activeConnections)
 	{
 		auto players = gameLobby().getAllClientPlayers(cc->connectionID);
 		std::stringstream sbuffer;
@@ -755,7 +745,7 @@ void CGameHandler::giveSpells(const CGTownInstance *t, const CGHeroInstance *h)
 		{
 			std::vector<SpellID> spells;
 			gameState().getAllowedSpells(spells, i+1);
-			for (auto & spell : spells)
+			for (const auto & spell : spells)
 				cs.spells.insert(spell);
 		}
 	}
@@ -825,7 +815,7 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, EMovementMode moveme
 	{
 		for (auto const & objectID : gameInfo().getTile(guardPos)->visitableObjects)
 		{
-			auto object = gameState().getObjInstance(objectID);
+			const auto * object = gameState().getObjInstance(objectID);
 
 			if (object->ID == MapObjectID::MONSTER) // exclude other objects, such as hero flying above monster
 				guardian = object;
@@ -847,7 +837,7 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, EMovementMode moveme
 
 	//check if destination tile is available
 	auto pathfinderHelper = std::make_unique<CPathfinderHelper>(gameState(), h, PathfinderOptions(gameInfo()));
-	auto ti = pathfinderHelper->getTurnInfo();
+	const auto * ti = pathfinderHelper->getTurnInfo();
 
 	const bool canFly = ti->hasFlyingMovement() || (h->inBoat() && h->getBoat()->layer == EPathfindingLayer::AIR);
 	const bool canWalkOnSea = ti->hasWaterWalking() || (h->inBoat() && h->getBoat()->layer == EPathfindingLayer::WATER);
@@ -889,7 +879,7 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, EMovementMode moveme
 	if(h->inBoat() && h->getBoat()->layer == EPathfindingLayer::SAIL && t.isLand() && t.blocked())
 		return complainRet("Cannot disembark hero, tile is blocked!");
 
-	if(distance(h->pos, dst) >= 1.5 && movementMode == EMovementMode::STANDARD)
+	if(!h->pos.areNeighbours(dst) && movementMode == EMovementMode::STANDARD)
 		return complainRet("Tiles " + h->pos.toString()+ " and "+ dst.toString() +" are not neighboring!");
 
 	if(h->isGarrisoned())
@@ -1010,7 +1000,7 @@ bool CGameHandler::moveHero(ObjectInstanceID hid, int3 dst, EMovementMode moveme
 
 	//still here? it is standard movement!
 	{
-		tmh.movePoints = (int)h->movementPointsRemaining() >= cost
+		tmh.movePoints = h->movementPointsRemaining() >= cost
 						? h->movementPointsRemaining() - cost
 						: 0;
 
@@ -1081,22 +1071,16 @@ void CGameHandler::setOwner(const CGObjectInstance * obj, const PlayerColor owne
 	{
 		statistics->accumulatedValues[owner].lastCapturedTownDay = gameState().getDate(Date::DAY);
 
-		if (owner.isValidPlayer()) //new owner is real player
-		{
-			if (town->hasBuilt(BuildingSubID::PORTAL_OF_SUMMONING))
-				setPortalDwelling(town, true, false);
-		}
+		if (owner.isValidPlayer() && town->hasBuilt(BuildingSubID::PORTAL_OF_SUMMONING))
+			setPortalDwelling(town, true, false);
 	}
 
-	if ((obj->ID == Obj::CREATURE_GENERATOR1 || obj->ID == Obj::CREATURE_GENERATOR4))
+	if ((obj->ID == Obj::CREATURE_GENERATOR1 || obj->ID == Obj::CREATURE_GENERATOR4) && owner.isValidPlayer())
 	{
-		if (owner.isValidPlayer())
+		for (const CGTownInstance * t : gameInfo().getPlayerState(owner)->getTowns())
 		{
-			for (const CGTownInstance * t : gameInfo().getPlayerState(owner)->getTowns())
-			{
-				if (t->hasBuilt(BuildingSubID::PORTAL_OF_SUMMONING))
-					setPortalDwelling(t);//set initial creatures for all portals of summoning
-			}
+			if (t->hasBuilt(BuildingSubID::PORTAL_OF_SUMMONING))
+				setPortalDwelling(t);//set initial creatures for all portals of summoning
 		}
 	}
 }
@@ -1117,9 +1101,10 @@ void CGameHandler::showTeleportDialog(TeleportDialog *iw)
 	sendToAllClients(*iw);
 }
 
-void CGameHandler::giveResource(PlayerColor player, GameResID which, int val) //TODO: cap according to Bersy's suggestion
+void CGameHandler::giveResource(PlayerColor player, GameResID which, int val)
 {
-	if (!val) return; //don't waste time on empty call
+	if (!val)
+		return; //don't waste time on empty call
 
 	TResources resources;
 	resources[which] = val;
@@ -1173,7 +1158,7 @@ void CGameHandler::giveCreatures(const CArmedInstance *obj, const CGHeroInstance
 	COMPLAIN_RET_IF(creatures.stacksCount() > GameConstants::ARMY_SIZE, "Too many stacks to give!");
 
 	//first we move creatures to give to make them army of object-source
-	for (auto & elem : creatures.Slots())
+	for (const auto & elem : creatures.Slots())
 	{
 		addToSlot(StackLocation(obj->id, obj->getSlotFor(elem.second->getCreature())), elem.second->getCreature(), elem.second->getCount());
 	}
@@ -1232,7 +1217,7 @@ void CGameHandler::heroVisitCastle(const CGTownInstance * obj, const CGHeroInsta
 		HeroVisitCastle vc;
 		vc.hid = hero->id;
 		vc.tid = obj->id;
-		vc.flags |= 1;
+		vc.startVisit = true;
 		sendAndApply(vc);
 	}
 	visitCastleObjects(obj, hero);
@@ -1249,13 +1234,13 @@ void CGameHandler::visitCastleObjects(const CGTownInstance * t, const CGHeroInst
 	visitCastleObjects(t, visitors);
 }
 
-void CGameHandler::visitCastleObjects(const CGTownInstance * t, std::vector<const CGHeroInstance * > visitors)
+void CGameHandler::visitCastleObjects(const CGTownInstance * t, const std::vector<const CGHeroInstance * > & visitors)
 {
 	std::vector<BuildingID> buildingsToVisit;
 	for (auto const & hero : visitors)
 		giveSpells (t, hero);
 
-	for (auto & building : t->rewardableBuildings)
+	for (const auto & building : t->rewardableBuildings)
 	{
 		if (!t->getTown()->buildings.at(building.first)->manualHeroVisit && t->hasBuilt(building.first))
 			buildingsToVisit.push_back(building.first);
@@ -1345,7 +1330,7 @@ void CGameHandler::giveHero(ObjectInstanceID id, PlayerColor player, ObjectInsta
 	sendAndApply(gh);
 
 	//Reveal fow around new hero, especially released from Prison
-	auto h = gameInfo().getHero(id);
+	const auto * h = gameInfo().getHero(id);
 	changeFogOfWar(h->getSightCenter(), h->getSightRadius(), player, ETileVisibility::REVEALED);
 }
 
@@ -1408,7 +1393,7 @@ void CGameHandler::useScholarSkill(ObjectInstanceID fromHero, ObjectInstanceID t
 		if (!cs2.spells.empty())//if found new spell - apply
 		{
 			iw.text.appendLocalString(EMetaText::GENERAL_TXT, 140);//learns
-			int size = static_cast<int>(cs2.spells.size());
+			int size = cs2.spells.size();
 			for (auto it : cs2.spells)
 			{
 				iw.components.emplace_back(ComponentType::SPELL, it);
@@ -1436,7 +1421,7 @@ void CGameHandler::useScholarSkill(ObjectInstanceID fromHero, ObjectInstanceID t
 		if (!cs1.spells.empty())
 		{
 			iw.text.appendLocalString(EMetaText::GENERAL_TXT, 147);//teaches
-			int size = static_cast<int>(cs1.spells.size());
+			int size = cs1.spells.size();
 			for (auto it : cs1.spells)
 			{
 				iw.components.emplace_back(ComponentType::SPELL, it);
@@ -1461,8 +1446,8 @@ void CGameHandler::useScholarSkill(ObjectInstanceID fromHero, ObjectInstanceID t
 
 void CGameHandler::heroExchange(ObjectInstanceID hero1, ObjectInstanceID hero2)
 {
-	auto h1 = gameInfo().getHero(hero1);
-	auto h2 = gameInfo().getHero(hero2);
+	const auto * h1 = gameInfo().getHero(hero1);
+	const auto * h2 = gameInfo().getHero(hero2);
 
 	if (gameInfo().getPlayerRelations(h1->getOwner(), h2->getOwner()) != PlayerRelations::ENEMIES)
 	{
@@ -1479,10 +1464,10 @@ void CGameHandler::heroExchange(ObjectInstanceID hero1, ObjectInstanceID hero2)
 	}
 }
 
-void CGameHandler::sendToAllClients(CPackForClient & pack)
+void CGameHandler::sendToAllClients(const CPackForClient & pack)
 {
 	logNetwork->trace("\tSending to all clients: %s", typeid(pack).name());
-	for (auto c : gameLobby().activeConnections)
+	for (const auto & c : gameLobby().activeConnections)
 		c->sendPack(pack);
 }
 
@@ -1563,7 +1548,7 @@ void CGameHandler::throwIfWrongPlayer(const std::shared_ptr<CConnection> & conne
 	}
 }
 
-void CGameHandler::throwAndComplain(const std::shared_ptr<CConnection> & connection, std::string txt)
+void CGameHandler::throwAndComplain(const std::shared_ptr<CConnection> & connection, const std::string & txt)
 {
 	complain(txt);
 	throwNotAllowedAction(connection);
@@ -1711,14 +1696,14 @@ bool CGameHandler::bulkMergeStacks(SlotID slotSrc, ObjectInstanceID srcOwner)
 	if(actualAmount < 1 && complain(complainNoCreatures))
 		return false;
 
-	auto currentCreature = creatureSet.getCreature(slotSrc);
+	const auto * currentCreature = creatureSet.getCreature(slotSrc);
 
 	if(!currentCreature && complain(complainNoCreatures))
 		return false;
 
 	auto creatureSlots = creatureSet.getCreatureSlots(currentCreature, slotSrc);
 
-	if(!creatureSlots.size())
+	if(creatureSlots.empty())
 		return false;
 
 	BulkRebalanceStacks bulkRS;
@@ -1752,8 +1737,7 @@ bool CGameHandler::bulkMoveArmy(ObjectInstanceID srcArmy, ObjectInstanceID destA
 	const CCreatureSet & setDest = *armyDest;
 	auto freeSlots = setDest.getFreeSlotsQueue();
 
-	typedef std::map<SlotID, std::pair<SlotID, TQuantity>> TRebalanceMap;
-	TRebalanceMap moves;
+	std::map<SlotID, std::pair<SlotID, TQuantity>> moves;
 
 	auto srcQueue = setSrc.getCreatureQueue(srcSlot); // Exclude srcSlot, it should be moved last
 	auto slotsLeft = setSrc.stacksCount();
@@ -1765,11 +1749,11 @@ bool CGameHandler::bulkMoveArmy(ObjectInstanceID srcArmy, ObjectInstanceID destA
 		auto pair = srcQueue.top();
 		srcQueue.pop();
 
-		auto currCreature = pair.first;
+		const auto * currCreature = pair.first;
 		auto currSlot = pair.second;
 		const auto quantity = setSrc.getStackCount(currSlot);
 
-		TMapCreatureSlot::iterator lb = destMap.lower_bound(currCreature);
+		const auto lb = destMap.lower_bound(currCreature);
 		const bool alreadyExists = (lb != destMap.end() && !(keyComp(currCreature, lb->first)));
 
 		if(!alreadyExists)
@@ -1786,7 +1770,7 @@ bool CGameHandler::bulkMoveArmy(ObjectInstanceID srcArmy, ObjectInstanceID destA
 	}
 	if(slotsLeft == 1)
 	{
-		auto lastCreature = setSrc.getCreature(srcSlot);
+		const auto * lastCreature = setSrc.getCreature(srcSlot);
 		auto slotToMove = SlotID();
 		// Try to find a slot for last creature
 		if(destMap.find(lastCreature) == destMap.end())
@@ -1810,7 +1794,7 @@ bool CGameHandler::bulkMoveArmy(ObjectInstanceID srcArmy, ObjectInstanceID destA
 	}
 	BulkRebalanceStacks bulkRS;
 
-	for(auto & move : moves)
+	for(const auto & move : moves)
 	{
 		RebalanceStacks rs;
 		rs.srcArmy = armySrc->id;
@@ -1841,7 +1825,7 @@ bool CGameHandler::bulkSplitAndRebalanceStack(SlotID slotSrc, ObjectInstanceID s
 		return false;
 
 	auto freeSlot = creatureSet.getFreeSlot();
-	auto currentCreature = creatureSet.getCreature(slotSrc);
+	const auto * currentCreature = creatureSet.getCreature(slotSrc);
 
 	if(freeSlot == SlotID() && creatureSet.isCreatureBalanced(currentCreature))
 		return true;
@@ -1946,7 +1930,7 @@ bool CGameHandler::arrangeStacks(ObjectInstanceID id1, ObjectInstanceID id2, ui8
 	{
 		if (id1 != id2) // Stack arrangement inside locked garrison is allowed
 		{
-			auto g = dynamic_cast<const CGGarrison *>(army);
+			const auto * g = dynamic_cast<const CGGarrison *>(army);
 			if (g && !g->removableUnits)
 			{
 				complain("Stacks in this garrison are not removable!\n");
@@ -2062,7 +2046,7 @@ bool CGameHandler::arrangeStacks(ObjectInstanceID id1, ObjectInstanceID id2, ui8
 	return true;
 }
 
-bool CGameHandler::hasPlayerAt(PlayerColor player, std::shared_ptr<CConnection> c) const
+bool CGameHandler::hasPlayerAt(PlayerColor player,  const std::shared_ptr<CConnection> & c) const
 {
 	return connections.count(player) && connections.at(player).count(c);
 }
@@ -2176,13 +2160,13 @@ bool CGameHandler::buildStructure(ObjectInstanceID tid, BuildingID requestedID,
 	};
 
 	//Checks if all requirements will be met with expected building list "buildingsThatWillBe"
-	auto areRequirementsFulfilled = [&](const BuildingID & buildID)
+	auto areRequirementsFulfilled = [&buildingsThatWillBe](const BuildingID & buildID)
 	{
 		return buildingsThatWillBe.count(buildID);
 	};
 
 	//Init the vectors
-	for(auto & build : t->getTown()->buildings)
+	for(const auto & build : t->getTown()->buildings)
 	{
 		if(t->hasBuilt(build.first))
 		{
@@ -2205,14 +2189,14 @@ bool CGameHandler::buildStructure(ObjectInstanceID tid, BuildingID requestedID,
 
 	while(!buildingsToAdd.empty())
 	{
-		auto b = buildingsToAdd.front();
+		const auto * b = buildingsToAdd.front();
 		buildingsToAdd.pop();
 
 		ns.bid.insert(b->bid);
 		buildingsThatWillBe.insert(b->bid);
 		remainingAutoBuildings -= b;
 
-		for(auto autoBuilding : remainingAutoBuildings)
+		for(const auto * autoBuilding : remainingAutoBuildings)
 		{
 			auto actualRequirements = t->genBuildingRequirements(autoBuilding->bid);
 
@@ -2303,15 +2287,6 @@ bool CGameHandler::razeStructure (ObjectInstanceID tid, BuildingID bid)
 	rs.bid.insert(bid);
 	rs.destroyed = t->destroyed + 1;
 	sendAndApply(rs);
-//TODO: Remove dwellers
-// 	if (t->subID == 4 && bid == 17) //Veil of Darkness
-// 	{
-// 		RemoveBonus rb(RemoveBonus::TOWN);
-// 		rb.whoID = t->id;
-// 		rb.source = BonusSource::TOWN_STRUCTURE;
-// 		rb.id = 17;
-// 		sendAndApply(rb);
-// 	}
 	return true;
 }
 
@@ -2448,14 +2423,14 @@ bool CGameHandler::recruitCreatures(ObjectInstanceID objid, ObjectInstanceID dst
 		COMPLAIN_RET_FALSE_IF(hero->hasArt(artId),"Hero already has this machine!");
 
 		bool hasFreeSlot = false;
-		for(auto slot : art->getPossibleSlots().at(ArtBearer::HERO))
-			if (hero->getArt(slot) == nullptr)
+		for(auto possibleSlot : art->getPossibleSlots().at(ArtBearer::HERO))
+			if (hero->getArt(possibleSlot) == nullptr)
 				hasFreeSlot = true;
 
 		if (!hasFreeSlot)
 		{
-			auto slot = art->getPossibleSlots().at(ArtBearer::HERO).front();
-			removeArtifact(ArtifactLocation(hero->id, slot));
+			auto possibleSlot = art->getPossibleSlots().at(ArtBearer::HERO).front();
+			removeArtifact(ArtifactLocation(hero->id, possibleSlot));
 		}
 		return giveHeroNewArtifact(hero, artId, ArtifactPosition::FIRST_AVAILABLE);
 	}
@@ -2631,8 +2606,8 @@ bool CGameHandler::garrisonSwap(ObjectInstanceID tid)
 // Function moves artifact from src to dst. If dst is not a backpack and is already occupied, old dst art goes to backpack and is replaced.
 bool CGameHandler::moveArtifact(const PlayerColor & player, const ArtifactLocation & src, const ArtifactLocation & dst)
 {
-	const auto srcArtSet = gameState().getArtSet(src);
-	const auto dstArtSet = gameState().getArtSet(dst);
+	const auto * srcArtSet = gameState().getArtSet(src);
+	const auto * dstArtSet = gameState().getArtSet(dst);
 	assert(srcArtSet);
 	assert(dstArtSet);
 
@@ -2641,13 +2616,13 @@ bool CGameHandler::moveArtifact(const PlayerColor & player, const ArtifactLocati
 		COMPLAIN_RET("That heroes cannot make any exchange!");
 
 	COMPLAIN_RET_FALSE_IF(!ArtifactUtils::checkIfSlotValid(*srcArtSet, src.slot), "moveArtifact: wrong artifact source slot");
-	const auto srcArtifact = srcArtSet->getArt(src.slot);
+	const auto * srcArtifact = srcArtSet->getArt(src.slot);
 	auto dstSlot = dst.slot;
 	if(dstSlot == ArtifactPosition::FIRST_AVAILABLE)
 		dstSlot = ArtifactUtils::getArtAnyPosition(dstArtSet, srcArtifact->getTypeId());
 	if(!ArtifactUtils::checkIfSlotValid(*dstArtSet, dstSlot))
 		return true;
-	const auto dstArtifact = dstArtSet->getArt(dstSlot);
+	const auto * dstArtifact = dstArtSet->getArt(dstSlot);
 	const bool isDstSlotOccupied = dstArtSet->bearerType() == ArtBearer::ALTAR ? false : dstArtifact != nullptr;
 	const bool isDstSlotBackpack = dstArtSet->bearerType() == ArtBearer::HERO ? ArtifactUtils::isSlotBackpack(dstSlot) : false;
 
@@ -2661,8 +2636,8 @@ bool CGameHandler::moveArtifact(const PlayerColor & player, const ArtifactLocati
 	if((!srcArtifact || !isDstSlotBackpack) && !srcArtifact->canBePutAt(dstArtSet, dstSlot, true))
 		COMPLAIN_RET("Cannot move artifact!");
 
-	auto srcSlotInfo = srcArtSet->getSlot(src.slot);
-	auto dstSlotInfo = dstArtSet->getSlot(dstSlot);
+	const auto * srcSlotInfo = srcArtSet->getSlot(src.slot);
+	const auto * dstSlotInfo = dstArtSet->getSlot(dstSlot);
 
 	if((srcSlotInfo && srcSlotInfo->locked) || (dstSlotInfo && dstSlotInfo->locked))
 		COMPLAIN_RET("Cannot move artifact locks.");
@@ -2691,7 +2666,7 @@ bool CGameHandler::moveArtifact(const PlayerColor & player, const ArtifactLocati
 		ma.artsPack1.emplace_back(dstSlot, src.slot);
 	}
 
-	auto hero = gameInfo().getHero(dst.artHolder);
+	const auto * hero = gameInfo().getHero(dst.artHolder);
 	if(ArtifactUtils::checkSpellbookIsNeeded(hero, srcArtifact->getTypeId(), dstSlot))
 		giveHeroNewArtifact(hero, ArtifactID::SPELLBOOK, ArtifactPosition::SPELLBOOK);
 
@@ -2708,8 +2683,8 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 	if(!isAllowedExchange(srcId, dstId))
 		COMPLAIN_RET("That heroes cannot make any exchange!");
 
-	auto psrcSet = gameState().getArtSet(srcId);
-	auto pdstSet = gameState().getArtSet(dstId);
+	const auto * psrcSet = gameState().getArtSet(srcId);
+	const auto * pdstSet = gameState().getArtSet(dstId);
 	if((!psrcSet) || (!pdstSet))
 		COMPLAIN_RET("bulkMoveArtifacts: wrong hero's ID");
 
@@ -2731,7 +2706,7 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 			slots.emplace_back(srcSlot, dstSlot);
 
 			// TODO Shouldn't be here. Possibly in callback after equipping the artifact
-			if(auto dstHero = gameInfo().getHero(dstId))
+			if(const auto * dstHero = gameInfo().getHero(dstId))
 			{
 				if(ArtifactUtils::checkSpellbookIsNeeded(dstHero, artifact->getTypeId(), dstSlot))
 					giveHeroNewArtifact(dstHero, ArtifactID::SPELLBOOK, ArtifactPosition::SPELLBOOK);
@@ -2743,7 +2718,7 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 	{
 		auto moveArtsWorn = [moveArtifact](const CArtifactSet * srcArtSet, std::vector<MoveArtifactInfo> & slots)
 		{
-			for(auto & artifact : srcArtSet->artifactsWorn)
+			for(const auto & artifact : srcArtSet->artifactsWorn)
 			{
 				if(ArtifactUtils::isArtRemovable(artifact))
 					moveArtifact(artifact.second.getArt(), artifact.first, slots);
@@ -2752,7 +2727,7 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 		auto moveArtsInBackpack = [](const CArtifactSet * artSet,
 			std::vector<MoveArtifactInfo> & slots) -> void
 		{
-			for(auto & slotInfo : artSet->artifactsInBackpack)
+			for(const auto & slotInfo : artSet->artifactsInBackpack)
 			{
 				auto slot = artSet->getArtPos(slotInfo.getArt());
 				slots.emplace_back(slot, slot);
@@ -2781,7 +2756,7 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 		if(equipped)
 		{
 			// Move over artifacts that are worn
-			for(auto & artInfo : psrcSet->artifactsWorn)
+			for(const auto & artInfo : psrcSet->artifactsWorn)
 			{
 				if(ArtifactUtils::isArtRemovable(artInfo))
 				{
@@ -2792,7 +2767,7 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 		if(backpack)
 		{
 			// Move over artifacts that are in backpack
-			for(auto & slotInfo : psrcSet->artifactsInBackpack)
+			for(const auto & slotInfo : psrcSet->artifactsInBackpack)
 			{
 				moveArtifact(psrcSet->getArt(psrcSet->getArtPos(slotInfo.getArt())),
 					psrcSet->getArtPos(slotInfo.getArt()), slotsSrcDst);
@@ -2803,9 +2778,9 @@ bool CGameHandler::bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceI
 	return true;
 }
 
-bool CGameHandler::manageBackpackArtifacts(const PlayerColor & player, const ObjectInstanceID heroID, const ManageBackpackArtifacts::ManageCmd & sortType)
+bool CGameHandler::manageBackpackArtifacts(const PlayerColor & player, const ObjectInstanceID & heroID, const ManageBackpackArtifacts::ManageCmd & sortType)
 {
-	const auto artSet = gameState().getArtSet(heroID);
+	const auto * artSet = gameState().getArtSet(heroID);
 	COMPLAIN_RET_FALSE_IF(artSet == nullptr, "manageBackpackArtifacts: wrong hero's ID");
 
 	BulkMoveArtifacts bma(player, heroID, heroID, false);
@@ -2887,9 +2862,9 @@ bool CGameHandler::manageBackpackArtifacts(const PlayerColor & player, const Obj
 	return true;
 }
 
-bool CGameHandler::saveArtifactsCostume(const PlayerColor & player, const ObjectInstanceID heroID, uint32_t costumeIdx)
+bool CGameHandler::saveArtifactsCostume(const PlayerColor & player, const ObjectInstanceID & heroID, uint32_t costumeIdx)
 {
-	auto artSet = gameState().getArtSet(heroID);
+	const auto * artSet = gameState().getArtSet(heroID);
 	COMPLAIN_RET_FALSE_IF(artSet == nullptr, "saveArtifactsCostume: wrong hero's ID");
 
 	ChangeArtifactsCostume costume(player, costumeIdx);
@@ -2903,11 +2878,11 @@ bool CGameHandler::saveArtifactsCostume(const PlayerColor & player, const Object
 	return true;
 }
 
-bool CGameHandler::switchArtifactsCostume(const PlayerColor & player, const ObjectInstanceID heroID, uint32_t costumeIdx)
+bool CGameHandler::switchArtifactsCostume(const PlayerColor & player, const ObjectInstanceID & heroID, uint32_t costumeIdx)
 {
-	const auto artSet = gameState().getArtSet(heroID);
+	const auto * artSet = gameState().getArtSet(heroID);
 	COMPLAIN_RET_FALSE_IF(artSet == nullptr, "switchArtifactsCostume: wrong hero's ID");
-	const auto playerState = gameInfo().getPlayerState(player);
+	const auto * playerState = gameInfo().getPlayerState(player);
 	COMPLAIN_RET_FALSE_IF(playerState == nullptr, "switchArtifactsCostume: wrong player");
 
 	if(auto costume = playerState->costumesArtifacts.find(costumeIdx); costume != playerState->costumesArtifacts.end())
@@ -3106,7 +3081,7 @@ bool CGameHandler::buyArtifact(const IMarket *m, const CGHeroInstance *h, GameRe
 		saa.id = ObjectInstanceID::NONE;
 		saa.arts = gameState().getMap().townMerchantArtifacts;
 	}
-	else if(const CGBlackMarket *bm = dynamic_cast<const CGBlackMarket *>(m)) //black market
+	else if(const auto *bm = dynamic_cast<const CGBlackMarket *>(m)) //black market
 	{
 		saa.id = bm->id;
 		saa.arts = bm->artifacts;
@@ -3226,7 +3201,7 @@ bool CGameHandler::sellCreatures(ui32 count, const IMarket *market, const CGHero
 		assert(0);
 	}
 
-	changeStackCount(StackLocation(hero->id, slot), -(TQuantity)count, ChangeValueMode::RELATIVE);
+	changeStackCount(StackLocation(hero->id, slot), -static_cast<int>(count), ChangeValueMode::RELATIVE);
 
 	giveResource(hero->tempOwner, resourceID, b2 * units);
 
@@ -3274,7 +3249,7 @@ bool CGameHandler::sendResources(ui32 val, PlayerColor player, GameResID r1, Pla
 
 	vstd::amin(val, curRes1);
 
-	giveResource(player, r1, -(int)val);
+	giveResource(player, r1, -static_cast<int>(val));
 	giveResource(r2, r1, val);
 
 	return true;
@@ -3338,9 +3313,8 @@ bool CGameHandler::complain(const std::string &problem)
 
 void CGameHandler::showGarrisonDialog(ObjectInstanceID upobj, ObjectInstanceID hid, bool removableUnits)
 {
-	//PlayerColor player = getOwner(hid);
-	auto upperArmy = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(upobj));
-	auto lowerArmy = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(hid));
+	const auto * upperArmy = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(upobj));
+	const auto * lowerArmy = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(hid));
 
 	assert(lowerArmy);
 	assert(upperArmy);
@@ -3397,7 +3371,7 @@ bool CGameHandler::isAllowedExchange(ObjectInstanceID id1, ObjectInstanceID id2)
 				return true;
 		}
 
-		auto market = gameState().getMarket(id1);
+		const auto * market = gameState().getMarket(id1);
 		if(market == nullptr)
 			market = gameState().getMarket(id2);
 		if(market)
@@ -3421,8 +3395,8 @@ bool CGameHandler::isAllowedExchange(ObjectInstanceID id1, ObjectInstanceID id2)
 		}
 		if (dialog)
 		{
-			auto topArmy = dialog->exchangingArmies.at(0);
-			auto bottomArmy = dialog->exchangingArmies.at(1);
+			const auto * topArmy = dialog->exchangingArmies.at(0);
+			const auto * bottomArmy = dialog->exchangingArmies.at(1);
 
 			if ((topArmy == o1 && bottomArmy == o2) || (bottomArmy == o1 && topArmy == o2))
 				return true;
@@ -3448,7 +3422,7 @@ void CGameHandler::objectVisited(const CGObjectInstance * obj, const CGHeroInsta
 
 	auto startVisit = [&](ObjectVisitStarted & event)
 	{
-		auto visitedObject = obj;
+		const auto * visitedObject = obj;
 
 		if(obj->ID == Obj::HERO)
 		{
@@ -3575,19 +3549,19 @@ void CGameHandler::checkVictoryLossConditionsForPlayer(PlayerColor player)
 		if (victoryLossCheckResult.victory())
 		{
 			//one player won -> all enemies lost
-			for (auto i = gameState().players.cbegin(); i!=gameState().players.cend(); i++)
+			for (const auto & playerIt : gameState().players)
 			{
-				if (i->first != player && gameInfo().getPlayerState(i->first)->status == EPlayerStatus::INGAME)
+				if (playerIt.first != player && gameInfo().getPlayerState(playerIt.first)->status == EPlayerStatus::INGAME)
 				{
-					peg.player = i->first;
-					peg.victoryLossCheckResult = gameInfo().getPlayerRelations(player, i->first) == PlayerRelations::ALLIES ?
+					peg.player = playerIt.first;
+					peg.victoryLossCheckResult = gameInfo().getPlayerRelations(player, playerIt.first) == PlayerRelations::ALLIES ?
 								victoryLossCheckResult : victoryLossCheckResult.invert(); // ally of winner
 
-					InfoWindow iw;
-					getVictoryLossMessage(player, peg.victoryLossCheckResult, iw);
-					iw.player = i->first;
+					InfoWindow iwOthers;
+					getVictoryLossMessage(player, peg.victoryLossCheckResult, iwOthers);
+					iwOthers.player = playerIt.first;
 
-					sendAndApply(iw);
+					sendAndApply(iwOthers);
 					sendAndApply(peg);
 				}
 			}
@@ -3604,14 +3578,14 @@ void CGameHandler::checkVictoryLossConditionsForPlayer(PlayerColor player)
 
 			//copy heroes vector to avoid iterator invalidation as removal change PlayerState
 			auto hlp = p->getHeroes();
-			for (auto h : hlp) //eliminate heroes
+			for (const auto * h : hlp) //eliminate heroes
 			{
 				if (h)
 					removeObject(h, player);
 			}
 
 			//player lost -> all his objects become unflagged (neutral)
-			for (auto obj : gameState().getMap().getObjects()) //unflag objs
+			for (const auto * obj : gameState().getMap().getObjects()) //unflag objs
 			{
 				if (obj && obj->tempOwner == player)
 					setOwner(obj, PlayerColor::NEUTRAL);
@@ -3632,10 +3606,10 @@ void CGameHandler::checkVictoryLossConditionsForPlayer(PlayerColor player)
 			{
 				if (gameInfo().getPlayerState(pc)->status == EPlayerStatus::INGAME)
 				{
-					InfoWindow iw;
-					getVictoryLossMessage(player, victoryLossCheckResult.invert(), iw);
-					iw.player = pc;
-					sendAndApply(iw);
+					InfoWindow iwOthers;
+					getVictoryLossMessage(player, victoryLossCheckResult.invert(), iwOthers);
+					iwOthers.player = pc;
+					sendAndApply(iwOthers);
 				}
 			}
 			checkVictoryLossConditions(playerColors);
@@ -3720,7 +3694,7 @@ bool CGameHandler::sacrificeCreatures(const IMarket * market, const CGHeroInstan
 	{
 		int oldCount = hero->getStackCount(slot[i]);
 
-		if(oldCount < (int)count[i])
+		if(oldCount < static_cast<int>(count[i]))
 		{
 			finish();
 			COMPLAIN_RET("Not enough creatures to sacrifice!")
@@ -3755,7 +3729,7 @@ bool CGameHandler::sacrificeArtifact(const IMarket * market, const CGHeroInstanc
 		COMPLAIN_RET("Evil hero can't sacrifice artifact!");
 
 	assert(market);
-	const auto artSet = market->getArtifactsStorage();
+	const auto * artSet = market->getArtifactsStorage();
 
 	int expSum = 0;
 	std::vector<ArtifactPosition> artPack;
@@ -3767,7 +3741,7 @@ bool CGameHandler::sacrificeArtifact(const IMarket * market, const CGHeroInstanc
 
 	for(const auto & artInstId : arts)
 	{
-		if(auto art = artSet->getArtByInstanceId(artInstId))
+		if(const auto * art = artSet->getArtByInstanceId(artInstId))
 		{
 			if(art->getType()->isTradable())
 			{
@@ -3796,7 +3770,7 @@ bool CGameHandler::sacrificeArtifact(const IMarket * market, const CGHeroInstanc
 
 bool CGameHandler::insertNewStack(const StackLocation &sl, const CCreature *c, TQuantity count)
 {
-	auto army = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl.army));
+	const auto * army = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl.army));
 
 	if (army->hasStackAtSlot(sl.slot))
 		COMPLAIN_RET("Slot is already taken!");
@@ -3815,7 +3789,7 @@ bool CGameHandler::insertNewStack(const StackLocation &sl, const CCreature *c, T
 
 bool CGameHandler::eraseStack(const StackLocation &sl, bool forceRemoval)
 {
-	auto army = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl.army));
+	const auto * army = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl.army));
 
 	if (!army->hasStackAtSlot(sl.slot))
 		COMPLAIN_RET("Cannot find a stack to erase");
@@ -3836,7 +3810,7 @@ bool CGameHandler::eraseStack(const StackLocation &sl, bool forceRemoval)
 
 bool CGameHandler::changeStackCount(const StackLocation &sl, TQuantity count, ChangeValueMode mode)
 {
-	auto army = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl.army));
+	const auto * army = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl.army));
 
 	TQuantity currentCount = army->getStackCount(sl.slot);
 	if ((mode == ChangeValueMode::ABSOLUTE && count < 0)
@@ -3864,7 +3838,7 @@ bool CGameHandler::changeStackCount(const StackLocation &sl, TQuantity count, Ch
 
 bool CGameHandler::addToSlot(const StackLocation &sl, const CCreature *c, TQuantity count)
 {
-	auto army = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl.army));
+	const auto * army = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl.army));
 
 	const CCreature *slotC = army->getCreature(sl.slot);
 	if (!slotC) //slot is empty
@@ -3913,8 +3887,8 @@ void CGameHandler::tryJoiningArmy(const CArmedInstance *src, const CArmedInstanc
 
 bool CGameHandler::moveStack(const StackLocation &src, const StackLocation &dst, TQuantity count)
 {
-	auto srcArmy = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(src.army));
-	auto dstArmy = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(dst.army));
+	const auto * srcArmy = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(src.army));
+	const auto * dstArmy = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(dst.army));
 
 	if (!srcArmy->hasStackAtSlot(src.slot))
 		COMPLAIN_RET("No stack to move!");
@@ -3958,16 +3932,16 @@ void CGameHandler::castSpell(const spells::Caster * caster, SpellID spellID, con
 	p.pos = pos;
 
 	const CSpell * s = spellID.toSpell();
-	s->adventureCast(spellEnv, p);
+	s->adventureCast(spellEnv.get(), p);
 
-	if(const auto hero = caster->getHeroCaster())
+	if(const auto * hero = caster->getHeroCaster())
 		useChargedArtifactUsed(hero->id, spellID);
 }
 
 bool CGameHandler::swapStacks(const StackLocation & sl1, const StackLocation & sl2)
 {
-	auto army1 = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl1.army));
-	auto army2 = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl2.army));
+	const auto * army1 = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl1.army));
+	const auto * army2 = dynamic_cast<const CArmedInstance*>(gameInfo().getObj(sl2.army));
 
 	if(!army1->hasStackAtSlot(sl1.slot))
 	{
@@ -3991,11 +3965,11 @@ bool CGameHandler::swapStacks(const StackLocation & sl1, const StackLocation & s
 
 bool CGameHandler::putArtifact(const ArtifactLocation & al, const ArtifactInstanceID & id, std::optional<bool> askAssemble)
 {
-	const auto artInst = gameInfo().getArtInstance(id);
+	const auto * artInst = gameInfo().getArtInstance(id);
 	assert(artInst && artInst->getType());
 	ArtifactLocation dst(al.artHolder, ArtifactPosition::PRE_FIRST);
 	dst.creature = al.creature;
-	auto putTo = gameState().getArtSet(al);
+	const auto * putTo = gameState().getArtSet(al);
 	assert(putTo);
 
 	if(al.slot == ArtifactPosition::FIRST_AVAILABLE)
@@ -4076,12 +4050,12 @@ void CGameHandler::spawnWanderingMonsters(CreatureID creatureID)
 	std::vector<int3>::iterator tile;
 	std::vector<int3> tiles;
 	gameState().getFreeTiles(tiles);
-	ui32 amount = (ui32)tiles.size() / 200; //Chance is 0.5% for each tile
+	ui32 amount = tiles.size() / 200; //Chance is 0.5% for each tile
 
 	RandomGeneratorUtil::randomShuffle(tiles, getRandomGenerator());
 	logGlobal->trace("Spawning wandering monsters. Found %d free tiles. Creature type: %d", tiles.size(), creatureID.num);
 	const CCreature *cre = creatureID.toCreature();
-	for (int i = 0; i < (int)amount; ++i)
+	for (int i = 0; i < amount; ++i)
 	{
 		tile = tiles.begin();
 		logGlobal->trace("\tSpawning monster at %s", tile->toString());
@@ -4162,8 +4136,8 @@ void CGameHandler::changeFogOfWar(const std::unordered_set<int3> &tiles, PlayerC
 		// do not hide tiles observed by owned objects. May lead to disastrous AI problems
 		// FIXME: this leads to a bug - shroud of darkness from Necropolis does can not override Skyship from Tower
 		std::unordered_set<int3> observedTiles;
-		auto p = gameInfo().getPlayerState(player);
-		for (auto obj : p->getOwnedObjects())
+		const auto * p = gameInfo().getPlayerState(player);
+		for (const auto * obj : p->getOwnedObjects())
 			gameInfo().getTilesInRange(observedTiles, obj->getSightCenter(), obj->getSightRadius(), ETileVisibility::REVEALED, obj->getOwner());
 
 		for (auto tile : observedTiles)
@@ -4357,7 +4331,7 @@ void CGameHandler::startBattle(const CArmedInstance *army1, const CArmedInstance
 
 void CGameHandler::useChargedArtifactUsed(const ObjectInstanceID & heroObjectID, const SpellID & spellID)
 {
-	const auto hero = gameInfo().getHero(heroObjectID);
+	const auto * hero = gameInfo().getHero(heroObjectID);
 	assert(hero);
 	assert(hero->canCastThisSpell(spellID.toSpell()));
 
@@ -4367,8 +4341,8 @@ void CGameHandler::useChargedArtifactUsed(const ObjectInstanceID & heroObjectID,
 	std::vector<std::pair<ArtifactPosition, ArtifactInstanceID>> chargedArts;
 	for(const auto & [slot, slotInfo] : hero->artifactsWorn)
 	{
-		const auto artInst = slotInfo.getArt();
-		const auto artType = artInst->getType();
+		const auto * artInst = slotInfo.getArt();
+		const auto * artType = artInst->getType();
 		if(artType->getDischargeCondition() == DischargeArtifactCondition::SPELLCAST)
 		{
 			chargedArts.emplace_back(slot, artInst->getId());

server/CGameHandler.h

@@ -28,6 +28,7 @@ class EVictoryLossCheckResult;
 class CRandomGenerator;
 class GameRandomizer;
 class StatisticDataSet;
+class ServerCallback;
 
 struct StartInfo;
 struct TerrainTile;
@@ -70,6 +71,7 @@ public:
 	std::unique_ptr<NewTurnProcessor> newTurnProcessor;
 	std::unique_ptr<GameRandomizer> randomizer;
 	std::unique_ptr<StatisticDataSet> statistics;
+	std::unique_ptr<SpellCastEnvironment> spellEnv;
 	std::shared_ptr<CGameState> gs;
 
 	//use enums as parameters, because doMove(sth, true, false, true) is not readable
@@ -82,9 +84,8 @@ public:
 	std::map<PlayerColor, std::set<std::shared_ptr<CConnection>>> connections; //player color -> connection to client with interface of that player
 
 	//queries stuff
-	ui32 QID;
+	QueryID QID;
 
-	SpellCastEnvironment * spellEnv;
 
 	const Services * services() const override;
 	const BattleCb * battle(const BattleID & battleID) const override;
@@ -92,6 +93,7 @@ public:
 	vstd::CLoggerBase * logger() const override;
 	events::EventBus * eventBus() const override;
 	CVCMIServer & gameLobby() const;
+	ServerCallback * spellcastEnvironment() const;
 
 	bool isBlockedByQueries(const CPackForServer *pack, PlayerColor player);
 	bool isAllowedExchange(ObjectInstanceID id1, ObjectInstanceID id2);
@@ -151,9 +153,9 @@ public:
 	void removeArtifact(const ObjectInstanceID & srcId, const std::vector<ArtifactPosition> & slotsPack);
 	bool moveArtifact(const PlayerColor & player, const ArtifactLocation & src, const ArtifactLocation & dst) override;
 	bool bulkMoveArtifacts(const PlayerColor & player, ObjectInstanceID srcId, ObjectInstanceID dstId, bool swap, bool equipped, bool backpack);
-	bool manageBackpackArtifacts(const PlayerColor & player, const ObjectInstanceID heroID, const ManageBackpackArtifacts::ManageCmd & sortType);
-	bool saveArtifactsCostume(const PlayerColor & player, const ObjectInstanceID heroID, uint32_t costumeIdx);
-	bool switchArtifactsCostume(const PlayerColor & player, const ObjectInstanceID heroID, uint32_t costumeIdx);
+	bool manageBackpackArtifacts(const PlayerColor & player, const ObjectInstanceID & heroID, const ManageBackpackArtifacts::ManageCmd & sortType);
+	bool saveArtifactsCostume(const PlayerColor & player, const ObjectInstanceID & heroID, uint32_t costumeIdx);
+	bool switchArtifactsCostume(const PlayerColor & player, const ObjectInstanceID & heroID, uint32_t costumeIdx);
 	bool eraseArtifactByClient(const ArtifactLocation & al);
 
 	void heroVisitCastle(const CGTownInstance * obj, const CGHeroInstance * hero) override;
@@ -191,7 +193,7 @@ public:
 	void visitObjectOnTile(const TerrainTile &t, const CGHeroInstance * h);
 	bool teleportHero(ObjectInstanceID hid, ObjectInstanceID dstid, ui8 source, PlayerColor asker = PlayerColor::NEUTRAL);
 	void visitCastleObjects(const CGTownInstance * obj, const CGHeroInstance * hero) override;
-	void visitCastleObjects(const CGTownInstance * obj, std::vector<const CGHeroInstance * > visitors);
+	void visitCastleObjects(const CGTownInstance * obj, const std::vector<const CGHeroInstance * > & visitors);
 	void levelUpHero(const CGHeroInstance * hero, SecondarySkill skill);//handle client respond and send one more request if needed
 	void levelUpHero(const CGHeroInstance * hero);//initial call - check if hero have remaining levelups & handle them
 	void levelUpCommander (const CCommanderInstance * c, int skill); //secondary skill 1 to 6, special skill : skill - 100
@@ -201,9 +203,9 @@ public:
 	//////////////////////////////////////////////////////////////////////////
 
 	void init(StartInfo *si, Load::ProgressAccumulator & progressTracking);
-	void handleClientDisconnection(std::shared_ptr<CConnection> c);
+	void handleClientDisconnection(const std::shared_ptr<CConnection> & c);
 	void handleReceivedPack(std::shared_ptr<CConnection> c, CPackForServer & pack);
-	bool hasPlayerAt(PlayerColor player, std::shared_ptr<CConnection> c) const;
+	bool hasPlayerAt(PlayerColor player, const std::shared_ptr<CConnection> & c) const;
 	bool hasBothPlayersAtSameConnection(PlayerColor left, PlayerColor right) const;
 
 	bool queryReply( QueryID qid, std::optional<int32_t> reply, PlayerColor player );
@@ -274,7 +276,7 @@ public:
 #endif
 	}
 
-	void sendToAllClients(CPackForClient & pack);
+	void sendToAllClients(const CPackForClient & pack);
 	void sendAndApply(CPackForClient & pack) override;
 	void sendAndApply(CGarrisonOperationPack & pack);
 	void sendAndApply(SetResources & pack);
@@ -290,7 +292,7 @@ public:
 	/// Throws if player is not present on connection of this pack
 	void throwIfWrongPlayer(const std::shared_ptr<CConnection> & connection, const CPackForServer * pack, PlayerColor player);
 	void throwIfWrongPlayer(const std::shared_ptr<CConnection> & connection, const CPackForServer * pack);
-	[[noreturn]] void throwAndComplain(const std::shared_ptr<CConnection> & connection, std::string txt);
+	[[noreturn]] void throwAndComplain(const std::shared_ptr<CConnection> & connection, const std::string & txt);
 
 	bool isPlayerOwns(const std::shared_ptr<CConnection> & connection, const CPackForServer * pack, ObjectInstanceID id);
 

server/CVCMIServer.cpp

@@ -15,13 +15,11 @@
 #include "LobbyNetPackVisitors.h"
 #include "processors/PlayerMessageProcessor.h"
 
-#include "../lib/CPlayerState.h"
 #include "../lib/CThreadHelper.h"
 #include "../lib/campaign/CampaignState.h"
 #include "../lib/entities/hero/CHeroHandler.h"
 #include "../lib/entities/hero/CHeroClass.h"
 #include "../lib/gameState/CGameState.h"
-#include "../lib/mapping/CMapDefines.h"
 #include "../lib/mapping/CMapInfo.h"
 #include "../lib/mapping/CMapHeader.h"
 #include "../lib/rmg/CMapGenOptions.h"
@@ -418,7 +416,7 @@ void CVCMIServer::clientConnected(std::shared_ptr<CConnection> c, std::vector<st
 		ClientPlayer cp;
 		cp.connection = c->connectionID;
 		cp.name = name;
-		playerNames.insert(std::make_pair(id, cp));
+		playerNames.try_emplace(id, cp);
 		announceTxt(boost::str(boost::format("%s (pid %d cid %d) joins the game") % name % id % c->connectionID));
 
 		//put new player in first slot with AI
@@ -618,9 +616,9 @@ void CVCMIServer::setPlayer(PlayerColor clickedColor)
 		int id;
 		void reset() { id = -1; color = PlayerColor::CANNOT_DETERMINE; }
 		PlayerToRestore(){ reset(); }
-	} playerToRestore;
-
+	};
 
+	PlayerToRestore playerToRestore;
 	PlayerSettings & clicked = si->playerInfos[clickedColor];
 
 	//identify clicked player
@@ -675,7 +673,7 @@ void CVCMIServer::setPlayer(PlayerColor clickedColor)
 	}
 }
 
-void CVCMIServer::setPlayerName(PlayerColor color, std::string name)
+void CVCMIServer::setPlayerName(PlayerColor color, const std::string & name)
 {
 	if(color == PlayerColor::CANNOT_DETERMINE)
 		return;
@@ -902,29 +900,29 @@ HeroTypeID CVCMIServer::nextAllowedHero(PlayerColor player, HeroTypeID initial,
 void CVCMIServer::optionNextBonus(PlayerColor player, int dir)
 {
 	PlayerSettings & s = si->playerInfos[player];
-	PlayerStartingBonus & ret = s.bonus = static_cast<PlayerStartingBonus>(static_cast<int>(s.bonus) + dir);
+	s.bonus = static_cast<PlayerStartingBonus>(static_cast<int>(s.bonus) + dir);
 
 	if(s.hero == HeroTypeID::NONE &&
-		!getPlayerInfo(player).heroesNames.size() &&
-		ret == PlayerStartingBonus::ARTIFACT) //no hero - can't be artifact
+		getPlayerInfo(player).heroesNames.empty() &&
+		s.bonus == PlayerStartingBonus::ARTIFACT) //no hero - can't be artifact
 	{
 		if(dir < 0)
-			ret = PlayerStartingBonus::RANDOM;
+			s.bonus = PlayerStartingBonus::RANDOM;
 		else
-			ret = PlayerStartingBonus::GOLD;
+			s.bonus = PlayerStartingBonus::GOLD;
 	}
 
-	if(ret > PlayerStartingBonus::RESOURCE)
-		ret = PlayerStartingBonus::RANDOM;
-	if(ret < PlayerStartingBonus::RANDOM)
-		ret = PlayerStartingBonus::RESOURCE;
+	if(s.bonus > PlayerStartingBonus::RESOURCE)
+		s.bonus = PlayerStartingBonus::RANDOM;
+	if(s.bonus < PlayerStartingBonus::RANDOM)
+		s.bonus = PlayerStartingBonus::RESOURCE;
 
-	if(s.castle == FactionID::RANDOM && ret == PlayerStartingBonus::RESOURCE) //random castle - can't be resource
+	if(s.castle == FactionID::RANDOM && s.bonus == PlayerStartingBonus::RESOURCE) //random castle - can't be resource
 	{
 		if(dir < 0)
-			ret = PlayerStartingBonus::GOLD;
+			s.bonus = PlayerStartingBonus::GOLD;
 		else
-			ret = PlayerStartingBonus::RANDOM;
+			s.bonus = PlayerStartingBonus::RANDOM;
 	}
 }
 
@@ -975,10 +973,10 @@ bool CVCMIServer::canUseThisHero(PlayerColor player, HeroTypeID ID)
 std::vector<HeroTypeID> CVCMIServer::getUsedHeroes()
 {
 	std::vector<HeroTypeID> heroIds;
-	for(auto & p : si->playerInfos)
+	for(const auto & p : si->playerInfos)
 	{
 		const auto & heroes = getPlayerInfo(p.first).heroesNames;
-		for(auto & hero : heroes)
+		for(const auto & hero : heroes)
 			if(hero.heroId.hasValue())
 				heroIds.push_back(hero.heroId);
 

server/CVCMIServer.h

@@ -114,7 +114,7 @@ public:
 
 	// Work with LobbyInfo
 	void setPlayer(PlayerColor clickedColor);
-	void setPlayerName(PlayerColor player, std::string name);
+	void setPlayerName(PlayerColor player, const std::string & name);
 	void setPlayerHandicap(PlayerColor player, Handicap handicap);
 	void optionNextHero(PlayerColor player, int dir); //dir == -1 or +1
 	void optionSetHero(PlayerColor player, HeroTypeID id);

server/NetPacksLobbyServer.cpp

@@ -14,15 +14,12 @@
 #include "CGameHandler.h"
 
 #include "../lib/StartInfo.h"
-#include "../lib/CRandomGenerator.h"
 
 #include "../lib/campaign/CampaignState.h"
 #include "../lib/entities/faction/CTownHandler.h"
-#include "../lib/entities/faction/CFaction.h"
 #include "../lib/gameState/CGameState.h"
 #include "../lib/serializer/Connection.h"
 #include "../lib/mapping/CMapInfo.h"
-#include "../lib/mapping/CMapHeader.h"
 #include "../lib/filesystem/Filesystem.h"
 
 void ClientPermissionsCheckerNetPackVisitor::visitForLobby(CPackForLobby & pack)
@@ -66,17 +63,6 @@ void ApplyOnServerNetPackVisitor::visitLobbyClientConnected(LobbyClientConnected
 void ApplyOnServerAfterAnnounceNetPackVisitor::visitLobbyClientConnected(LobbyClientConnected & pack)
 {
 	srv.updateAndPropagateLobbyState();
-
-// FIXME: what is this??? We do NOT support reconnection into ongoing game - at the very least queries and battles are NOT serialized
-//	if(srv.getState() == EServerState::GAMEPLAY)
-//	{
-//		//immediately start game
-//		std::unique_ptr<LobbyStartGame> startGameForReconnectedPlayer(new LobbyStartGame);
-//		startGameForReconnectedPlayer->initializedStartInfo = srv.si;
-//		startGameForReconnectedPlayer->initializedGameState = srv.gh->gameState();
-//		startGameForReconnectedPlayer->clientId = pack.c->connectionID;
-//		srv.announcePack(std::move(startGameForReconnectedPlayer));
-//	}
 }
 
 void ClientPermissionsCheckerNetPackVisitor::visitLobbyClientDisconnected(LobbyClientDisconnected & pack)
@@ -145,9 +131,8 @@ void ApplyOnServerNetPackVisitor::visitLobbySetCampaign(LobbySetCampaign & pack)
 	bool isCurrentMapConquerable = pack.ourCampaign->currentScenario() && pack.ourCampaign->isAvailable(*pack.ourCampaign->currentScenario());
 
 	auto scenarios = pack.ourCampaign->allScenarios();
-	for(std::set<CampaignScenarioID>::reverse_iterator itr = scenarios.rbegin(); itr != scenarios.rend(); itr++) // reverse -> on multiple scenario selection set lowest id at the end
+	for(auto scenarioID : boost::adaptors::reverse(scenarios)) // reverse -> on multiple scenario selection set lowest id at the end
 	{
-		auto scenarioID = *itr;
 		if(pack.ourCampaign->isAvailable(scenarioID))
 		{
 			if(!isCurrentMapConquerable || (isCurrentMapConquerable && scenarioID == *pack.ourCampaign->currentScenario()))
@@ -213,7 +198,7 @@ void ApplyOnServerNetPackVisitor::visitLobbyStartGame(LobbyStartGame & pack)
 	{
 		srv.verifyStateBeforeStart(true);
 	}
-	catch(...)
+	catch(const std::exception &)
 	{
 		result = false;
 		return;
@@ -302,7 +287,7 @@ void ApplyOnServerNetPackVisitor::visitLobbyChangePlayerOption(LobbyChangePlayer
 		srv.optionNextHero(pack.color, pack.value);
 		break;
 	case LobbyChangePlayerOption::BONUS_ID:
-		srv.optionSetBonus(pack.color, PlayerStartingBonus(pack.value));
+		srv.optionSetBonus(pack.color, static_cast<PlayerStartingBonus>(pack.value));
 		break;
 	case LobbyChangePlayerOption::BONUS:
 		srv.optionNextBonus(pack.color, pack.value);

server/NetPacksServer.cpp

@@ -23,7 +23,6 @@
 #include "../lib/mapObjects/CGHeroInstance.h"
 #include "../lib/gameState/CGameState.h"
 #include "../lib/battle/IBattleState.h"
-#include "../lib/battle/BattleAction.h"
 #include "../lib/battle/Unit.h"
 #include "../lib/spells/CSpellHandler.h"
 #include "../lib/spells/ISpellMechanics.h"
@@ -229,7 +228,6 @@ void ApplyGhNetPackVisitor::visitManageEquippedArtifacts(ManageEquippedArtifacts
 void ApplyGhNetPackVisitor::visitAssembleArtifacts(AssembleArtifacts & pack)
 {
 	gh.throwIfWrongOwner(connection, &pack, pack.heroID);
-	// gh.throwIfPlayerNotActive(&pack); // Might happen when player captures artifacts in battle?
 	result = gh.assembleArtifacts(pack.heroID, pack.artifactSlot, pack.assemble, pack.assembleTo);
 }
 
@@ -433,7 +431,7 @@ void ApplyGhNetPackVisitor::visitCastAdvSpell(CastAdvSpell & pack)
 	p.pos = pack.pos;
 
 	const CSpell * s = pack.sid.toSpell();
-	result = s->adventureCast(gh.spellEnv, p);
+	result = s->adventureCast(gh.spellEnv.get(), p);
 }
 
 void ApplyGhNetPackVisitor::visitPlayerMessage(PlayerMessage & pack)

server/TurnTimerHandler.cpp

@@ -99,10 +99,10 @@ void TurnTimerHandler::update(int waitTimeMs)
 	// create copy for iterations - battle might end during onBattleLoop call
 	std::vector<BattleID> ongoingBattles;
 
-	for (auto & battle : gameHandler.gameState().currentBattles)
+	for (const auto & battle : gameHandler.gameState().currentBattles)
 		ongoingBattles.push_back(battle->battleID);
 
-	for (auto & battleID : ongoingBattles)
+	for (const auto & battleID : ongoingBattles)
 		onBattleLoop(battleID, waitTimeMs);
 }
 

server/battles/BattleActionProcessor.cpp

@@ -14,16 +14,15 @@
 
 #include "../CGameHandler.h"
 
-#include "../../lib/texts/CGeneralTextHandler.h"
 #include "../../lib/CStack.h"
 #include "../../lib/IGameSettings.h"
 #include "../../lib/battle/CBattleInfoCallback.h"
 #include "../../lib/battle/CObstacleInstance.h"
 #include "../../lib/battle/IBattleState.h"
 #include "../../lib/battle/BattleAction.h"
+#include "../../lib/callback/IGameInfoCallback.h"
 #include "../../lib/callback/GameRandomizer.h"
 #include "../../lib/entities/building/TownFortifications.h"
-#include "../../lib/gameState/CGameState.h"
 #include "../../lib/networkPacks/PacksForClientBattle.h"
 #include "../../lib/networkPacks/SetStackEffect.h"
 #include "../../lib/spells/AbilityCaster.h"
@@ -119,12 +118,12 @@ bool BattleActionProcessor::doHeroSpellAction(const CBattleInfoCallback & battle
 		logGlobal->warn("Spell cannot be cast!");
 		std::vector<std::string> texts;
 		problem.getAll(texts);
-		for(auto s : texts)
-			logGlobal->warn(s);
+		for(const auto & text : texts)
+			logGlobal->warn(text);
 		return false;
 	}
 
-	parameters.cast(gameHandler->spellEnv, ba.getTarget(&battle));
+	parameters.cast(gameHandler->spellcastEnvironment(), ba.getTarget(&battle));
 	gameHandler->useChargedArtifactUsed(h->id, ba.spell);
 
 	return true;
@@ -138,7 +137,7 @@ bool BattleActionProcessor::doWalkAction(const CBattleInfoCallback & battle, con
 	if (!canStackAct(battle, stack))
 		return false;
 
-	if(target.size() < 1)
+	if(target.empty())
 	{
 		gameHandler->complain("Destination required for move action.");
 		return false;
@@ -188,7 +187,7 @@ bool BattleActionProcessor::doDefendAction(const CBattleInfoCallback & battle, c
 
 	buffer.push_back(bonus2);
 
-	sse.toUpdate.push_back(std::make_pair(ba.stackNumber, buffer));
+	sse.toUpdate.emplace_back(ba.stackNumber, buffer);
 	gameHandler->sendAndApply(sse);
 
 	BattleLogMessage message;
@@ -320,8 +319,8 @@ bool BattleActionProcessor::doAttackAction(const CBattleInfoCallback & battle, c
 		&& startingPos == target.at(2).hexValue
 		&& stack->alive())
 	{
+		assert(stack->unitId() == ba.stackNumber);
 		moveStack(battle, ba.stackNumber, startingPos);
-		//NOTE: curStack->unitId() == ba.stackNumber (rev 1431)
 	}
 	return true;
 }
@@ -334,7 +333,7 @@ bool BattleActionProcessor::doShootAction(const CBattleInfoCallback & battle, co
 	if (!canStackAct(battle, stack))
 		return false;
 
-	if(target.size() < 1)
+	if(target.empty())
 	{
 		gameHandler->complain("Destination required for shot action.");
 		return false;
@@ -421,7 +420,7 @@ bool BattleActionProcessor::doCatapultAction(const CBattleInfoCallback & battle,
 		spells::BattleCast parameters(&battle, stack, spells::Mode::SPELL_LIKE_ATTACK, spell); //We can shot infinitely by catapult
 		auto shotLevel = stack->valOfBonuses(Selector::typeSubtype(BonusType::CATAPULT_EXTRA_SHOTS, catapultAbility->subtype));
 		parameters.setSpellLevel(shotLevel);
-		parameters.cast(gameHandler->spellEnv, target);
+		parameters.cast(gameHandler->spellcastEnvironment(), target);
 	}
 	return true;
 }
@@ -479,7 +478,7 @@ bool BattleActionProcessor::doUnitSpellAction(const CBattleInfoCallback & battle
 	if(randSpellcaster)
 		vstd::amax(spellLvl, randSpellcaster->val);
 	parameters.setSpellLevel(spellLvl);
-	parameters.cast(gameHandler->spellEnv, target);
+	parameters.cast(gameHandler->spellcastEnvironment(), target);
 	return true;
 }
 
@@ -491,7 +490,7 @@ bool BattleActionProcessor::doHealAction(const CBattleInfoCallback & battle, con
 	if (!canStackAct(battle, stack))
 		return false;
 
-	if(target.size() < 1)
+	if(target.empty())
 	{
 		gameHandler->complain("Destination required for heal action.");
 		return false;
@@ -515,7 +514,7 @@ bool BattleActionProcessor::doHealAction(const CBattleInfoCallback & battle, con
 		spells::BattleCast parameters(&battle, stack, spells::Mode::SPELL_LIKE_ATTACK, spell); //We can heal infinitely by first aid tent
 		auto dest = battle::Destination(destStack, target.at(0).hexValue);
 		parameters.setSpellLevel(0);
-		parameters.cast(gameHandler->spellEnv, {dest});
+		parameters.cast(gameHandler->spellcastEnvironment(), {dest});
 	}
 	return true;
 }
@@ -710,7 +709,7 @@ int BattleActionProcessor::moveStack(const CBattleInfoCallback & battle, int sta
 
 	if (curStack->hasBonusOfType(BonusType::FLYING))
 	{
-		if (path.second <= creSpeed && path.first.size() > 0)
+		if (path.second <= creSpeed && !path.first.empty())
 		{
 			if (canUseGate && dbState != EGateState::OPENED &&
 				occupyGateDrawbridgeHex(dest))
@@ -736,15 +735,16 @@ int BattleActionProcessor::moveStack(const CBattleInfoCallback & battle, int sta
 	else //for non-flying creatures
 	{
 		BattleHexArray tiles;
-		const int tilesToMove = std::max((int)(path.first.size() - creSpeed), 0);
-		int v = (int)path.first.size()-1;
+		const int tilesToMove = std::max<int>(path.first.size() - creSpeed, 0);
+		int v = static_cast<int>(path.first.size())-1;
 		path.first.insert(start);
 
 		// check if gate need to be open or closed at some point
-		BattleHex openGateAtHex, gateMayCloseAtHex;
+		BattleHex openGateAtHex;
+		BattleHex gateMayCloseAtHex;
 		if (canUseGate)
 		{
-			for (int i = (int)path.first.size()-1; i >= 0; i--)
+			for (int i = static_cast<int>(path.first.size())-1; i >= 0; i--)
 			{
 				auto needOpenGates = [&](const BattleHex & hex) -> bool
 				{
@@ -752,7 +752,7 @@ int BattleActionProcessor::moveStack(const CBattleInfoCallback & battle, int sta
 						return true;
 					if (hex == BattleHex::GATE_BRIDGE && i-1 >= 0 && path.first[i-1] == BattleHex::GATE_OUTER)
 						return true;
-					else if (hex == BattleHex::GATE_OUTER || hex == BattleHex::GATE_INNER)
+					if (hex == BattleHex::GATE_OUTER || hex == BattleHex::GATE_INNER)
 						return true;
 
 					return false;
@@ -950,10 +950,10 @@ void BattleActionProcessor::makeAttack(const CBattleInfoCallback & battle, const
 	if (gameHandler->randomizer->rollCombatAbility(ownerArmy, attacker->valOfBonuses(BonusType::DOUBLE_DAMAGE_CHANCE)))
 		bat.flags |= BattleAttack::DEATH_BLOW;
 
-	const auto * owner = battle.battleGetFightingHero(attacker->unitSide());
-	if(owner)
+	const auto * ownerHero = battle.battleGetFightingHero(attacker->unitSide());
+	if(ownerHero)
 	{
-		int chance = owner->valOfBonuses(BonusType::BONUS_DAMAGE_CHANCE, BonusSubtypeID(attacker->creatureId()));
+		int chance = ownerHero->valOfBonuses(BonusType::BONUS_DAMAGE_CHANCE, BonusSubtypeID(attacker->creatureId()));
 		if (gameHandler->randomizer->rollCombatAbility(ownerArmy, chance))
 			bat.flags |= BattleAttack::BALLISTA_DOUBLE_DMG;
 	}
@@ -984,7 +984,7 @@ void BattleActionProcessor::makeAttack(const CBattleInfoCallback & battle, const
 
 		//TODO: should spell override creature`s projectile?
 
-		auto spell = bat.spellID.toSpell();
+		const auto * spell = bat.spellID.toSpell();
 
 		battle::Target target;
 		target.emplace_back(defender, targetHex);
@@ -992,11 +992,11 @@ void BattleActionProcessor::makeAttack(const CBattleInfoCallback & battle, const
 		spells::BattleCast event(&battle, attacker, spells::Mode::SPELL_LIKE_ATTACK, spell);
 		event.setSpellLevel(bonus->val);
 
-		auto attackedCreatures = spell->battleMechanics(&event)->getAffectedStacks(target);
+		auto affectedStacks = spell->battleMechanics(&event)->getAffectedStacks(target);
 
 		//TODO: get exact attacked hex for defender
 
-		for(const CStack * stack : attackedCreatures)
+		for(const CStack * stack : affectedStacks)
 		{
 			if(stack != defender && stack->alive()) //do not hit same stack twice
 			{
@@ -1141,7 +1141,7 @@ void BattleActionProcessor::attackCasting(const CBattleInfoCallback & battle, bo
 				if (meleeRanged == CAddInfo::NONE || meleeRanged == 0 || (meleeRanged == 1 && ranged) || (meleeRanged == 2 && !ranged))
 					castMe = true;
 			}
-			int chance = attacker->valOfBonuses((Selector::typeSubtype(attackMode, BonusSubtypeID(spellID))));
+			int chance = attacker->valOfBonuses(Selector::typeSubtype(attackMode, BonusSubtypeID(spellID)));
 			vstd::amin(chance, 100);
 
 			const CSpell * spell = SpellID(spellID).toSpell();
@@ -1166,13 +1166,13 @@ void BattleActionProcessor::attackCasting(const CBattleInfoCallback & battle, bo
 			//casting
 			if(castMe)
 			{
-				parameters.cast(gameHandler->spellEnv, target);
+				parameters.cast(gameHandler->spellcastEnvironment(), target);
 			}
 		}
 	}
 }
 
-std::set<SpellID> BattleActionProcessor::getSpellsForAttackCasting(TConstBonusListPtr spells, const CStack *defender)
+std::set<SpellID> BattleActionProcessor::getSpellsForAttackCasting(const TConstBonusListPtr & spells, const CStack *defender)
 {
 	std::set<SpellID> spellsToCast;
 	constexpr int unlayeredItemsInternalLayer = -1;
@@ -1277,9 +1277,7 @@ void BattleActionProcessor::handleDeathStare(const CBattleInfoCallback & battle,
 
 	if(killedCreatures)
 	{
-		//TODO: death stare or accurate shot was not originally available for multiple-hex attacks, but...
-
-		SpellID spellID = SpellID(SpellID::DEATH_STARE); //also used as fallback spell for ACCURATE_SHOT
+		SpellID spellID(SpellID::DEATH_STARE); //also used as fallback spell for ACCURATE_SHOT
 		auto bonus = attacker->getBonus(Selector::typeSubtype(BonusType::DEATH_STARE, subtype));
 		if(bonus && bonus->additionalInfo[0] != SpellID::NONE)
 			spellID = SpellID(bonus->additionalInfo[0]);
@@ -1291,7 +1289,7 @@ void BattleActionProcessor::handleDeathStare(const CBattleInfoCallback & battle,
 		spells::Target target;
 		target.emplace_back(defender);
 		parameters.setEffectValue(killedCreatures);
-		parameters.cast(gameHandler->spellEnv, target);
+		parameters.cast(gameHandler->spellcastEnvironment(), target);
 	}
 }
 
@@ -1335,7 +1333,7 @@ void BattleActionProcessor::handleAfterAttackCasting(const CBattleInfoCallback &
 		target.emplace_back(defender);
 
 		parameters.setEffectValue(acidDamage * attacker->getCount());
-		parameters.cast(gameHandler->spellEnv, target);
+		parameters.cast(gameHandler->spellcastEnvironment(), target);
 	}
 
 
@@ -1365,9 +1363,9 @@ void BattleActionProcessor::handleAfterAttackCasting(const CBattleInfoCallback &
 		else
 			resurrectInfo.type = attacker->creatureId();
 
-		if(attacker->hasBonusOfType((BonusType::TRANSMUTATION), BonusCustomSubtype::transmutationPerHealth))
+		if(attacker->hasBonusOfType(BonusType::TRANSMUTATION, BonusCustomSubtype::transmutationPerHealth))
 			resurrectInfo.count = std::max((defender->getCount() * defender->getMaxHealth()) / resurrectInfo.type.toCreature()->getMaxHealth(), 1u);
-		else if (attacker->hasBonusOfType((BonusType::TRANSMUTATION), BonusCustomSubtype::transmutationPerUnit))
+		else if (attacker->hasBonusOfType(BonusType::TRANSMUTATION, BonusCustomSubtype::transmutationPerUnit))
 			resurrectInfo.count = defender->getCount();
 		else
 			return; //wrong subtype
@@ -1399,7 +1397,7 @@ void BattleActionProcessor::handleAfterAttackCasting(const CBattleInfoCallback &
 		{
 			chanceToTrigger = attacker->valOfBonuses(BonusType::DESTRUCTION, BonusCustomSubtype::destructionKillPercentage);
 			int percentageToDie = attacker->getBonus(Selector::type()(BonusType::DESTRUCTION).And(Selector::subtype()(BonusCustomSubtype::destructionKillPercentage)))->additionalInfo[0];
-			amountToDie = static_cast<int>(defender->getCount() * percentageToDie * 0.01f);
+			amountToDie = defender->getCount() * percentageToDie / 100;
 		}
 		else if(attacker->hasBonusOfType(BonusType::DESTRUCTION, BonusCustomSubtype::destructionKillAmount)) //killing by count
 		{
@@ -1485,7 +1483,7 @@ void BattleActionProcessor::applyBattleEffects(const CBattleInfoCallback & battl
 	{
 		//TODO: use damage with bonus but without penalties
 		auto fireShieldDamage = (std::min<int64_t>(def->getAvailableHealth(), bsa.damageAmount) * def->valOfBonuses(BonusType::FIRE_SHIELD)) / 100;
-		fireShield.push_back(std::make_pair(def, fireShieldDamage));
+		fireShield.emplace_back(def, fireShieldDamage);
 	}
 }
 
@@ -1596,7 +1594,7 @@ bool BattleActionProcessor::makePlayerBattleAction(const CBattleInfoCallback & b
 	}
 	else
 	{
-		auto active = battle.battleActiveUnit();
+		const auto * active = battle.battleActiveUnit();
 		if(!active)
 		{
 			gameHandler->complain("No active unit in battle!");

server/battles/BattleActionProcessor.h

@@ -8,7 +8,7 @@
  *
  */
 #pragma once
-#include "bonuses/BonusList.h"
+#include "bonuses/Bonus.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -51,7 +51,7 @@ class BattleActionProcessor : boost::noncopyable
 	void handleAfterAttackCasting(const CBattleInfoCallback & battle, bool ranged, const CStack * attacker, const CStack * defender);
 	void attackCasting(const CBattleInfoCallback & battle, bool ranged, BonusType attackMode, const battle::Unit * attacker, const CStack * defender);
 
-	std::set<SpellID> getSpellsForAttackCasting(TConstBonusListPtr spells, const CStack *defender);
+	std::set<SpellID> getSpellsForAttackCasting(const TConstBonusListPtr & spells, const CStack *defender);
 
 	// damage, drain life & fire shield; returns amount of drained life
 	void applyBattleEffects(const CBattleInfoCallback & battle, BattleAttack & bat, std::shared_ptr<battle::CUnitState> attackerState, FireShieldInfo & fireShield, const CStack * def, battle::HealInfo & healInfo, int distance, bool secondary) const;

server/battles/BattleFlowProcessor.cpp

@@ -16,12 +16,10 @@
 #include "../TurnTimerHandler.h"
 
 #include "../../lib/CStack.h"
-#include "../../lib/IGameSettings.h"
 #include "../../lib/battle/CBattleInfoCallback.h"
 #include "../../lib/battle/IBattleState.h"
 #include "../../lib/callback/GameRandomizer.h"
 #include "../../lib/entities/building/TownFortifications.h"
-#include "../../lib/gameState/CGameState.h"
 #include "../../lib/mapObjects/CGTownInstance.h"
 #include "../../lib/networkPacks/PacksForClientBattle.h"
 #include "../../lib/spells/BonusCaster.h"
@@ -129,7 +127,7 @@ void BattleFlowProcessor::tryPlaceMoats(const CBattleInfoCallback & battle)
 		auto moatCaster = spells::SilentCaster(battle.sideToPlayer(BattleSide::DEFENDER), actualCaster);
 		auto cast = spells::BattleCast(&battle, &moatCaster, spells::Mode::PASSIVE, fortifications.moatSpell.toSpell());
 		auto target = spells::Target();
-		cast.cast(gameHandler->spellEnv, target);
+		cast.cast(gameHandler->spellcastEnvironment(), target);
 	}
 }
 
@@ -170,7 +168,7 @@ void BattleFlowProcessor::trySummonGuardians(const CBattleInfoCallback & battle,
 		{
 			battle::UnitInfo info;
 			info.id = battle.battleNextUnitId();
-			info.count =  std::max(1, (int)(stack->getCount() * 0.01 * summonInfo->val));
+			info.count =  std::max(1, stack->getCount() * summonInfo->val / 100);
 			info.type = creatureData;
 			info.side = stack->unitSide();
 			info.position = hex;
@@ -195,13 +193,13 @@ void BattleFlowProcessor::castOpeningSpells(const CBattleInfoCallback & battle)
 {
 	for(auto i : {BattleSide::ATTACKER, BattleSide::DEFENDER})
 	{
-		auto h = battle.battleGetFightingHero(i);
+		const auto * h = battle.battleGetFightingHero(i);
 		if (!h)
 			continue;
 
 		TConstBonusListPtr bl = h->getBonusesOfType(BonusType::OPENING_BATTLE_SPELL);
 
-		for (auto b : *bl)
+		for (const auto & b : *bl)
 		{
 			spells::BonusCaster caster(h, b);
 
@@ -211,7 +209,7 @@ void BattleFlowProcessor::castOpeningSpells(const CBattleInfoCallback & battle)
 			parameters.setSpellLevel(3);
 			parameters.setEffectDuration(b->val);
 			parameters.massive = true;
-			parameters.castIfPossible(gameHandler->spellEnv, spells::Target());
+			parameters.castIfPossible(gameHandler->spellcastEnvironment(), spells::Target());
 		}
 	}
 }
@@ -246,14 +244,14 @@ void BattleFlowProcessor::startNextRound(const CBattleInfoCallback & battle, boo
 
 	// operate on copy - removing obstacles will invalidate iterator on 'battle' container
 	auto obstacles = battle.battleGetAllObstacles();
-	for (auto &obstPtr : obstacles)
+	for (const auto & obstPtr : obstacles)
 	{
-		if (const SpellCreatedObstacle *sco = dynamic_cast<const SpellCreatedObstacle *>(obstPtr.get()))
-			if (sco->turnsRemaining == 0)
-				removeObstacle(battle, *obstPtr);
+		const auto * sco = dynamic_cast<const SpellCreatedObstacle *>(obstPtr.get());
+		if (sco && sco->turnsRemaining == 0)
+			removeObstacle(battle, *obstPtr);
 	}
 
-	for(auto stack : battle.battleGetAllStacks(true))
+	for(const auto * stack : battle.battleGetAllStacks(true))
 	{
 		if(stack->alive() && !isFirstRound)
 			stackEnchantedTrigger(battle, stack);
@@ -271,8 +269,8 @@ const CStack * BattleFlowProcessor::getNextStack(const CBattleInfoCallback & bat
 	if(q.front().empty())
 		return nullptr;
 
-	auto next = q.front().front();
-	const auto stack = dynamic_cast<const CStack *>(next);
+	const auto * next = q.front().front();
+	const auto * stack = dynamic_cast<const CStack *>(next);
 
 	// regeneration takes place before everything else but only during first turn attempt in each round
 	// also works under blind and similar effects
@@ -324,7 +322,7 @@ void BattleFlowProcessor::activateNextStack(const CBattleInfoCallback & battle)
 			return stack->ghostPending;
 		});
 
-		for(auto stack : pendingGhosts)
+		for(const auto * stack : pendingGhosts)
 			removeGhosts.changedStacks.emplace_back(stack->unitId(), UnitChanges::EOperation::REMOVE);
 
 		if(!removeGhosts.changedStacks.empty())
@@ -476,7 +474,7 @@ bool BattleFlowProcessor::tryMakeAutomaticAction(const CBattleInfoCallback & bat
 
 	if (next->unitType()->getId() == CreatureID::FIRST_AID_TENT)
 	{
-		TStacks possibleStacks = battle.battleGetStacksIf([=](const CStack * s)
+		TStacks possibleStacks = battle.battleGetStacksIf([&next](const CStack * s)
 		{
 			return s->unitOwner() == next->unitOwner() && s->canBeHealed();
 		});
@@ -612,7 +610,7 @@ void BattleFlowProcessor::makeStackDoNothing(const CBattleInfoCallback & battle,
 	makeAutomaticAction(battle, next, doNothing);
 }
 
-bool BattleFlowProcessor::makeAutomaticAction(const CBattleInfoCallback & battle, const CStack *stack, BattleAction &ba)
+bool BattleFlowProcessor::makeAutomaticAction(const CBattleInfoCallback & battle, const CStack *stack, const BattleAction &ba)
 {
 	BattleSetActiveStack bsa;
 	bsa.battleID = battle.getBattle()->getBattleID();
@@ -627,7 +625,7 @@ bool BattleFlowProcessor::makeAutomaticAction(const CBattleInfoCallback & battle
 void BattleFlowProcessor::stackEnchantedTrigger(const CBattleInfoCallback & battle, const CStack * st)
 {
 	auto bl = *(st->getBonusesOfType(BonusType::ENCHANTED));
-	for(auto b : bl)
+	for(const auto & b : bl)
 	{
 		if (!b->subtype.as<SpellID>().hasValue())
 			continue;
@@ -644,7 +642,7 @@ void BattleFlowProcessor::stackEnchantedTrigger(const CBattleInfoCallback & batt
 
 		if(val > 3)
 		{
-			for(auto s : battle.battleGetAllStacks())
+			for(const auto * s : battle.battleGetAllStacks())
 				if(battle.battleMatchOwner(st, s, true) && s->isValidTarget()) //all allied
 					target.emplace_back(s);
 		}
@@ -652,7 +650,7 @@ void BattleFlowProcessor::stackEnchantedTrigger(const CBattleInfoCallback & batt
 		{
 			target.emplace_back(st);
 		}
-		battleCast.applyEffects(gameHandler->spellEnv, target, false, true);
+		battleCast.applyEffects(gameHandler->spellcastEnvironment(), target, false, true);
 	}
 }
 
@@ -681,16 +679,13 @@ void BattleFlowProcessor::stackTurnTrigger(const CBattleInfoCallback & battle, c
 			BonusList bl = *(st->getBonusesOfType(BonusType::BIND_EFFECT));
 			auto adjacent = battle.battleAdjacentUnits(st);
 
-			for (auto b : bl)
+			for (const auto & b : bl)
 			{
 				if(b->additionalInfo != CAddInfo::NONE)
 				{
 					const CStack * stack = battle.battleGetStackByID(b->additionalInfo[0]); //binding stack must be alive and adjacent
-					if(stack)
-					{
-						if(vstd::contains(adjacent, stack)) //binding stack is still present
-							unbind = false;
-					}
+					if(stack && vstd::contains(adjacent, stack)) //binding stack is still present
+						unbind = false;
 				}
 				else
 				{
@@ -781,7 +776,7 @@ void BattleFlowProcessor::stackTurnTrigger(const CBattleInfoCallback & battle, c
 				parameters.setSpellLevel(bonus->val);
 
 				//todo: recheck effect level
-				if(parameters.castIfPossible(gameHandler->spellEnv, spells::Target(1, parameters.massive ? spells::Destination() : spells::Destination(st))))
+				if(parameters.castIfPossible(gameHandler->spellcastEnvironment(), spells::Target(1, parameters.massive ? spells::Destination() : spells::Destination(st))))
 				{
 					cast = true;
 

server/battles/BattleFlowProcessor.h

@@ -52,7 +52,7 @@ class BattleFlowProcessor : boost::noncopyable
 	void setActiveStack(const CBattleInfoCallback & battle, const battle::Unit * stack, BattleUnitTurnReason reason);
 
 	void makeStackDoNothing(const CBattleInfoCallback & battle, const CStack * next);
-	bool makeAutomaticAction(const CBattleInfoCallback & battle, const CStack * stack, BattleAction & ba); //used when action is taken by stack without volition of player (eg. unguided catapult attack)
+	bool makeAutomaticAction(const CBattleInfoCallback & battle, const CStack * stack, const BattleAction & ba); //used when action is taken by stack without volition of player (eg. unguided catapult attack)
 
 public:
 	explicit BattleFlowProcessor(BattleProcessor * owner, CGameHandler * newGameHandler);

server/battles/BattleProcessor.cpp

@@ -37,9 +37,9 @@
 
 BattleProcessor::BattleProcessor(CGameHandler * gameHandler)
 	: gameHandler(gameHandler)
-	, flowProcessor(std::make_unique<BattleFlowProcessor>(this, gameHandler))
 	, actionsProcessor(std::make_unique<BattleActionProcessor>(this, gameHandler))
-	, resultProcessor(std::make_unique<BattleResultProcessor>(this, gameHandler))
+	, flowProcessor(std::make_unique<BattleFlowProcessor>(this, gameHandler))
+	, resultProcessor(std::make_unique<BattleResultProcessor>(gameHandler))
 {
 }
 
@@ -113,7 +113,7 @@ void BattleProcessor::startBattle(const CArmedInstance *army1, const CArmedInsta
 	const auto * attackerInfo = gameHandler->gameInfo().getPlayerState(army1->getOwner(), false);
 	if(attackerInfo && !army2->getOwner().isValidPlayer())
 	{
-		for(auto bonus : attackerInfo->battleBonuses)
+		for(const auto & bonus : attackerInfo->battleBonuses)
 		{
 			GiveBonus giveBonus(GiveBonus::ETarget::OBJECT);
 			giveBonus.id = hero1->id;

server/battles/BattleProcessor.h

@@ -9,7 +9,8 @@
  */
 #pragma once
 
-#include "../../lib/GameConstants.h"
+#include "../../lib/constants/EntityIdentifiers.h"
+#include "../../lib/constants/Enumerations.h"
 #include "../../lib/battle/BattleSide.h"
 
 VCMI_LIB_NAMESPACE_BEGIN

server/battles/BattleResultProcessor.cpp

@@ -31,9 +31,8 @@
 
 #include <boost/lexical_cast.hpp>
 
-BattleResultProcessor::BattleResultProcessor(BattleProcessor * owner, CGameHandler * newGameHandler)
-//	: owner(owner)
-	: gameHandler(newGameHandler)
+BattleResultProcessor::BattleResultProcessor(CGameHandler * gameHandler)
+	: gameHandler(gameHandler)
 {
 }
 
@@ -62,7 +61,7 @@ CasualtiesAfterBattle::CasualtiesAfterBattle(const CBattleInfoCallback & battle,
 	{
 		// Use const cast - in order to call non-const "takeResurrected" for proper calculation of casualties
 		// TODO: better solution
-		CStack * st = const_cast<CStack*>(stConst);
+		auto * st = const_cast<CStack*>(stConst);
 
 		logGlobal->debug("Calculating casualties for %s", st->nodeName());
 
@@ -146,7 +145,7 @@ void CasualtiesAfterBattle::updateArmy(CGameHandler *gh)
 	if (gh->gameInfo().getObjInstance(army->id) == nullptr)
 		throw std::runtime_error("Object " + army->getObjectName() + " is not on the map!");
 
-	for (TStackAndItsNewCount &ncount : newStackCounts)
+	for (const auto & ncount : newStackCounts)
 	{
 		if (ncount.second > 0)
 			gh->changeStackCount(ncount.first, ncount.second, ChangeValueMode::ABSOLUTE);
@@ -263,7 +262,7 @@ void BattleResultProcessor::endBattle(const CBattleInfoCallback & battle)
 	battleQuery->result = std::make_optional(*battleResult);
 
 	//Check how many battle gameHandler->queries were created (number of players blocked by battle)
-	const int queriedPlayers = battleQuery ? (int)boost::count(gameHandler->queries->allQueries(), battleQuery) : 0;
+	const int queriedPlayers = battleQuery ? boost::count(gameHandler->queries->allQueries(), battleQuery) : 0;
 
 	assert(finishingBattles.count(battle.getBattle()->getBattleID()) == 0);
 	finishingBattles[battle.getBattle()->getBattleID()] = std::make_unique<FinishingBattleHelper>(battle, *battleResult, queriedPlayers);
@@ -310,8 +309,8 @@ void BattleResultProcessor::endBattleConfirm(const CBattleInfoCallback & battle)
 		return;
 	}
 
-	auto * battleResult = battleResults.at(battle.getBattle()->getBattleID()).get();
-	auto * finishingBattle = finishingBattles.at(battle.getBattle()->getBattleID()).get();
+	const auto * battleResult = battleResults.at(battle.getBattle()->getBattleID()).get();
+	const auto * finishingBattle = finishingBattles.at(battle.getBattle()->getBattleID()).get();
 
 	//calculate casualties before deleting battle
 	CasualtiesAfterBattle cab1(battle, BattleSide::ATTACKER);

server/battles/BattleResultProcessor.h

@@ -48,7 +48,8 @@ struct FinishingBattleHelper
 
 	ObjectInstanceID winnerId;
 	ObjectInstanceID loserId;
-	PlayerColor victor, loser;
+	PlayerColor victor;
+	PlayerColor loser;
 	BattleSide winnerSide;
 
 	int remainingBattleQueriesCount;
@@ -66,14 +67,13 @@ struct FinishingBattleHelper
 
 class BattleResultProcessor : boost::noncopyable
 {
-	//	BattleProcessor * owner;
 	CGameHandler * gameHandler;
 
 	std::map<BattleID, std::unique_ptr<BattleResult>> battleResults;
 	std::map<BattleID, std::unique_ptr<FinishingBattleHelper>> finishingBattles;
 
 public:
-	explicit BattleResultProcessor(BattleProcessor * owner, CGameHandler * newGameHandler);
+	explicit BattleResultProcessor(CGameHandler * gameHandler);
 
 	bool battleIsEnding(const CBattleInfoCallback & battle) const;
 

server/processors/HeroPoolProcessor.cpp

@@ -39,7 +39,7 @@ TavernHeroSlot HeroPoolProcessor::selectSlotForRole(const PlayerColor & player,
 	const auto & heroes = heroesPool->getHeroesFor(player);
 
 	// if tavern has empty slot - use it
-	if (heroes.size() == 0)
+	if (heroes.empty())
 		return TavernHeroSlot::NATIVE;
 
 	if (heroes.size() == 1)
@@ -320,7 +320,7 @@ const CHeroClass * HeroPoolProcessor::pickClassFor(bool isNative, const PlayerCo
 		if (isNative && heroClass->faction != factionID)
 			continue;
 
-		bool hasSameClass = vstd::contains_if(currentTavern, [&](const CGHeroInstance * hero){
+		bool hasSameClass = vstd::contains_if(currentTavern, [&heroClass](const CGHeroInstance * hero){
 			return hero->getHeroClass() == heroClass;
 		});
 

server/processors/NewTurnProcessor.cpp

@@ -17,7 +17,6 @@
 #include "../../lib/CPlayerState.h"
 #include "../../lib/IGameSettings.h"
 #include "../../lib/StartInfo.h"
-#include "../../lib/TerrainHandler.h"
 #include "../../lib/callback/GameRandomizer.h"
 #include "../../lib/constants/StringConstants.h"
 #include "../../lib/entities/building/CBuilding.h"
@@ -66,7 +65,7 @@ void NewTurnProcessor::handleTimeEvents(PlayerColor color)
 		//remove objects specified by event
 		for(const ObjectInstanceID objectIdToRemove : event.deletedObjectsInstances)
 		{
-			auto objectInstance = gameHandler->gameInfo().getObj(objectIdToRemove, false);
+			const auto * objectInstance = gameHandler->gameInfo().getObj(objectIdToRemove, false);
 			if(objectInstance != nullptr)
 				gameHandler->removeObject(objectInstance, PlayerColor::NEUTRAL);
 		}
@@ -244,7 +243,7 @@ ResourceSet NewTurnProcessor::generatePlayerIncome(PlayerColor playerID, bool ne
 	TResources incomeHandicapped = income;
 	incomeHandicapped.applyHandicap(playerSettings->handicap.percentIncome);
 
-	for (auto obj :	state.getOwnedObjects())
+	for (const auto * obj :	state.getOwnedObjects())
 		incomeHandicapped += obj->asOwnable()->dailyIncome();
 
 	if (!state.isHuman())
@@ -496,7 +495,7 @@ RumorState NewTurnProcessor::pickNewRumor()
 
 			case RumorState::TYPE_RAND:
 				auto vector = LIBRARY->generaltexth->findStringsWithPrefix("core.randtvrn");
-				rumorId = rand.nextInt((int)vector.size() - 1);
+				rumorId = rand.nextInt(static_cast<int>(vector.size()) - 1);
 
 				break;
 		}
@@ -510,7 +509,7 @@ std::tuple<EWeekType, CreatureID> NewTurnProcessor::pickWeekType(bool newMonth)
 {
 	for (const auto & townID : gameHandler->gameState().getMap().getAllTowns())
 	{
-		auto t = gameHandler->gameState().getTown(townID);
+		const auto * t = gameHandler->gameState().getTown(townID);
 		if (t->hasBuilt(BuildingID::GRAIL, ETownType::INFERNO))
 			return { EWeekType::DEITYOFFIRE, CreatureID::IMP };
 	}
@@ -528,7 +527,7 @@ std::tuple<EWeekType, CreatureID> NewTurnProcessor::pickWeekType(bool newMonth)
 				CreatureID creatureID = gameHandler->randomizer->rollCreature();
 				return { EWeekType::DOUBLE_GROWTH, creatureID};
 			}
-			else if (LIBRARY->creh->doubledCreatures.size())
+			else if (!LIBRARY->creh->doubledCreatures.empty())
 			{
 				CreatureID creatureID = *RandomGeneratorUtil::nextItem(LIBRARY->creh->doubledCreatures, gameHandler->getRandomGenerator());
 				return { EWeekType::DOUBLE_GROWTH, creatureID};
@@ -565,7 +564,7 @@ std::vector<SetMana> NewTurnProcessor::updateHeroesManaPoints()
 {
 	std::vector<SetMana> result;
 
-	for (auto & elem : gameHandler->gameState().players)
+	for (const auto & elem : gameHandler->gameState().players)
 	{
 		for (const CGHeroInstance *h : elem.second.getHeroes())
 		{
@@ -582,7 +581,7 @@ std::vector<SetMovePoints> NewTurnProcessor::updateHeroesMovementPoints()
 {
 	std::vector<SetMovePoints> result;
 
-	for (auto & elem : gameHandler->gameState().players)
+	for (const auto & elem : gameHandler->gameState().players)
 	{
 		for (const CGHeroInstance *h : elem.second.getHeroes())
 		{
@@ -626,12 +625,12 @@ InfoWindow NewTurnProcessor::createInfoWindow(EWeekType weekType, CreatureID cre
 		default:
 			if (newMonth)
 			{
-				iw.text.appendLocalString(EMetaText::ARRAY_TXT, (130));
+				iw.text.appendLocalString(EMetaText::ARRAY_TXT, 130);
 				iw.text.replaceLocalString(EMetaText::ARRAY_TXT, gameHandler->getRandomGenerator().nextInt(32, 41));
 			}
 			else
 			{
-				iw.text.appendLocalString(EMetaText::ARRAY_TXT, (133));
+				iw.text.appendLocalString(EMetaText::ARRAY_TXT, 133);
 				iw.text.replaceLocalString(EMetaText::ARRAY_TXT, gameHandler->getRandomGenerator().nextInt(43, 57));
 			}
 	}
@@ -669,16 +668,12 @@ NewTurn NewTurnProcessor::generateNewTurnPack()
 	{
 		for (const auto & townID : gameHandler->gameState().getMap().getAllTowns())
 		{
-			auto t = gameHandler->gameState().getTown(townID);
+			const auto * t = gameHandler->gameState().getTown(townID);
 			n.availableCreatures.push_back(generateTownGrowth(t, n.specialWeek, n.creatureid, firstTurn));
 		}
-	}
 
-	if (newWeek)
 		n.newRumor = pickNewRumor();
 
-	if (newWeek)
-	{
 		//new week info popup
 		if (n.specialWeek != EWeekType::FIRST_WEEK)
 			n.newWeekNotification = createInfoWindow(n.specialWeek, n.creatureid, newMonth);
@@ -701,9 +696,9 @@ void NewTurnProcessor::onNewTurn()
 	{
 		for (const auto & townID : gameHandler->gameState().getMap().getAllTowns())
 		{
-			auto t = gameHandler->gameState().getTown(townID);
+			const auto * t = gameHandler->gameState().getTown(townID);
 			if (t->hasBuilt(BuildingSubID::PORTAL_OF_SUMMONING))
-				gameHandler->setPortalDwelling(t, true, (n.specialWeek == EWeekType::PLAGUE ? true : false)); //set creatures for Portal of Summoning
+				gameHandler->setPortalDwelling(t, true, n.specialWeek == EWeekType::PLAGUE); //set creatures for Portal of Summoning
 		}
 	}
 
@@ -711,7 +706,7 @@ void NewTurnProcessor::onNewTurn()
 	{
 		for (const auto & townID : gameHandler->gameState().getMap().getAllTowns())
 		{
-			auto t = gameHandler->gameState().getTown(townID);
+			const auto * t = gameHandler->gameState().getTown(townID);
 			if (!t->getOwner().isValidPlayer())
 				updateNeutralTownGarrison(t, 1 + gameHandler->gameInfo().getDate(Date::DAY) / 7);
 		}

server/queries/BattleQueries.cpp

@@ -16,11 +16,10 @@
 #include "../battles/BattleProcessor.h"
 
 #include "../../lib/battle/IBattleState.h"
-#include "../../lib/battle/SideInBattle.h"
 #include "../../lib/battle/BattleLayout.h"
+#include "../../lib/battle/SideInBattle.h"
 #include "../../lib/CPlayerState.h"
 #include "../../lib/mapObjects/CGObjectInstance.h"
-#include "../../lib/mapObjects/CGTownInstance.h"
 #include "../../lib/networkPacks/PacksForServer.h"
 
 void CBattleQuery::notifyObjectAboutRemoval(const CGObjectInstance * visitedObject, const CGHeroInstance * visitingHero) const
@@ -77,7 +76,7 @@ void CBattleQuery::onExposure(QueryPtr topQuery)
 		owner->popQuery(*this);
 }
 
-CBattleDialogQuery::CBattleDialogQuery(CGameHandler * owner, const IBattleInfo * bi, std::optional<BattleResult> Br):
+CBattleDialogQuery::CBattleDialogQuery(CGameHandler * owner, const IBattleInfo * bi, const std::optional<BattleResult> & Br):
 	CDialogQuery(owner),
 	bi(bi),
 	result(Br)

server/queries/BattleQueries.h

@@ -28,7 +28,7 @@ public:
 	std::optional<BattleResult> result;
 
 	CBattleQuery(CGameHandler * owner);
-	CBattleQuery(CGameHandler * owner, const IBattleInfo * Bi); //TODO
+	CBattleQuery(CGameHandler * owner, const IBattleInfo * Bi);
 	void notifyObjectAboutRemoval(const CGObjectInstance * visitedObject, const CGHeroInstance * visitingHero) const override;
 	bool blocksPack(const CPackForServer *pack) const override;
 	void onRemoval(PlayerColor color) override;
@@ -42,6 +42,6 @@ class CBattleDialogQuery : public CDialogQuery
 	std::optional<BattleResult> result;
 
 public:
-	CBattleDialogQuery(CGameHandler * owner, const IBattleInfo * Bi, std::optional<BattleResult> Br);
+	CBattleDialogQuery(CGameHandler * owner, const IBattleInfo * Bi, const std::optional<BattleResult> & Br);
 	void onRemoval(PlayerColor color) override;
 };

server/queries/CQuery.cpp

@@ -30,9 +30,7 @@ CQuery::CQuery(CGameHandler * gameHandler)
 	: owner(gameHandler->queries.get())
 	, gh(gameHandler)
 {
-	static QueryID QID = QueryID(0);
-
-	queryID = ++QID;
+	queryID = ++gameHandler->QID;
 	logGlobal->trace("Created a new query with id %d", queryID);
 }
 
@@ -143,8 +141,8 @@ void CDialogQuery::setReply(std::optional<int32_t> reply)
 		answer = *reply;
 }
 
-CGenericQuery::CGenericQuery(CGameHandler * gh, PlayerColor color, std::function<void(std::optional<int32_t>)> Callback):
-	CQuery(gh), callback(Callback)
+CGenericQuery::CGenericQuery(CGameHandler * gh, PlayerColor color, const std::function<void(std::optional<int32_t>)> & callback):
+	CQuery(gh), callback(callback)
 {
 	addPlayer(color);
 }
@@ -164,9 +162,9 @@ void CGenericQuery::onExposure(QueryPtr topQuery)
 	//do nothing
 }
 
-void CGenericQuery::setReply(std::optional<int32_t> reply)
+void CGenericQuery::setReply(std::optional<int32_t> receivedReply)
 {
-	this->reply = reply;
+	reply = receivedReply;
 }
 
 void CGenericQuery::onRemoval(PlayerColor color)

server/queries/CQuery.h

@@ -9,7 +9,7 @@
  */
 #pragma once
 
-#include "../../lib/GameConstants.h"
+#include "../../lib/constants/EntityIdentifiers.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -34,7 +34,7 @@ using QueryPtr = std::shared_ptr<CQuery>;
 // - object visit
 // - hero movement
 // Queries can cause another queries, forming a stack of queries for each player. Eg: hero movement -> object visit -> dialog.
-class CQuery
+class CQuery : boost::noncopyable
 {
 public:
 	std::vector<PlayerColor> players; //players that are affected (often "blocked") by query
@@ -80,7 +80,7 @@ std::ostream &operator<<(std::ostream &out, QueryPtr query);
 class CDialogQuery : public CQuery
 {
 public:
-	CDialogQuery(CGameHandler * owner);
+	explicit CDialogQuery(CGameHandler * owner);
 	bool endsByPlayerAnswer() const override;
 	bool blocksPack(const CPackForServer *pack) const override;
 	void setReply(std::optional<int32_t> reply) override;
@@ -91,7 +91,7 @@ protected:
 class CGenericQuery : public CQuery
 {
 public:
-	CGenericQuery(CGameHandler * gh, PlayerColor color, std::function<void(std::optional<int32_t>)> Callback);
+	CGenericQuery(CGameHandler * gh, PlayerColor color, const std::function<void(std::optional<int32_t>)> & callback);
 
 	bool blocksPack(const CPackForServer * pack) const override;
 	bool endsByPlayerAnswer() const override;

server/queries/MapQueries.cpp

@@ -262,7 +262,6 @@ void CHeroMovementQuery::onExposure(QueryPtr topQuery)
 	assert(players.size() == 1);
 
 	if(visitDestAfterVictory && hero->tempOwner == players[0]) //hero still alive, so he won with the guard
-		//TODO what if there were H4-like escape? we should also check pos
 	{
 		logGlobal->trace("Hero %s after victory over guard finishes visit to %s", hero->getNameTranslated(), tmh.end.toString());
 		//finish movement

server/queries/QueriesProcessor.cpp

@@ -99,8 +99,8 @@ void QueriesProcessor::popIfTop(const CQuery & query)
 std::vector<std::shared_ptr<const CQuery>> QueriesProcessor::allQueries() const
 {
 	std::vector<std::shared_ptr<const CQuery>> ret;
-	for(auto & playerQueries : queries)
-		for(auto & query : playerQueries.second)
+	for(const auto & playerQueries : queries)
+		for(const auto & query : playerQueries.second)
 			ret.push_back(query);
 
 	return ret;
@@ -110,8 +110,8 @@ std::vector<QueryPtr> QueriesProcessor::allQueries()
 {
 	//TODO code duplication with const function :(
 	std::vector<QueryPtr> ret;
-	for(auto & playerQueries : queries)
-		for(auto & query : playerQueries.second)
+	for(const auto & playerQueries : queries)
+		for(const auto & query : playerQueries.second)
 			ret.push_back(query);
 
 	return ret;