IPC Refactor #771
IPC Refactor #771
Conversation
VMSolidus
added
Size: 3-Medium
DangerRevolution
requested review from
DEATHB4DEFEAT,
Peptide90,
Pspritechologist,
OldDanceJacket and
DangerRevolution
|
|
||
| if (entityManager.HasComponent<EncryptionKeyHolderComponent>(target)) | ||
| { | ||
| var encryption = new InternalEncryptionKeySpawner(); |
There was a problem hiding this comment.
Is it really a good approach to make a new instance of EntitySystem? You should use IoC instead.
| || !_silicon.TryGetSiliconBattery(uid, out var drinkerBatteryComponent) | ||
| || !TryComp(uid, out PowerCellSlotComponent? batterySlot) | ||
| || !TryComp<BatteryDrinkerSourceComponent>(args.Target.Value, out var sourceComp) | ||
| || sourceComp is null |
There was a problem hiding this comment.
sourceComp can't be null since TryComp<BatteryDrinkerSourceComponent>(args.Target.Value, out var sourceComp) already checks for it.
| if (args.Cancelled || args.Target == null | ||
| || !TryComp<BatteryComponent>(args.Target.Value, out var sourceBattery) | ||
| || !_silicon.TryGetSiliconBattery(uid, out var drinkerBatteryComponent) | ||
| || !TryComp(uid, out PowerCellSlotComponent? batterySlot) | ||
| || !TryComp<BatteryDrinkerSourceComponent>(args.Target.Value, out var sourceComp) | ||
| || sourceComp is null | ||
| || !_container.TryGetContainer(uid, batterySlot.CellSlotId, out var container) | ||
| || container.ContainedEntities is null) |
There was a problem hiding this comment.
Also this checks should probably go in order from args.Target.Value checks to uid checks to increase readability.
| if (!args.CanAccess || !args.CanInteract | ||
| || !TryComp<BatteryDrinkerComponent>(args.User, out var drinkerComp) | ||
| || !TestDrinkableBattery(uid, drinkerComp) | ||
| || !_silicon.TryGetSiliconBattery(args.User, out var drinkerBattery)) |
There was a problem hiding this comment.
out var drinkerBattery is not used anywhere in the code after you get it. You should either remove the check entirely or mark variable as not used (out _)
| if (args.User == uid | ||
| || !TryComp<SiliconComponent>(uid, out var siliconComp)) | ||
| || !HasComp<SiliconComponent>(uid)) |
There was a problem hiding this comment.
just for the style - it should be on the same line since it's not that long.
| { | ||
| if (args.Cancelled || args.Target == null | ||
| || !TryComp<BlindableComponent>(args.Target, out var blindComp) | ||
| || blindComp is { EyeDamage: 0 }) |
There was a problem hiding this comment.
instead of pattern check you can directly check blindComp.EyeDamage == 0
| public DamageSpecifier Damage; | ||
|
|
||
| [DataField(customTypeSerializer:typeof(PrototypeIdSerializer<ToolQualityPrototype>))] | ||
| public string QualityNeeded = "Welding"; |
There was a problem hiding this comment.
customTypeSerializerbe can be removed and the variable type should be then changed to ProtoId<ToolQualityPrototype>
| if (args.Cancelled || args.Used == null | ||
| || !TryComp<DamageableComponent>(args.Target, out var damageable) | ||
| || !TryComp<WeldingHealingComponent>(args.Used, out var component) | ||
| || damageable.DamageContainerID is null | ||
| || !component.DamageContainers.Contains(damageable.DamageContainerID) | ||
| || !HasDamage(damageable, component) | ||
| || !TryComp<WelderComponent>(args.Used, out var welder) | ||
| || !TryComp<SolutionContainerManagerComponent>(args.Used, out var solutionContainer) | ||
| || !_solutionContainer.ResolveSolution(((EntityUid) args.Used, solutionContainer), welder.FuelSolutionName, ref welder.FuelSolution)) |
There was a problem hiding this comment.
it should be probably better ordered. args.Used checks first, then args.Target.
| if (args.Handled | ||
| || !EntityManager.TryGetComponent(args.Used, out WeldingHealingComponent? component) | ||
| || !EntityManager.TryGetComponent(args.Target, out DamageableComponent? damageable) | ||
| || damageable.DamageContainerID is null | ||
| || !component.DamageContainers.Contains(damageable.DamageContainerID) | ||
| || !HasDamage(damageable, component) | ||
| || !_toolSystem.HasQuality(args.Used, component.QualityNeeded) | ||
| || args.User == args.Target && !component.AllowSelfHeal) |
|
|
||
| if (!TryComp<PowerCellSlotComponent>(uid, out var cellSlotComp)) | ||
| return; | ||
| args.Cancelled = !component.DoSiliconsDreamOfElectricSheep; |
There was a problem hiding this comment.
you should not do it like this. Either check if it's canceled first and return, or use &= instead of =
FoxxoTrystan
requested review from
FoxxoTrystan
and removed request for
DangerRevolution
Description
IPCs were ported from a codebase that didn't necessarily follow all of our repo's coding standards. And while I had done my part to cleanup as much of the system as was practical within the bounds of a Maintainer Review, there were a lot of things that I felt were inappropriate to leave to review, and wished to go over with a fine lense. Thus, here is my Refactor of IPC code.
Do not merge this without first testing that nothing was broken. Because I haven't tested it myself yet.