Making Functions bit more granular

#1
Hi, Justin,

I'm right now trying to follow how Ability system works and it's a bit difficult to follow since there many functions are doing quite a lot of thing within each body.
Ability system is pretty much the most important system in UCC and it's also one of the most complicated to understand since there are too many things are going on.

For example, in ItemEquipVerifier, there is TryToggleItem that will either equip or unequip items depends on the input parameter. It took me a while to understand why Toggle is there. Toggle can mean many things or it can mean nothing.

It would be much cleaner if you separate the function into two clear types. TryEquipItem or TryUnequipItem.

Likewise, EquipUnequip.cs is a bit difficult to follow because some functions do many things.
If the function is long and it branches on input parameters, it's probably a good practice to separate them out without making it too granular.

My general observation is that there are many long functions and it would be better if the functions are more granular.
Even if the function does not have more than one consumer, it's still better practice to make it into more digestible size.

I'm sure it will help many potential future users.

Otherwise, I enjoy reading your code. It's very well written code.

Thanks.
 

Justin

Administrator
Staff member
#2
Equip Unequip is probably the most complicated ability and it went through the most number of revisions before release. When I started out I had two separate Unequip and Equip abilities but that ended up not working out because equipping goes along with unequip and visa-versa. Item sets make it even more complicated because you can have different items equipping/unequipping depending on the slot. You can also quickly scroll through the inventory so the equip/unequip event is never sent before continuing on to the next item set so it gets nasty pretty quickly.

Because Equip Unequip is working well right now and I went through so many iterations in order to get it to this stage I am extremely hesitant to change it unless a bug is being fixed or a feature is being added. Splitting out a function into a smaller function is no problem, but it looks like the Update method is really the only extended length method and the rest are pretty concise. I don't want to split out EquipUnequip into separate equip and unequip methods because from previous experience that leads to a lot of problems with edge cases.
 
#3
In this particular case, I was giving an example for TryToggleItem in ItemEquipVerifier.

Sure. why fix it if it's working, but if you can make the function clearer by reducing the number of parameters for a confusing function, it will help the user understand it more easily.

I guess what happened was that, it started as simple function but it kept on adding more test cases.

I'm just trying to help you or share my opinions while I'm learning UCC.

Thanks.
 
Top