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

Linking belt refactoring #5206

Merged
merged 2 commits into from
May 27, 2017
Merged

Conversation

Phyma
Copy link
Contributor

@Phyma Phyma commented May 26, 2017

startLinkingbelt now uses canLinkBelt so if condition needs to be changed then you can do it on one position.

Nothing will change with this, only easier to edit condition in the future.
First time edit so please point out malpractice.

When merged this pull request will:

  • Describe what this pull request will do
  • Each change in a separate line
  • Include documentation if applicable
  • Respect the Development Guidelines

startLinking belt now uses canLinkBelt so if condition needs to be changed then you can do it on one position.
@Phyma Phyma changed the title Linking belt refatoring Linking belt refactoring May 26, 2017
jonpas
jonpas previously requested changes May 26, 2017

// Exit if the belt is full or empty
if ((_ammoCount == 0) || (getNumber (_magazineCfg >> "count") - _ammoCount) == 0) exitWith {false};
if(!_canLink) exitWith{};
Copy link
Member

Choose a reason for hiding this comment

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

Space before ( and {.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed thanks for input :)

@jonpas jonpas added the kind/cleanup Release Notes: **CHANGED:** label May 26, 2017
@jonpas jonpas added this to the 3.10.0 milestone May 26, 2017
Fixed requested change
@PabstMirror PabstMirror merged commit 5145b89 into acemod:master May 27, 2017
Copy link
Member

@jonpas jonpas left a comment

Choose a reason for hiding this comment

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

Oh, apparently I didn't press "submit" on this review. 😟


// Exit if the belt is full or empty
if ((_ammoCount == 0) || (getNumber (_magazineCfg >> "count") - _ammoCount) == 0) exitWith {false};
if ( !_canLink ) exitWith {} ;

// Check if the player has any of the same same magazines
// Calculate max ammo it can link
Copy link
Member

Choose a reason for hiding this comment

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

This section below is already included in FUNC(canLinkBelt) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey but I wanted to keep that FUNC(canLinkBelt) returned a bool and not a number.

So thats why i kept the lower part in both cases. But I could redo both of the files to keep them 100% unique?

Just say the word nad ill do it :)

Copy link
Member

@jonpas jonpas May 27, 2017

Choose a reason for hiding this comment

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

I think it would be a good idea to cut down on duplicate code if nothing else. As long as it doesn't make it more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


// Exit if the belt is full or empty
if ((_ammoCount == 0) || (getNumber (_magazineCfg >> "count") - _ammoCount) == 0) exitWith {false};
if ( !_canLink ) exitWith {} ;
Copy link
Member

Choose a reason for hiding this comment

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

if (!_canLink) exitWith {}; would be per our coding guidelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Release Notes: **CHANGED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants