EDIT (2018-12-15): Fixed the example (again).
> […] why do you clone() the two arrays? Is the clone() function in clone(this.associatedDomains) built into Sugarcube?
To make deep copies of the arrays and their contents. And yes, the clone() function is a SugarCube built-in.
> On a probably related note, if I were to pass the entire current instance of an object as the argument for $ReviveData$ in toJSON() , would I need to make a deep copy in the copy ctor or would a shallow copy suffice?
It depends on what you're copying. There's no one size fits all answer, though when in doubt a deep copy is probably the safe option.
To pause for a moment on your example. You're doing very odd things in your constructor. For example:
The this.PlayerTurnState own property is a frozen generic object which only contains primitive values. There doesn't seem any reason for that to be an own property of the instances; or even a prototype property, though it would make more sense attached there. It would be better as a static property of the class.
By their names alone, the this.EnemyTurnState and this.CombatResultEnum properties seem like more candidates for the static property treatment.
The this.randoCombatantExcept own method is similar. It's not a closure over some state internal to its parent function (the constructor), so there doesn't seem any reason for it to be an own property. It would be better placed on the prototype.
Additionally, within this.randoCombatantExcept. You declare combatants but use this.combatants on the next line, I'm betting that latter should be combatants instead. I'd also suggest not mixing ES6 arrow functions into what seems to be an, essentially, ES5 codebase.
> That ctor adaptation based on args looks pretty wretched, but I'm new to JS and I think that might be the only way to support both explicit params and a copy ctor in the absence of function overloading; please advise if there's a more elegant way.
There are several ways you could go about it. The easiest is probably to use a config/option object parameter, rather than explicit parameters. Though, this requires you alter how you normally instantiate the class.
/*
Combat class constructor.
*/
window.Combat = function Combat(config) {
/* ... */
// Copy an instance of Combat.
//
// We know it is/was an instance of Combat because it
// has a property that only an instance would have.
// In other words, the generic object you pass in to
// create a new instance will never have that property.
// E.g. `playerTurnState` seems like a good candidate.
if (config.hasOwnProperty('playerTurnState')) {
// NOTE: Only use one of the following examples!
// Either copy the own properties individually, for maximum control.
this.playerParty = clone(config.playerParty);
this.enemyParty = clone(config.enemyParty);
this.destPassageName = clone(config.destPassageName);
this.playerTurnState = clone(config.playerTurnState);
this.enemyTurnState = clone(config.enemyTurnState);
this.combatResult = clone(config.combatResult);
// Or copy the own properties automatically, for simplicity.
Object.keys(config).forEach(function (pn) {
this[pn] = clone(config[pn]);
}, this);
}
// Create a new instance of Combat.
else {
// Initialize from the config object parameter.
this.playerParty = config.playerParty;
this.enemyParty = config.enemyParty;
this.destPassageName = config.destPassageName;
// Initialize from defaults and whatnot.
this.playerTurnState = Combat.PlayerTurnState.selectAbility;
this.enemyTurnState = Combat.EnemyTurnState.runAI;
this.combatResult = Combat.CombatResultEnum.pending;
}
/* ... */
};
You could instantiate the class with an object literal. For example:
new Combat({
playerParty : /* ... */,
enemyParty : /* ... */,
destPassage : /* ... */
});
EDIT: Note, the above example may not be complete. When making a copy of an existing instance within the constructor, you're going to need to clone/deep-copy any own properties which are not primitives; though cloning everything might be easier.
For bonus points. Based on my notes above, the rest of the class might look like:
/*
Combat class prototype methods and properties.
*/
window.Combat.prototype.clone = function () {
return new window.Combat(this);
};
window.Combat.prototype.toJSON = function () {
// NOTE: Do not use `this` directly or you'll trigger
// out of control recursion. Make a copy of the own
// properties instead.
var data = {};
Object.keys(this).forEach(function (pn) {
data[pn] = clone(this[pn]);
}, this);
return JSON.reviveWrapper('new window.Combat($ReviveData$)', data);
};
window.Combat.prototype.randoCombatantExcept = function (exceptCharacter) {
var eligibleCombatants = this.enemyParty.concat(this.playerParty)
.filter(function (combatant) { return combatant.id !== exceptCharacter.id; });
return eligibleCombatants[Math.floor(Math.random() * eligibleCombatants.length)];
};
/*
Combat class static methods and properties.
*/
window.Combat.PlayerTurnState = Object.freeze({
selectTarget : 1,
selectAbility : 2,
displayResults : 3,
selectEnemyTarget : 4,
selectAllyTarget : 5
});
window.Combat.EnemyTurnState = Object.freeze({
/* ... */
});
window.Combat.CombatResultEnum = Object.freeze({
/* ... */
});
> Will do, but what's the reason for this?
You neither need nor want to stuff static data and/or behaviors into story variables because each moment within the history gets a copy of them, and the history itself is serialized within the current play session and saves. Doing so bloats the history, and thus the current session and saves, for no good reason or purpose.
EDIT (2018-12-15): Fixed the example (again).
commented
by
TheMadExile
(68.6k points)