Skip to content

Commit

Permalink
Fix InterceptEffect.InterceptChance (null check)
Browse files Browse the repository at this point in the history
  • Loading branch information
@NetDwarf
NetDwarf committed Jul 30, 2022
1 parent 00b1090 commit 314bc13
Showing 1 changed file with 4 additions and 14 deletions.
18 changes: 4 additions & 14 deletions GameServer/effects/InterceptEffect.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,25 +62,15 @@ public GameLiving InterceptTarget
get { return m_interceptTarget; }
}

/// <summary>
/// chance to intercept
/// </summary>
public int InterceptChance
{
get
{
GamePet pet = InterceptSource as GamePet;
if (pet.Brain is BrittleBrain)
return 100;
else if (pet is BDSubPet)
// Patch 1.123: The intercept chance on the Fossil Defender has been reduced by 20%.
// Can't find documentation for previous intercept chance, so assuming 50%
return 30;
else if (pet != null)
// Patch 1.125: Reduced the spirit warrior's intercept chance from 75% to 60% and intercept radius from 150 to 125
return 60;
else
return 50;
if(pet == null) return 50;

This comment has been minimized.

Sign in to view

Copy link
@ahungry

ahungry Aug 25, 2022

No offense intended (I'm not totally familiar with DOL/history, maybe you've been the sole maintainer for some time?) - Just randomly browsing things - what's the benefit to stripping the comments?

I'm guessing the bug was that a null check came after a property reference on the potential null, so the restructure makes sense, but you lost a lot of useful context and its more challenging to read (and formatted wrong)

This comment has been minimized.

Sign in to view

Copy link
@NetDwarf

NetDwarf Aug 25, 2022

Author Contributor 
/// <summary>
/// chance to intercept
/// </summary>

Repeating the method name.

// Patch 1.123: The intercept chance on the Fossil Defender has been reduced by 20%

The only additional information is the patch version when it was introduced. Do we really need that information? In case we want to roll it back to a previous patch maybe, but we could also read the patch notes then (and I think that's mandatory anyway then).

// Can't find documentation for previous intercept chance, so assuming 50%

Do we really care about the value it had before? The previous remark applies here. We also do not get that information anyway.

// Patch 1.125: Reduced the spirit warrior's intercept chance from 75% to 60% and intercept radius from 150 to 125

The intercept radius is not altered in this part of the code and we see the current percentage chance in code. Again the only additional information is what the

I think most of these comments are better suited for a commit message (especially to signify why it was changed). If it is omitted and still accepted (i.e. slipped through in a bigger PR or it is already known), you could also read it in the patch notes and I don't think code is the right place to replicate the patch notes.

This comment has been minimized.

Sign in to view

Copy link
@ahungry

ahungry Aug 25, 2022

I found a lot of interesting comments browsing through the DOL source code (reading some of the other larger comment blocks) about historical changes/research based things (the "why" of a lot of the implementation code). I think the code base would have been much less interesting if all comments were stripped.

I've found VCS history is usually not a good source of permanence or easily contextualizing how the comment applies (for instance, DOL used to be svn right? Maybe not in this migration, but in a lot, some info is lost going VCS to VCS) - also, just the interface to read it, even with a git blame on a relevant line, or git log filename - it can be much harder to read what's going on than a maintained comment that is coupled to the lines it's annotating.

Just my 2c, take it with a grain of salt 😄

This comment has been minimized.

Sign in to view

Copy link
@NetDwarf

NetDwarf Aug 25, 2022

Author Contributor

I found a lot of interesting comments browsing through the DOL source code [...]

It is not really meant as a means of documentation beyond the code itself (of course with some exceptions). For this purpose an actual documentation would be better, however in this case it's mostly done for us in the form of patch notes already.

I've found VCS history is usually not a good source of permanence or easily contextualizing how the comment applies [...]

VCS is not really meant as a form of documentation, sure, but it would be a good information for the reviewer of a PR. On the flip side: Conversations and side infos are not really meant to be placed in the code.

[...] even with a git blame on a relevant line, or git log filename - it can be much harder to read what's going on than a maintained comment that is coupled to the lines it's annotating.

The word maintained is the problem here. It means you have to edit the code twice and have to remember that all the time and sometimes the comment you actually need to edit then, is a few lines away even. In this code base are so many comments and a good number of them are not maintained. Essentially the comments are lying, but no worry, nobody reads them anyway and that's the problem with them. When a comment appears say every 100 lines or so, you are more likely to actually read it, compared to when you have every 5th line a comment that explains why you did what you did and when and what you don't know yet and what might need to be done ... this does not belong in the code, really.
else if (pet is BDSubPet) return 30;
else if (pet.Brain is BrittleBrain) return 100;
else return 60;
}
}

Expand Down

0 comments on commit 314bc13

Please sign in to comment.