Refactor devilution.DefaultClass::CheckCursMove()

[AJenbo]

I've selected devilution.DefaultClass::CheckCursMove() for refactoring, which is a unit of 421 lines of code. Addressing this will make our codebase more maintainable and improve Better Code Hub's Write Short Units of Code guideline rating! 👍

Here's the gist of this guideline:

• Definition 📖
  Limit the length of code units to 15 lines of code.
• Why❓
  Small units are easier to analyse, test and reuse.
• How 🔧
  When writing new units, don't let them grow above 15 lines of code. When a unit grows beyond this, split it in smaller units of no longer than 15 lines.

You can find more info about this guideline in Building Maintainable Software. 📖

---

Good luck and happy coding! ✨ 💯

[mpanneton]

Hi! I may be able to help to refactor certain part of this code.

First, many conditions check things like: (mx + 2 < MAXDUNX && my + 1 < MAXDUNY) can it be replace it by using the currentTile Point variable on line 343 and the function bool InDungeonBounds(Point position) in the gendung.h file?

instead of : if (!flipflag && mx + 2 < MAXDUNX && my + 1 < MAXDUNY && dMonster[mx + 2][my + 1] != 0 && IsTileLit({ mx + 2, my + 1 }))

We could have : if (!flipflag && InDungeonBounds(currentTile) && dMonster[mx + 2][my + 1] != 0 && IsTileLit({ mx + 2, my + 1 }))

Thanks!

[AJenbo]

@mpanneton Hi an welcome :)

This looks like a really good part to start on. I would suggest a few further steps for the code in question.

On line 594 you will see currentTile being used build an offset that is then used for the acutal comparisons:

Point testPosition = currentTile + Direction::South;
Object *object = ObjectAtPosition(testPosition);

This avoid applying the offset in each comparison.

If you create a helper IsMonsterAtPosition() for dMonster similar to IsObjectAtPosition() is for dObject, that would also avoid having to repeat testPosition for x/y.

Leaving us with the following:

Point testPosition = currentTile + Direction::South + Direction::SouthEast;
if (!flipflag && InDungeonBounds(testPosition) && IsMonsterAtPosition(testPosition) && IsTileLit(testPosition)) {
    cursPosition = testPosition
}

I'm thinking that this can also be punted of in to a small helper function since there is lots of similar checks in the code, this would again save on repetition and make things more readable:

bool IsVisibleMonsterOnTile(Point tile)
{
    return InDungeonBounds(tile) && IsMonsterAtPosition(tile) && IsTileLit(tile)
}

Point testPosition = currentTile + Direction::South + Direction::SouthEast;
if (!flipflag && IsVisibleMonsterOnTile(testPosition)) {
    cursPosition = testPosition
}

(IsMonsterAtPosition might be redundant at this point, but could be useful elsewhere)