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

[REF] refactor on nasty Dedupe function #15830

Merged
merged 1 commit into from
Nov 15, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Minor refactoring

Before

Code hell

After

Yeah, still pretty bad.

Technical Details

I found with other pieces of refactoring static functions that creating a class to support the refactor
made it much easier as that way I could leverage the fact thatt classes have properties and
get away from the crazy param passing that characterizes a nest of static functions.

This adds a class for that purpose and moves a small chunk of code handling into the class.

The goal is to move the handling is done purely for the form back onto the form....

Comments

@civibot civibot bot added the master label Nov 12, 2019
@civibot
Copy link

civibot bot commented Nov 12, 2019

(Standard links)

I found with other pieces of refactoring static functions that creating a class to support the refactor
made it much easier as that way I could leverage the fact thatt classes have properties and
get away from the crazy param passing that characterizes a nest of static functions.

This adds a class for that purpose and moves a small chunk of code handling into the class.

The goal is to move the handling is done purely for the form back onto the form....
@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS I did a quick merge test with "related tables" groups/tags/contributions.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
      • Walked it through and it looks good, just not sure about the casting to int at line 1318. Are they sometimes not ints? Relatedly the docblock for the getter/setter says "mixed" but should be "int", unless they are mixed sometimes?
    • [r-maint] Unknown
    • [r-test] PASS

@seamuslee001 seamuslee001 merged commit 1c489cf into civicrm:master Nov 15, 2019
@seamuslee001 seamuslee001 deleted the dedupe4 branch November 15, 2019 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants