Weapon attack cleanup

[Saklad5]

This is a series of refactors centered around eliminating unnecessary raw loops from WeaponAttackAction.

[HammerGS]

This has a ton of data stuff attached to it. I'm assuming you didn't update your branch to master before doing the PR.

[Saklad5]

This has a ton of data stuff attached to it. I'm assuming you didn't update your branch to master before doing the PR.

Mistakes were made. I'm working on it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.92%. Comparing base (36e8cab) to head (014cb8f).
Report is 41 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5922      +/-   ##
============================================
+ Coverage     28.90%   28.92%   +0.01%     
- Complexity    13926    13943      +17     
============================================
  Files          2539     2539              
  Lines        268363   268319      -44     
  Branches      47933    47872      -61     
============================================
+ Hits          77578    77606      +28     
+ Misses       186817   186748      -69     
+ Partials       3968     3965       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[SJuliez]

Request to keep streams out of if statements. I'd rather have an extra line and boolean assignment and a variable name that says what this test is about.

[Saklad5]

I don't see how having a distinct boolean would add clarity beyond what the existing comment provides.

[Saklad5]

Do you want me to change the comment to be an initialized boolean anyway?

[rjhancock]

I agree with @SJuliez. It makes the code harder to read.

[SJuliez]

For readability and code length, this does not feel like an improvement to me.

[Saklad5]

target really should be final, but the current implementation for Swarm LRMs prevents that.

[Saklad5]

I find this more readable because it makes it much easier to tell that it is simply getting any entity that successfully applied TAG to the target.

[Saklad5]

If this is an issue, I could just drop this commit. I'd rather not hold up further review of this pull request due to a few controversial changes.

[rjhancock]

This changes the code and may have other impacts as the old code picked the last target that matched where as the new code picks a random one.

Depending upon the rules, this may not be a valid change.

[Saklad5]

This will literally never make a difference. The source of the TAG does not matter.

[HammerGS]

First I want to say thank you for the PR and you're interest to improve MegaMek. However, we have some concerns about this refactor, particularly given its scope and the core area it affects.

Our main points of concern are:

Readability: We strive to keep our code easy to understand. While streams and optionals are useful, overusing them can make the code less accessible, especially for newcomers or those with different skill levels. I'm not a coder but years of working with the code I've learned to read it, but I don't fully understand what I'm looking it here. Running it with the debugger hasn't helped me.

Consistency: MegaMek's code has developed over many years, establishing its own style and conventions. This creates a scenario where we need to refactor and roll out changes in a consistent pattern, and behind the scenes discussions around this happen and having refactoring PR's can disrupt plans.

Performance: Although it's not our top priority in most areas, unnecessary use of streams and optionals can slow things down. We prefer changes that offer clear performance benefits.

Risk management: Refactoring critical parts of the code can lead to unexpected issues. MegaMek is complex, so we need to carefully consider and justify changes. Smaller, focused updates are easier to review and less likely to cause problems.

Collaboration: We value feedback and teamwork in our development process. If concerns raised by Devs (or the tools) aren't addressed, it's unlikely we'll accept a pull request. (You can find our guidelines for developer and contributor expectations on the MegaMek Wiki.) Likewise, after discussions with Devs and getting their feedback if that feedback is ignored it does reduce the likelyhood of a PR being accepted.

Given these concerns, we're going to hold off on merging this PR while we evaluate its overall impact. Please address the existing comments on the PR first. Once that's done, we'll discuss the next steps.

For future reference, it's a good idea to chat with the Dev team before starting any major refactoring work. We are open to using these but want to start small and in specific areas of the code.

As I mentioned in my recent Sarna interview, "You need to earn some street cred by showing you understand our 20-year-old codebase. It needs careful handling, like dealing with a 680-year-old Icarus. You've got to really learn the code, not just know how to code."

[rjhancock]

Please don't do this inside of a conditional block. If you're going to do a stream, please do so either inside or outside of it.

[Scoppio]

I'd not do this kind of code change without also adding tests to make sure they keep doing what they were supposed to do.

And hard agree with others, streams are great and I do like them alot, but they are to be used to manipulate data, not to make flow control, they should be used to remove code branching and are great at that, using it in conditionals is really a bad form of its use.

     if (ae.hasC3() || ae.hasC3i() || ae.hasActiveNovaCEWS() && 
                    game.getEntitiesVector().stream().anyMatch(en ->
                    !en.isEnemyOf(ae)
                    && en.onSameC3NetworkAs(ae)
                    && Compute.canSee(game, en, target))) 
     { 

Should be something instead like this:

var foobar =  game.getEntitiesVector().stream().filter(en ->
   !en.isEnemyOf(ae)
   && en.onSameC3NetworkAs(ae)
   && Compute.canSee(game, en, target));
   
if (ae.hasC3() || ae.hasC3i() || ae.hasActiveNovaCEWS() && foobar)  {