Bug in SkillMod?

[Cabadam]

Bug in SkillMod?

Was looking at the code for the Owner property on SkillMod:

		public Mobile Owner
		{
			get
			{
				return m_Owner;
			}
			set
			{
				if ( m_Owner != value )
				{
					if ( m_Owner != null )
						m_Owner.RemoveSkillMod( this );

					[COLOR="Red"]m_Owner = value;

					if ( m_Owner != value )[/COLOR]
						m_Owner.AddSkillMod( this );
				}
			}
		}

Unless I misread that, the m_Owner.AddSkillMod will NEVER execute. Shouldn't that be:
m_Owner != null
as the first 'if' is?

 

[noobie]

yes, you are right.

but the particular setter method is called from Mobile.AddSkillMod.

So, it is should not be called since recursive would be unnecessary in such a case.

I am not sure whether it is called from elsewhere except Mobile.AddSkillMod; VS.NET sucks

the last "if" statement should be even there..

good catch

 

[Phantom]

The code is fine, since it works exactly like it should.

 

[noobie]

The code is fine, since it works exactly like it should.

actually Cabadam is right..

the last if statement is never executed since in the previous line, m_Owner is set to "value"..

it works fine, because actually there is no need the last line to be executed..

 

[Cabadam]

The code is fine, since it works exactly like it should.

Well, code that works and code that is correct are two very different things.

Given the fact that it works, then the if statement should probably be removed altogether. The fact that it was there implies that the developer was expecting it to do something (and perhaps at one point it did, but the behavior changed to no longer require it). Maybe SkillMod's used to be explicity applied via the Owner property rather than through the call to Mobile.AddSkillMod, dunno.

 

[Cabadam]

but the particular setter method is called from Mobile.AddSkillMod.

So, it is should not be called since recursive would be unnecessary in such a case.

Well, it would only recurse once - the first if would see to that. It would give the user the option of either adding a skilmod via mobile.AddSkillMod() or by simply setting the owner property of the SkillMod. Would just be a style choice.

I am not sure whether it is called from elsewhere except Mobile.AddSkillMod; VS.NET sucks

And that's where ReSharper steps in
Right now, Mobile.AddSkillMod is the only place its being used. But its a public property, so it could be used anywhere.

 

[Quantos]

Does the fact that the statement in question do anything other than take up space?

If that is the ony result of it then perhaps you should speak to one of the developers about the possibility of having it removed.

 

[Cabadam]

If that is the ony result of it then perhaps you should speak to one of the developers about the possibility of having it removed.

I kinda thought that's what this forum was...

 

[Phantom]

Well your suggestion was wrong, I can tell you that much, this is a Core Modification submission forum.

 

[noobie]

lol

 

[Cabadam]

Well your suggestion was wrong, I can tell you that much, this is a Core Modification submission forum.

Huh? Yes it is a core mod forum. I would call deleting something from the core a 'modification'... what would you call it?

 

[Quantos]

Huh? Yes it is a core mod forum. I would call deleting something from the core a 'modification'... what would you call it?

I would call it a request, not a submission. Submission would suggest that you have the solution and posted it.

*Thread Moved from 'Code Modifications>Subsystems'*
*Thread Closed.*