Suggestion: Remove sentinel values in enums

[Kakuri]

My next great project approaches... what have I gotten myself into...

I suggest we abandon the practice of using sentinel values in enums. It can cause problems (see below) and is not recommended.

(A sentinel value is a duplicate value in an enum to indicate boundaries, like eRealm.Albion = 1 and the sentinel value eRealm._FirstPlayerRealm = 1.)

Consider the following:

Code: Select all

Dictionary<eRealm, int> resists = new Dictionary<eRealm, int>();

resists.Add( eRealm.Hibernia, 10);

Now let's say we wanted to get the value for Hibernia:

Code: Select all

eRealm hib = eRealm.Hibernia;
int resist = resists[hib];

But this doesn't work! Because "hib" isn't actually "eRealm.Hibernia" - it's "eRealm._Last."

I do realize that a fair bit of information has changed as DAOC has evolved (new weapons, classes), but enums should generally be used for static data. Which means we know what's there. Which means we don't need sentinel values. Instead of:

Code: Select all

if( realm >= eRealm._First && realm <= eRealm._Last )

you can just go take a moment to look at the definition of eRealm and do:

Code: Select all

if( realm >= eRealm.Albion && realm <= eRealm.Hibernia )

If another realm is ever added, I think the eRealm._First/_Last will be the least of our complications. For frequent checks of this type, the functionality is probably best encapsulated in its own method anyway, like IsPlayerRealm(), IsArmor(), IsWeapon(), etc.

Please share your thoughts on the idea. If it's agreed to be a good idea, I'll probably sacrifice myself to the hassle of implementing it.

I have a dream... one day, I imagine I will look upon the DOL API and be pleased, and I will use it to make new things... in the meantime, it looks like I'm stuck trying to give the API some extra shiny.

[Dunnerholl]

I do realize that a fair bit of information has changed as DAOC has evolved (new weapons, classes), but enums should generally be used for static data. Which means we know what's there. Which means we don't need sentinel values. Instead of:

Code: Select all

if( realm >= eRealm._First && realm <= eRealm._Last )

you can just go take a moment to look at the definition of eRealm and do:

Code: Select all

if( realm >= eRealm.Albion && realm <= eRealm.Hibernia )

If another realm is ever added, I think the eRealm._First/_Last will be the least of our complications.

just my instant thoughts while reading this :

that whole example

Code: Select all

if( realm >= eRealm.Albion && realm <= eRealm.Hibernia )

could be changed to

Code: Select all

if( Enum.IsDefined(typeof(eRealm), realm ) )

which could be refactored to a method....

another thing that i came across lately in enums is such as the enum eCraftingSkill that has a value NoCrafting = 0,

such can lead to confusion too, to include somthing that you wouldn't expect there to be.

i think that the _FIRST and _LAST are just for convenience and should be removed and the loops updated to use maybe foreach, which would be safe for changes.

[Graveen]

In fact, the eRealm.Hibernia is broken because i introduced a change for the doors. Introducing the new eRealm, broke the compatibility. /realm is now displaying _Last instead of Hib.

