Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[medical] Code cleanup using SQFLint #6485

Merged
merged 10 commits into from
Aug 2, 2018

Conversation

dedmen
Copy link
Contributor

@dedmen dedmen commented Jul 30, 2018

This is some minor cleanup of SQFLint warnings. And one actual bug of a wrong variable being used.
I also want to cleanup the spacing after the header.

Tasks left that I want in here.

1.

We have mixed

<header>
params []
<header>

params []
<header>


params []

Through the files. I'd clean it up to a single newline after the header. Then maybe file local #defines, another newline and then params. But for that I need #6484 merged first.

2.
Also there are tons of unused variables. Especially params with all entries defined although not all are needed.
I'd clean them up like this
params ["_var1", "_var2" /*, "_var3", "_var4" */] in case where the last ones aren't needed.
Or
params ["" /*_var1*/, "" /*_var2*/, "_var3", "_var4"]
in case where ones in the middle aren't needed. What do you think about that?

3.

private _countFirstAidKit = {_x == "FirstAidKit"} count items _unit;

This is horrible. As I already wrote in Slack.

Maybe add a ACE common function like. "getNumberOfItem" the returns the count of how many of a specific item type the unit has in inventory?
Would we want weaponItems or assignedItems for that too? Maybe only on condition like in the CBA func?

4.

params ["_caller", "_target", ["_drag", false]];

the _drag thing is not implemented. Shouldn't such things be marked as //#TODO in the code to make them easier to find and harder to forget?

@kymckay kymckay added status/WIP kind/cleanup Release Notes: **CHANGED:** labels Jul 30, 2018
@kymckay kymckay added this to the Medical Rewrite milestone Jul 30, 2018
@dedmen
Copy link
Contributor Author

dedmen commented Jul 30, 2018

Last commit has my proposed solution for 3.
But it's very specialized. It could be generalized further by adding weaponItems/assignedItems as I said in first post.

Still waiting for feedback about 2. but we can probably just leave that out for now. We want to do a optimization and cleanup run later anyway.

_return = switch (_status) do {

private _status = _unit getVariable [QEGVAR(medical,triageLevel), -1];
private _return = switch (_status) do {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of a return variable is pointless in this function since the switch is at the end anyway 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about replacing the switch by an ARRAY param [_status, default] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah. Would then call localize for all entries everytime

@kymckay
Copy link
Member

kymckay commented Jul 30, 2018

I like the common function solution 👍

@dedmen dedmen changed the title [WIP] [medical] Code cleanup using SQFLint [medical] Code cleanup using SQFLint Jul 30, 2018
* Doesn't count assignedItems, weapons, weapon attachments, magazines in weapons
*
* Arguments:
* 0: unit <OBJECT>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought the same actually. But I just copied it from another function :D

* 1: Classname of item (Case-Sensitive) <STRING>
*
* Return Value:
* itemCount <NUMBER>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Item Count

@jonpas
Copy link
Member

jonpas commented Jul 30, 2018

I don't like the empty line between header and file local defines. Include above header has no newline, amd header separates it nicely already.

@dedmen
Copy link
Contributor Author

dedmen commented Jul 30, 2018

@jonpas so.. #define directly after header without newline? Or before header?

@jonpas
Copy link
Member

jonpas commented Jul 30, 2018

After, without the newline I'd say.

@kymckay
Copy link
Member

kymckay commented Aug 2, 2018

@dedmen Is this ready for review?

@dedmen
Copy link
Contributor Author

dedmen commented Aug 2, 2018

yes. I'll fix the conflicts now. Nothing else I want to do here.

@kymckay kymckay merged commit 7a0a00e into acemod:medical-rewrite Aug 2, 2018
BaerMitUmlaut pushed a commit that referenced this pull request Aug 5, 2019
* Use private
* Fix wrong spacing
* Fix wrong variable being used
* Cleanup empty line after header
* ace_common_fnc_getCountOfItem
* Remove useless _return variable
* Naming
@PabstMirror PabstMirror modified the milestones: Medical Rewrite, 3.13.0 Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Release Notes: **CHANGED:** status/WIP
Projects
Development

Successfully merging this pull request may close these issues.

5 participants