vcmi: modernize lib/spells (except adventure one, needs major rework)

lib/spells/AdventureSpellMechanics.cpp

@@ -79,7 +79,7 @@ ESpellCastResult AdventureSpellMechanics::applyAdventureEffects(SpellCastEnviron
 
 		owner->getEffects(bonuses, schoolLevel, false, parameters.caster->getEnchantPower(owner));
 
-		for(Bonus b : bonuses)
+		for(const Bonus & b : bonuses)
 		{
 			GiveBonus gb;
 			gb.id = parameters.caster->id.getNum();
@@ -182,7 +182,7 @@ ESpellCastResult SummonBoatMechanics::applyAdventureEffects(SpellCastEnvironment
 	{
 		if(obj && obj->ID == Obj::BOAT)
 		{
-			const CGBoat *b = static_cast<const CGBoat*>(obj);
+			const auto * b = dynamic_cast<const CGBoat *>(obj);
 			if(b->hero)
 				continue; //we're looking for unoccupied boat
 
@@ -249,7 +249,7 @@ ESpellCastResult ScuttleBoatMechanics::applyAdventureEffects(SpellCastEnvironmen
 
 	//TODO: test range, visibility
 	const TerrainTile *t = &env->getMap()->getTile(parameters.pos);
-	if(!t->visitableObjects.size() || t->visitableObjects.back()->ID != Obj::BOAT)
+	if(t->visitableObjects.empty() || t->visitableObjects.back()->ID != Obj::BOAT)
 	{
 		env->complain("There is no boat to scuttle!");
 		return ESpellCastResult::ERROR;
@@ -328,7 +328,7 @@ ESpellCastResult DimensionDoorMechanics::applyAdventureEffects(SpellCastEnvironm
 	{
 		SetMovePoints smp;
 		smp.hid = parameters.caster->id;
-		if(movementCost < (int)parameters.caster->movement)
+		if(movementCost < static_cast<int>(parameters.caster->movement))
 			smp.val = parameters.caster->movement - movementCost;
 		else
 			smp.val = 0;
@@ -356,7 +356,7 @@ ESpellCastResult TownPortalMechanics::applyAdventureEffects(SpellCastEnvironment
 		if(nullptr == destination)
 			return ESpellCastResult::ERROR;
 
-		if((int)parameters.caster->movement < moveCost)
+		if(static_cast<int>(parameters.caster->movement) < moveCost)
 			return ESpellCastResult::ERROR;
 
 		if(destination->visitingHero)
@@ -372,7 +372,7 @@ ESpellCastResult TownPortalMechanics::applyAdventureEffects(SpellCastEnvironment
 	{
 		const TerrainTile & tile = env->getMap()->getTile(parameters.pos);
 
-		const auto topObj = tile.topVisitableObj(false);
+		auto * const topObj = tile.topVisitableObj(false);
 
 		if(!topObj)
 		{
@@ -406,7 +406,7 @@ ESpellCastResult TownPortalMechanics::applyAdventureEffects(SpellCastEnvironment
 			return ESpellCastResult::ERROR;
 		}
 
-		if((int)parameters.caster->movement < moveCost)
+		if(static_cast<int>(parameters.caster->movement) < moveCost)
 		{
 			env->complain("This hero has not enough movement points!");
 			return ESpellCastResult::ERROR;
@@ -449,7 +449,7 @@ ESpellCastResult TownPortalMechanics::beginCast(SpellCastEnvironment * env, cons
 
 	const int moveCost = movementCost(parameters);
 
-	if((int)parameters.caster->movement < moveCost)
+	if(static_cast<int>(parameters.caster->movement) < moveCost)
 	{
 		InfoWindow iw;
 		iw.player = parameters.caster->tempOwner;
@@ -464,7 +464,7 @@ ESpellCastResult TownPortalMechanics::beginCast(SpellCastEnvironment * env, cons
 		{
 			if(reply.getType() == JsonNode::JsonType::DATA_INTEGER)
 			{
-				ObjectInstanceID townId((si32)reply.Integer());
+				ObjectInstanceID townId(static_cast<si32>(reply.Integer()));
 
 				const CGObjectInstance * o = env->getCb()->getObj(townId, true);
 				if(o == nullptr)
@@ -488,7 +488,7 @@ ESpellCastResult TownPortalMechanics::beginCast(SpellCastEnvironment * env, cons
 
 		MapObjectSelectDialog request;
 
-		for(auto t : towns)
+		for(const auto * t : towns)
 		{
 			if(t->visitingHero == nullptr) //empty town
 				request.objects.push_back(t->id);

lib/spells/BattleSpellMechanics.cpp

@@ -127,11 +127,12 @@ namespace SRSLPraserHelpers
 	}
 }
 
-
-BattleSpellMechanics::BattleSpellMechanics(const IBattleCast * event, std::shared_ptr<effects::Effects> effects_, std::shared_ptr<IReceptiveCheck> targetCondition_)
-	: BaseMechanics(event),
-	effects(effects_),
-	targetCondition(targetCondition_)
+BattleSpellMechanics::BattleSpellMechanics(const IBattleCast * event,
+										   std::shared_ptr<effects::Effects> effects_,
+										   std::shared_ptr<IReceptiveCheck> targetCondition_):
+	BaseMechanics(event),
+	effects(std::move(effects_)),
+	targetCondition(std::move(targetCondition_))
 {}
 
 BattleSpellMechanics::~BattleSpellMechanics() = default;
@@ -167,7 +168,7 @@ bool BattleSpellMechanics::canBeCast(Problem & problem) const
 	{
 	case Mode::HERO:
 		{
-			const CGHeroInstance * castingHero = dynamic_cast<const CGHeroInstance *>(caster);//todo: unify hero|creature spell cost
+			const auto * castingHero = dynamic_cast<const CGHeroInstance *>(caster); //todo: unify hero|creature spell cost
 			if(!castingHero)
 			{
 				logGlobal->debug("CSpell::canBeCast: invalid caster");
@@ -272,7 +273,7 @@ void BattleSpellMechanics::cast(ServerCallback * server, const Target & target)
 	//calculate spell cost
 	if(mode == Mode::HERO)
 	{
-		auto casterHero = dynamic_cast<const CGHeroInstance *>(caster);
+		const auto * casterHero = dynamic_cast<const CGHeroInstance *>(caster);
 		spellCost = battle()->battleGetSpellCost(owner, casterHero);
 
 		if(nullptr != otherHero) //handle mana channel
@@ -383,7 +384,7 @@ void BattleSpellMechanics::beforeCast(BattleSpellCast & sc, vstd::RNG & rng, con
 	std::set<const battle::Unit *> unitTargets = collectTargets();
 
 	//process them
-	for(auto unit : unitTargets)
+	for(const auto * unit : unitTargets)
 		filterUnit(unit);
 
 	//and update targets
@@ -405,7 +406,7 @@ void BattleSpellMechanics::beforeCast(BattleSpellCast & sc, vstd::RNG & rng, con
 		}
 	}
 
-	for(auto unit : resisted)
+	for(const auto * unit : resisted)
 		sc.resistedCres.insert(unit->unitId());
 }
 
@@ -448,16 +449,16 @@ void BattleSpellMechanics::doRemoveEffects(ServerCallback * server, const std::v
 {
 	SetStackEffect sse;
 
-	for(auto unit : targets)
+	for(const auto * unit : targets)
 	{
 		std::vector<Bonus> buffer;
 		auto bl = unit->getBonuses(selector);
 
-		for(auto item : *bl)
+		for(const auto & item : *bl)
 			buffer.emplace_back(*item);
 
 		if(!buffer.empty())
-			sse.toRemove.push_back(std::make_pair(unit->unitId(), buffer));
+			sse.toRemove.emplace_back(unit->unitId(), buffer);
 	}
 
 	if(!sse.toRemove.empty())
@@ -487,8 +488,10 @@ std::set<BattleHex> BattleSpellMechanics::spellRangeInHexes(BattleHex centralHex
 
 	if(rng.size() >= 2 && rng[0] != 'X') //there is at least one hex in range (+artificial comma)
 	{
-		std::string number1, number2;
-		int beg, end;
+		std::string number1;
+		std::string number2;
+		int beg = 0;
+		int end = 0;
 		bool readingFirst = true;
 		for(auto & elem : rng)
 		{
@@ -504,12 +507,12 @@ std::set<BattleHex> BattleSpellMechanics::spellRangeInHexes(BattleHex centralHex
 				//calculating variables
 				if(readingFirst)
 				{
-					beg = atoi(number1.c_str());
+					beg = std::stoi(number1);
 					number1 = "";
 				}
 				else
 				{
-					end = atoi(number2.c_str());
+					end = std::stoi(number2);
 					number2 = "";
 				}
 				//obtaining new hexes
@@ -524,7 +527,7 @@ std::set<BattleHex> BattleSpellMechanics::spellRangeInHexes(BattleHex centralHex
 					readingFirst = true;
 				}
 				//adding obtained hexes
-				for(auto & curLayer_it : curLayer)
+				for(const auto & curLayer_it : curLayer)
 				{
 					ret.insert(curLayer_it);
 				}
@@ -532,7 +535,7 @@ std::set<BattleHex> BattleSpellMechanics::spellRangeInHexes(BattleHex centralHex
 			}
 			else if(elem == '-') //dash
 			{
-				beg = atoi(number1.c_str());
+				beg = std::stoi(number1);
 				number1 = "";
 				readingFirst = false;
 			}
@@ -546,7 +549,7 @@ Target BattleSpellMechanics::transformSpellTarget(const Target & aimPoint) const
 {
 	Target spellTarget;
 
-	if(aimPoint.size() < 1)
+	if(aimPoint.empty())
 	{
 		logGlobal->error("Aimed spell cast with no destination.");
 	}
@@ -560,7 +563,7 @@ Target BattleSpellMechanics::transformSpellTarget(const Target & aimPoint) const
 		if(aimPointHex.isValid())
 		{
 			auto spellRange = spellRangeInHexes(aimPointHex);
-			for(auto & hex : spellRange)
+			for(const auto & hex : spellRange)
 				spellTarget.push_back(Destination(hex));
 		}
 	}

lib/spells/BonusCaster.cpp

@@ -22,10 +22,10 @@ VCMI_LIB_NAMESPACE_BEGIN
 namespace spells
 {
 
-BonusCaster::BonusCaster(const Caster * actualCaster_, std::shared_ptr<Bonus> bonus_)
-	: ProxyCaster(actualCaster_),
+BonusCaster::BonusCaster(const Caster * actualCaster_, std::shared_ptr<Bonus> bonus_):
+	ProxyCaster(actualCaster_),
 	actualCaster(actualCaster_),
-	bonus(bonus_)
+	bonus(std::move(bonus_))
 {
 
 }

lib/spells/CSpellHandler.cpp

@@ -84,24 +84,6 @@ static const ESpellSchool SCHOOL_ORDER[4] =
 };
 } //namespace SpellConfig
 
-///CSpell::LevelInfo
-CSpell::LevelInfo::LevelInfo():
-	cost(0),
-	power(0),
-	AIValue(0),
-	smartTarget(true),
-	clearTarget(false),
-	clearAffected(false),
-	range("0")
-{
-
-}
-
-CSpell::LevelInfo::~LevelInfo()
-{
-
-}
-
 ///CSpell
 CSpell::CSpell():
 	id(SpellID::NONE),
@@ -115,23 +97,19 @@ CSpell::CSpell():
 	damage(false),
 	offensive(false),
 	special(true),
-	targetType(spells::AimType::NO_TARGET),
-	mechanics(),
-	adventureMechanics()
+	targetType(spells::AimType::NO_TARGET)
 {
 	levels.resize(GameConstants::SPELL_SCHOOL_LEVELS);
 }
 
-CSpell::~CSpell()
-{
-
-}
+//must be instantiated in .cpp file for access to complete types of all member fields
+CSpell::~CSpell() = default;
 
 bool CSpell::adventureCast(SpellCastEnvironment * env, const AdventureSpellCastParameters & parameters) const
 {
 	assert(env);
 
-	if(!adventureMechanics.get())
+	if(!adventureMechanics)
 	{
 		env->complain("Invalid adventure spell cast attempt!");
 		return false;
@@ -185,7 +163,7 @@ void CSpell::forEachSchool(const std::function<void(const spells::SchoolInfo &,
 	bool stop = false;
 	for(ESpellSchool iter : SpellConfig::SCHOOL_ORDER)
 	{
-		const spells::SchoolInfo & cnf = SpellConfig::SCHOOL[(ui8)iter];
+		const spells::SchoolInfo & cnf = SpellConfig::SCHOOL[static_cast<ui8>(iter)];
 		if(school.at(cnf.id))
 		{
 			cb(cnf, stop);
@@ -405,15 +383,15 @@ int64_t CSpell::adjustRawDamage(const spells::Caster * caster, const battle::Uni
 	//affected creature-specific part
 	if(nullptr != affectedCreature)
 	{
-		auto bearer = affectedCreature;
+		const auto * bearer = affectedCreature;
 		//applying protections - when spell has more then one elements, only one protection should be applied (I think)
 		forEachSchool([&](const spells::SchoolInfo & cnf, bool & stop)
 		{
-			if(bearer->hasBonusOfType(Bonus::SPELL_DAMAGE_REDUCTION, (ui8)cnf.id))
+			if(bearer->hasBonusOfType(Bonus::SPELL_DAMAGE_REDUCTION, static_cast<ui8>(cnf.id)))
 			{
-				ret *= 100 - bearer->valOfBonuses(Bonus::SPELL_DAMAGE_REDUCTION, (ui8)cnf.id);
+				ret *= 100 - bearer->valOfBonuses(Bonus::SPELL_DAMAGE_REDUCTION, static_cast<ui8>(cnf.id));
 				ret /= 100;
-				stop = true;//only bonus from one school is used
+				stop = true; //only bonus from one school is used
 			}
 		});
 
@@ -439,7 +417,7 @@ int64_t CSpell::adjustRawDamage(const spells::Caster * caster, const battle::Uni
 
 int64_t CSpell::calculateRawEffectValue(int32_t effectLevel, int32_t basePowerMultiplier, int32_t levelPowerMultiplier) const
 {
-	return (int64_t)basePowerMultiplier * getBasePower() + levelPowerMultiplier * getLevelPower(effectLevel);
+	return static_cast<int64_t>(basePowerMultiplier) * getBasePower() + levelPowerMultiplier * getLevelPower(effectLevel);
 }
 
 void CSpell::setIsOffensive(const bool val)
@@ -540,18 +518,7 @@ CSpell::AnimationItem::AnimationItem() :
 
 }
 
-
 ///CSpell::AnimationInfo
-CSpell::AnimationInfo::AnimationInfo()
-{
-
-}
-
-CSpell::AnimationInfo::~AnimationInfo()
-{
-
-}
-
 std::string CSpell::AnimationInfo::selectProjectile(const double angle) const
 {
 	std::string res;
@@ -577,7 +544,7 @@ CSpell::TargetInfo::TargetInfo(const CSpell * spell, const int level, spells::Mo
 	clearAffected(false),
 	clearTarget(false)
 {
-	auto & levelInfo = spell->getLevelInfo(level);
+	const auto & levelInfo = spell->getLevelInfo(level);
 
 	smart = levelInfo.smartTarget;
 	massive = levelInfo.range == "X";
@@ -592,10 +559,6 @@ bool DLL_LINKAGE isInScreenRange(const int3 & center, const int3 & pos)
 }
 
 ///CSpellHandler
-CSpellHandler::CSpellHandler() = default;
-
-CSpellHandler::~CSpellHandler() = default;
-
 std::vector<JsonNode> CSpellHandler::loadLegacyData(size_t dataSize)
 {
 	using namespace SpellConfig;
@@ -648,8 +611,8 @@ std::vector<JsonNode> CSpellHandler::loadLegacyData(size_t dataSize)
 
 			auto & chances = lineNode["gainChance"].Struct();
 
-			for(size_t i = 0; i < GameConstants::F_NUMBER; i++)
-				chances[ETownType::names[i]].Integer() = static_cast<si64>(parser.readNumber());
+			for(const auto & name : ETownType::names)
+				chances[name].Integer() = static_cast<si64>(parser.readNumber());
 
 			auto AIVals = parser.readNumArray<si32>(GameConstants::SPELL_SCHOOL_LEVELS);
 
@@ -713,7 +676,7 @@ CSpell * CSpellHandler::loadFromJson(const std::string & scope, const JsonNode &
 
 	SpellID id(static_cast<si32>(index));
 
-	CSpell * spell = new CSpell();
+	auto * spell = new CSpell();
 	spell->id = id;
 	spell->identifier = identifier;
 	spell->modScope = scope;
@@ -774,9 +737,9 @@ CSpell * CSpellHandler::loadFromJson(const std::string & scope, const JsonNode &
 	{
 		if(counteredSpell.second.Bool())
 		{
-			VLC->modh->identifiers.requestIdentifier(counteredSpell.second.meta, counteredSpell.first, [=](si32 id)
+			VLC->modh->identifiers.requestIdentifier(counteredSpell.second.meta, counteredSpell.first, [=](si32 id) 
 			{
-				spell->counteredSpells.push_back(SpellID(id));
+				spell->counteredSpells.emplace_back(id);
 			});
 		}
 	}
@@ -820,7 +783,7 @@ CSpell * CSpellHandler::loadFromJson(const std::string & scope, const JsonNode &
 
 	spell->special = flags["special"].Bool();
 
-	auto findBonus = [&](std::string name, std::vector<Bonus::BonusType> & vec)
+	auto findBonus = [&](const std::string & name, std::vector<Bonus::BonusType> & vec)
 	{
 		auto it = bonusNameMap.find(name);
 		if(it == bonusNameMap.end())
@@ -829,11 +792,11 @@ CSpell * CSpellHandler::loadFromJson(const std::string & scope, const JsonNode &
 		}
 		else
 		{
-			vec.push_back((Bonus::BonusType)it->second);
+			vec.push_back(static_cast<Bonus::BonusType>(it->second));
 		}
 	};
 
-	auto readBonusStruct = [&](std::string name, std::vector<Bonus::BonusType> & vec)
+	auto readBonusStruct = [&](const std::string & name, std::vector<Bonus::BonusType> & vec)
 	{
 		for(auto bonusData: json[name].Struct())
 		{

lib/spells/CSpellHandler.h

@@ -97,9 +97,6 @@ public:
 
 	struct DLL_LINKAGE AnimationInfo
 	{
-		AnimationInfo();
-		~AnimationInfo();
-
 		///displayed on all affected targets.
 		TAnimationQueue affect;
 
@@ -123,17 +120,18 @@ public:
 
 		std::string selectProjectile(const double angle) const;
 	} animationInfo;
+
 public:
 	struct LevelInfo
 	{
-		si32 cost;
-		si32 power;
-		si32 AIValue;
+		si32 cost = 0;
+		si32 power = 0;
+		si32 AIValue = 0;
 
-		bool smartTarget;
-		bool clearTarget;
-		bool clearAffected;
-		std::string range;
+		bool smartTarget = true;
+		bool clearTarget = false;
+		bool clearAffected = false;
+		std::string range = "0";
 
 		//TODO: remove these two when AI will understand special effects
 		std::vector<std::shared_ptr<Bonus>> effects; //deprecated
@@ -141,9 +139,6 @@ public:
 
 		JsonNode battleEffects;
 
-		LevelInfo();
-		~LevelInfo();
-
 		template <typename Handler> void serialize(Handler & h, const int version)
 		{
 			h & cost;
@@ -369,9 +364,6 @@ bool DLL_LINKAGE isInScreenRange(const int3 &center, const int3 &pos); //for spe
 class DLL_LINKAGE CSpellHandler: public CHandlerBase<SpellID, spells::Spell, CSpell, spells::Service>
 {
 public:
-	CSpellHandler();
-	virtual ~CSpellHandler();
-
 	///IHandler base
 	std::vector<JsonNode> loadLegacyData(size_t dataSize) override;
 	void afterLoadFinalization() override;

lib/spells/ISpellMechanics.cpp

@@ -62,7 +62,7 @@ class CustomMechanicsFactory : public ISpellMechanicsFactory
 public:
 	std::unique_ptr<Mechanics> create(const IBattleCast * event) const override
 	{
-		BattleSpellMechanics * ret = new BattleSpellMechanics(event, effects, targetCondition);
+		auto * ret = new BattleSpellMechanics(event, effects, targetCondition);
 		return std::unique_ptr<Mechanics>(ret);
 	}
 protected:
@@ -117,7 +117,7 @@ public:
 
 			if(!levelInfo.effects.empty())
 			{
-				auto timed = new effects::Timed();
+				auto * timed = new effects::Timed();
 				timed->cumulative = false;
 				timed->bonus = levelInfo.effects;
 				effect.reset(timed);
@@ -125,7 +125,7 @@ public:
 
 			if(!levelInfo.cumulativeEffects.empty())
 			{
-				auto timed = new effects::Timed();
+				auto * timed = new effects::Timed();
 				timed->cumulative = true;
 				timed->bonus = levelInfo.cumulativeEffects;
 				effect.reset(timed);
@@ -137,20 +137,15 @@ public:
 	}
 };
 
-
-BattleCast::BattleCast(const CBattleInfoCallback * cb_, const Caster * caster_, const Mode mode_, const CSpell * spell_)
-	: spell(spell_),
+BattleCast::BattleCast(const CBattleInfoCallback * cb_, const Caster * caster_, const Mode mode_, const CSpell * spell_):
+	spell(spell_),
 	cb(cb_),
+	gameCb(IObjectInterface::cb), //FIXME: pass player callback (problem is that BattleAI do not have one)
 	caster(caster_),
 	mode(mode_),
-	magicSkillLevel(),
-	effectPower(),
-	effectDuration(),
-	effectValue(),
 	smart(boost::logic::indeterminate),
 	massive(boost::logic::indeterminate)
 {
-	gameCb = IObjectInterface::cb; //FIXME: pass player callback (problem is that BattleAI do not have one)
 }
 
 BattleCast::BattleCast(const BattleCast & orig, const Caster * caster_)
@@ -245,7 +240,7 @@ void BattleCast::setEffectValue(BattleCast::Value64 value)
 	effectValue = boost::make_optional(value);
 }
 
-void BattleCast::applyEffects(ServerCallback * server, Target target,  bool indirect, bool ignoreImmunity) const
+void BattleCast::applyEffects(ServerCallback * server, const Target & target, bool indirect, bool ignoreImmunity) const
 {
 	auto m = spell->battleMechanics(this);
 
@@ -296,7 +291,7 @@ void BattleCast::cast(ServerCallback * server, Target target)
 
 			if(!mirrorTargets.empty())
 			{
-				auto mirrorDestination = (*RandomGeneratorUtil::nextItem(mirrorTargets, *server->getRNG()));
+				const auto * mirrorDestination = (*RandomGeneratorUtil::nextItem(mirrorTargets, *server->getRNG()));
 
 				Target mirrorTarget;
 				mirrorTarget.emplace_back(mirrorDestination);
@@ -325,7 +320,7 @@ bool BattleCast::castIfPossible(ServerCallback * server, Target target)
 {
 	if(spell->canBeCast(cb, mode, caster))
 	{
-		cast(server, target);
+		cast(server, std::move(target));
 		return true;
 	}
 	return false;
@@ -397,10 +392,8 @@ ISpellMechanicsFactory::ISpellMechanicsFactory(const CSpell * s)
 
 }
 
-ISpellMechanicsFactory::~ISpellMechanicsFactory()
-{
-
-}
+//must be instantiated in .cpp file for access to complete types of all member fields
+ISpellMechanicsFactory::~ISpellMechanicsFactory() = default;
 
 std::unique_ptr<ISpellMechanicsFactory> ISpellMechanicsFactory::get(const CSpell * s)
 {
@@ -420,16 +413,14 @@ Mechanics::Mechanics()
 
 Mechanics::~Mechanics() = default;
 
-BaseMechanics::BaseMechanics(const IBattleCast * event)
-	: Mechanics(),
+BaseMechanics::BaseMechanics(const IBattleCast * event):
 	owner(event->getSpell()),
 	mode(event->getMode()),
 	smart(event->isSmart()),
-	massive(event->isMassive())
+	massive(event->isMassive()),
+	cb(event->getBattle()),
+	gameCb(event->getGame())
 {
-	cb = event->getBattle();
-	gameCb = event->getGame();
-
 	caster = event->getCaster();
 
 	//FIXME: do not crash on invalid side
@@ -510,7 +501,7 @@ bool BaseMechanics::adaptProblem(ESpellCastProblem::ESpellCastProblem source, Pr
 		{
 			MetaString text;
 			//TODO: refactor
-			auto hero = dynamic_cast<const CGHeroInstance *>(caster);
+			const auto * hero = dynamic_cast<const CGHeroInstance *>(caster);
 			if(!hero)
 				return adaptGenericProblem(target);
 

lib/spells/ISpellMechanics.h

@@ -49,7 +49,7 @@ namespace scripting
 class DLL_LINKAGE SpellCastEnvironment : public ServerCallback
 {
 public:
-	virtual ~SpellCastEnvironment(){};
+	virtual ~SpellCastEnvironment() = default;
 
 	virtual const CMap * getMap() const = 0;
 	virtual const CGameInfoCallback * getCb() const = 0;
@@ -128,7 +128,7 @@ public:
 	void setEffectValue(Value64 value);
 
 	///only apply effects to specified targets
-	void applyEffects(ServerCallback * server, Target target, bool indirect = false, bool ignoreImmunity = false) const;
+	void applyEffects(ServerCallback * server, const Target & target, bool indirect = false, bool ignoreImmunity = false) const;
 
 	///normal cast
 	void cast(ServerCallback * server, Target target);

lib/spells/Problem.cpp

@@ -18,19 +18,9 @@ namespace spells
 namespace detail
 {
 
-ProblemImpl::ProblemImpl()
-{
-
-}
-
-ProblemImpl::~ProblemImpl()
-{
-
-}
-
 void ProblemImpl::add(MetaString && description, Severity severity)
 {
-	data.push_back(std::make_pair(description, severity));
+	data.emplace_back(description, severity);
 }
 
 void ProblemImpl::getAll(std::vector<std::string> & target) const

lib/spells/Problem.h

@@ -24,9 +24,6 @@ namespace detail
 class DLL_LINKAGE ProblemImpl : public Problem
 {
 public:
-	ProblemImpl();
-	virtual ~ProblemImpl();
-
 	void add(MetaString && description, Severity severity = CRITICAL) override;
 
 	void getAll(std::vector<std::string> & target) const override;

lib/spells/TargetCondition.cpp

@@ -26,20 +26,11 @@ VCMI_LIB_NAMESPACE_BEGIN
 namespace spells
 {
 
-TargetConditionItem::TargetConditionItem() = default;
-TargetConditionItem::~TargetConditionItem() = default;
-
 class TargetConditionItemBase : public TargetConditionItem
 {
 public:
-	bool inverted;
-	bool exclusive;
-
-	TargetConditionItemBase()
-		: inverted(false),
-		exclusive(false)
-	{
-	}
+	bool inverted = false;
+	bool exclusive = false;
 
 	void setInverted(bool value) override
 	{
@@ -87,10 +78,7 @@ private:
 class CreatureCondition : public TargetConditionItemBase
 {
 public:
-	CreatureCondition(CreatureID type_)
-		: type(type_)
-	{
-	}
+	CreatureCondition(const CreatureID & type_): type(type_) {}
 
 protected:
 	bool check(const Mechanics * m, const battle::Unit * target) const override
@@ -218,11 +206,6 @@ protected:
 //for Hypnotize
 class HealthValueCondition : public TargetConditionItemBase
 {
-public:
-	HealthValueCondition()
-	{
-	}
-
 protected:
 	bool check(const Mechanics * m, const battle::Unit * target) const override
 	{
@@ -238,8 +221,7 @@ protected:
 class SpellEffectCondition : public TargetConditionItemBase
 {
 public:
-	SpellEffectCondition(SpellID spellID_)
-		: spellID(spellID_)
+	SpellEffectCondition(const SpellID & spellID_): spellID(spellID_)
 	{
 		std::stringstream builder;
 		builder << "source_" << Bonus::SPELL_EFFECT << "id_" << spellID.num;
@@ -262,13 +244,6 @@ private:
 
 class ReceptiveFeatureCondition : public TargetConditionItemBase
 {
-public:
-	ReceptiveFeatureCondition()
-	{
-		selector = Selector::type()(Bonus::RECEPTIVE);
-		cachingString = "type_RECEPTIVE";
-	}
-
 protected:
 	bool check(const Mechanics * m, const battle::Unit * target) const override
 	{
@@ -276,17 +251,12 @@ protected:
 	}
 
 private:
-	CSelector selector;
-	std::string cachingString;
+	CSelector selector = Selector::type()(Bonus::RECEPTIVE);
+	std::string cachingString = "type_RECEPTIVE";
 };
 
 class ImmunityNegationCondition : public TargetConditionItemBase
 {
-public:
-	ImmunityNegationCondition()
-	{
-	}
-
 protected:
 	bool check(const Mechanics * m, const battle::Unit * target) const override
 	{
@@ -404,16 +374,12 @@ const TargetConditionItemFactory * TargetConditionItemFactory::getDefault()
 	return singleton.get();
 }
 
-TargetCondition::TargetCondition() = default;
-
-TargetCondition::~TargetCondition() = default;
-
 bool TargetCondition::isReceptive(const Mechanics * m, const battle::Unit * target) const
 {
 	if(!check(absolute, m, target))
 		return false;
 
-	for(auto item : negation)
+	for(const auto & item : negation)
 	{
 		if(item->isReceptive(m, target))
 			return true;
@@ -462,7 +428,7 @@ bool TargetCondition::check(const ItemVector & condition, const Mechanics * m, c
 	bool nonExclusiveCheck = false;
 	bool nonExclusiveExits = false;
 
-	for(auto & item : condition)
+	for(const auto & item : condition)
 	{
 		if(item->isExclusive())
 		{
@@ -481,7 +447,7 @@ bool TargetCondition::check(const ItemVector & condition, const Mechanics * m, c
 
 void TargetCondition::loadConditions(const JsonNode & source, bool exclusive, bool inverted, const ItemFactory * itemFactory)
 {
-	for(auto & keyValue : source.Struct())
+	for(const auto & keyValue : source.Struct())
 	{
 		bool isAbsolute;
 
@@ -494,9 +460,11 @@ void TargetCondition::loadConditions(const JsonNode & source, bool exclusive, bo
 		else
 			continue;
 
-		std::string scope, type, identifier;
+		std::string scope;
+		std::string type;
+		std::string identifier;
 
-		VLC->modh->parseIdentifier(keyValue.first, scope, type, identifier);
+		CModHandler::parseIdentifier(keyValue.first, scope, type, identifier);
 
 		std::shared_ptr<TargetConditionItem> item = itemFactory->createConfigurable(scope, type, identifier);
 

lib/spells/TargetCondition.h

@@ -30,9 +30,6 @@ class Mechanics;
 class DLL_LINKAGE TargetConditionItem : public IReceptiveCheck
 {
 public:
-	TargetConditionItem();
-	virtual ~TargetConditionItem();
-
 	virtual void setInverted(bool value) = 0;
 	virtual void setExclusive(bool value) = 0;
 
@@ -70,9 +67,6 @@ public:
 	ItemVector absolute;
 	ItemVector negation;
 
-	TargetCondition();
-	virtual ~TargetCondition();
-
 	bool isReceptive(const Mechanics * m, const battle::Unit * target) const override;
 
 	void serializeJson(JsonSerializeFormat & handler, const ItemFactory * itemFactory);

lib/spells/ViewSpellInt.cpp

@@ -16,12 +16,6 @@
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-ObjectPosInfo::ObjectPosInfo():
-	pos(), id(Obj::NO_OBJ), subId(-1), owner(PlayerColor::CANNOT_DETERMINE)
-{
-
-}
-
 ObjectPosInfo::ObjectPosInfo(const CGObjectInstance * obj):
 	pos(obj->visitablePos()), id(obj->ID), subId(obj->subID), owner(obj->tempOwner)
 {

lib/spells/ViewSpellInt.h

@@ -20,11 +20,11 @@ VCMI_LIB_NAMESPACE_BEGIN
  struct DLL_LINKAGE ObjectPosInfo
  {
  	int3 pos;
- 	Obj id;
- 	si32 subId;
- 	PlayerColor owner;
- 	ObjectPosInfo();
- 	ObjectPosInfo(const CGObjectInstance * obj);
+	Obj id = Obj::NO_OBJ;
+	si32 subId = -1;
+	PlayerColor owner = PlayerColor::CANNOT_DETERMINE;
+	ObjectPosInfo() = default;
+	ObjectPosInfo(const CGObjectInstance * obj);
 
 	template <typename Handler> void serialize(Handler & h, const int version)
 	{