The change for INTRA REALM doors, is normally allowing deep interaction with doors now. Deep interaction is /use on item to open, break, a door. A door can belong to a guild, and be either attacked or not (interesting in instances, well, in any place there is a door that is not a keep door. For example, Cetus door is matching this requirement.

I was not really interested (and i talked with the developper introducing this) in having a 5th realm (considered, previously, a 5th realm was removed.... labeled 6); but i prefered having a working canvas instead of get rid of theses functionnality. And i think serverrules can be used to know if a door is attackable, instead of introducing a new realm.

Perhaps we should inspirate and have a look on 2.0 code, but this is an hard thing, both understanding actual code, and ex future code.

I think another thing, related to the changes. Perhaps we could provide a script, each time we are introducing breaking changes. The script should encapsulate all our changes - simply put a new folder in gameserverscripts- thus allowing retro compatibility, but without loading the actual core.
Of course, if it is possible. Else, good bye.
I recently removed all obsolete methods in DolDatabase, once i fixed Storm's scripts

I think some 'non enums' are interesting in fact; Dunnerholl is talking about NoCrafting, i see at least eRealm.None very interesting.

And last, yes, there are a lot of things in that way. The simple fact of casting (eRealm)player.Realm is not logical, but this is an endless run, involving both the common senses and our personnal feeling about the code.

Typically, the _last and _first are interesting to remove, but we have to find a replacement (using reflection, certainly) for the moving enums: version, classes, races, etc...

And yes, the way Dunnerholl is using Enum.IsDefined is really good, as it stays logical and do not involve an abstract comparison operator between realms.

[Dunnerholl]

another thing to notice are comments.....

the big majority of comments are bad, lying, outdated, non helping , misleading, confusing.....
in short do more worse than they help - i think even those i put in myself


just had a quick look at eRealm :

/// <summary>
/// LastRealmNumber to allow dynamic allocation of realm specific arrays.
/// </summary>
_Last = 3,

how do you like that comment? how is it different from _LastPlayerRealm = 3, ? what are dynamic allocation of realm specific arrays and how is this different from Door ?

/// <summary>
/// Hibernia Realm
/// </summary>
Hibernia = 3,

who would guess that?


/// <summary>
/// DoorRealmNumber to allow dynamic allocation of realm specific arrays.
/// </summary>
Door = 6,

what is dynamic allocation of realm specific arrays? where can you check it? why is Door called DoorRealmNumber in the comment?


/// <summary>
/// No specific realm
/// </summary>
None = 0,

is it true? or could it be called the mobrealm? or defaultrealm? how is 0 not specific?

[Dunnerholl]

I was not really interested (and i talked with the developper introducing this) in having a 5th realm (considered, previously, a 5th realm was removed.... labeled 6); but i prefered having a working canvas instead of get rid of theses functionnality. And i think serverrules can be used to know if a door is attackable, instead of introducing a new realm.

i dont really understand why realm 6 is needed at all.

how is it different from a realm 0 door as it was before? is 0 simply attackable by all?

ps: while checking i found one (eRealm)6 in DoorRequestHandler and replaced it

i wish you good luck ironing out that stuff kakuri, i found so many hard setted int values instead of using the enum, its unbelieveable.

[Dunnerholl]

one more about the first link u provided - i even didnt know this...


[Flags]
public enum AttributeTargets {
Assembly = 0x0001,
Module = 0x0002,
Cass = 0x0004,
Struct = 0x0008,
...
}

could that be useful for all our flags?

like such stuff

if (!sameRegion)
playerStatus |= 0x20;

what normal person can remember all that hexvalues?

at least someone left a comment listing them below

// 0x00 = Normal , 0x01 = Dead , 0x02 = Mezzed , 0x04 = Diseased ,
// 0x08 = Poisoned , 0x10 = Link Dead , 0x20 = In Another Region, 0x40 - NS

so u see already that dev knew it will be hard to read, bad he didnt write an enum or something else instead of a comment

[Dinberg]

While for realms its not a problem, there are other places which could be an issue. What about EquipmentLast, FirstBackPack, FirstVault for inventory enums? I do think its logical, bit I see the issue crystal clear. How badly would instead using the byte value as a key affect performance? This definately isnt my area, but if you used realm = (byte)eRealm.Hibernia, wouldn't that solve alot of the issue? _First and _last will be identical to Albion and Hibernia when represented in byte form.

[Aredhel]

While for realms its not a problem, there are other places which could be an issue. What about EquipmentLast, FirstBackPack, FirstVault for inventory enums? I do think its logical, bit I see the issue crystal clear. How badly would instead using the byte value as a key affect performance? This definately isnt my area, but if you used realm = (byte)eRealm.Hibernia, wouldn't that solve alot of the issue? _First and _last will be identical to Albion and Hibernia when represented in byte form.

Exactly,

Code: Select all

HousingInventory_First = 150,
HousingInventory_Last = 249,

HouseVault_First = 1000,
HouseVault_Last = 1399,

comes to mind immediately. I found that these sentinel values are used for looping most of the time, imagine you'd have to write a loop and then intellisense presented you with an alphabetically sorted list with all possible enums - you'd have to check the actual file these enums are defined in to get an idea which the boundaries really are. In my opinion, this change would be a cosmetical one at the cost of coding convenience.

Oh and please stick to one topic, if you need to bring up comments, start a new thread or we'll start mixing things before long

[Dunnerholl]

While for realms its not a problem, there are other places which could be an issue. What about EquipmentLast, FirstBackPack, FirstVault for inventory enums? I do think its logical, bit I see the issue crystal clear. How badly would instead using the byte value as a key affect performance? This definately isnt my area, but if you used realm = (byte)eRealm.Hibernia, wouldn't that solve alot of the issue? _First and _last will be identical to Albion and Hibernia when represented in byte form.

Exactly,

Code: Select all

HousingInventory_First = 150,
HousingInventory_Last = 249,

HouseVault_First = 1000,
HouseVault_Last = 1399,

comes to mind immediately. I found that these sentinel values are used for looping most of the time, imagine you'd have to write a loop and then intellisense would present you with an alphabetically sorted list with all possible enums - you'd have to check the actual file these enums are defined in to get an idea which the boundaries really are. In my opinion, this change would be a cosmetical one at the cost of coding convenience.

Oh and please stick to one topic, if you need to bring up comments, start a new thread or we'll start mixing things before long

why not useing foreach over the enum? of course not possible with duplicates as currently

[Aredhel]

While for realms its not a problem, there are other places which could be an issue. What about EquipmentLast, FirstBackPack, FirstVault for inventory enums? I do think its logical, bit I see the issue crystal clear. How badly would instead using the byte value as a key affect performance? This definately isnt my area, but if you used realm = (byte)eRealm.Hibernia, wouldn't that solve alot of the issue? _First and _last will be identical to Albion and Hibernia when represented in byte form.

Exactly,

Code: Select all

HousingInventory_First = 150,
HousingInventory_Last = 249,

HouseVault_First = 1000,
HouseVault_Last = 1399,

comes to mind immediately. I found that these sentinel values are used for looping most of the time, imagine you'd have to write a loop and then intellisense would present you with an alphabetically sorted list with all possible enums - you'd have to check the actual file these enums are defined in to get an idea which the boundaries really are. In my opinion, this change would be a cosmetical one at the cost of coding convenience.

Oh and please stick to one topic, if you need to bring up comments, start a new thread or we'll start mixing things before long

why not useing foreach over the enum? of course not possible with duplicates as currently

Code: Select all

public enum eInventorySlot : int
{
LastEmptyQuiver = -6,
FirstEmptyQuiver = -5,
LastEmptyVault = -4,
FirstEmptyVault = -3,
LastEmptyBackpack = -2,
FirstEmptyBackpack = -1,

Invalid = 0,
Ground = 1,

Min_Inv = 7,

HorseArmor = 7, // Equipment, horse armor
HorseBarding = 8, // Equipment, horse barding
Horse = 9, // Equipment, horse

RightHandWeapon = 10,//Equipment, Visible
MinEquipable = 10,
LeftHandWeapon = 11,//Equipment, Visible
TwoHandWeapon = 12,//Equipment, Visible
DistanceWeapon = 13,//Equipment, Visible
FirstQuiver = 14,
SecondQuiver = 15,
ThirdQuiver = 16,
FourthQuiver = 17,
HeadArmor = 21,//Equipment, Visible
HandsArmor = 22,//Equipment, Visible
FeetArmor = 23,//Equipment, Visible
Jewellery = 24,//Equipment
TorsoArmor = 25,//Equipment, Visible
Cloak = 26,//Equipment, Visible
LegsArmor = 27,//Equipment, Visible
ArmsArmor = 28,//Equipment, Visible
Neck = 29,//Equipment
Waist = 32,//Equipment
LeftBracer = 33,//Equipment
RightBracer = 34,//Equipment
LeftRing = 35,//Equipment
RightRing = 36,//Equipment
Mythical = 37,
MaxEquipable = 37,

FirstBackpack = 40,
LastBackpack = 79,

Mithril = 80,
Platinum = 81,
Gold = 82,
Silver = 83,
Copper = 84,

PlayerPaperDoll = 100,
FirstVault = 110,
LastVault = 149,

HousingInventory_First = 150,
HousingInventory_Last = 249,

HouseVault_First = 1000,
HouseVault_Last = 1399,

Consignment_First = 1500,
Consignment_Last = 1599,

//FirstFixLoot = 256, //You can define drops that will ALWAYS occur (eg quest drops etc.)
//LastFixLoot = 356, //100 drops should be enough ... if not, just raise this var, we have thousands free
//LootPagesStart = 500, //Let's say each loot page is 100 slots in size, lots of space for random drops

// money slots changed since 178
Mithril178 = 500,
Platinum178 = 501,
Gold178 = 502,
Silver178 = 503,
Copper178 = 504,
NewPlayerPaperDoll= 600,

Max_Inv = 249,
}

Have fun looping with foreach

[Dinberg]

Given Mythic has made changes to the slots before, It seems silly to not allow us to easily change the system. Sure, we could do a foreach, or even a while, but these loops are scattered many times over in the code, and changing them in light of any new updates would very quickly become tiresome. I suppose technically they should be constants, like 'public const byte InventoryBackpackFirst', or a constant enum value, but theres quite a few so I can see why it was tempting to add them to the eInventorySlots.

Also the reason these were used (as far as I believe, anyway) was because we wanted to conveniently handle a range of values. So we dont actually want to use every value, as a foreach would suggest.

[Dunnerholl]

Have fun looping with foreach

alrght, but why would i want to loop this in the first place if it covers several logic units?

[Dinberg]

alrght, but why would i want to loop this in the first place if it covers several logic units?

I'm beginning to think there's an air of misunderstanding in this thread ^^. I'm certainly now confused. What exactly is the proposal, how would the various attributes in the enums be replaced/by what system? Thanks, I'm enjoying this debate

[Aredhel]

Have fun looping with foreach

alrght, but why would i want to loop this in the first place if it covers several logic units?

Errr... I might want to check if item X is in the players inventory, how do you suggest I should do that ignoring the fact that there already are methods doing just that?

[Aredhel]

alrght, but why would i want to loop this in the first place if it covers several logic units?

I'm beginning to think there's an air of misunderstanding in this thread ^^. I'm certainly now confused. What exactly is the proposal, how would the various attributes in the enums be replaced/by what system? Thanks, I'm enjoying this debate

I'm beginning to think people have no idea of what they're getting themselves into

The proper way to do this would be to use different enum types like eInventoryBackpack, eInventoryMoney etc. and then you could actually loop over single compartments using foreach. However, this is no small change and would go all the way down to the packet handler, possibly requiring several overloads to handle each enum type properly (unless you want to cast them to integers of course, which you wouldn't because you're on a mission to make the code more elegant ).