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

Couple issues with this library #2

Open
adelowo opened this issue Jan 17, 2017 · 3 comments
Open

Couple issues with this library #2

adelowo opened this issue Jan 17, 2017 · 3 comments

Comments

@adelowo
Copy link

adelowo commented Jan 17, 2017

Nice work man but i have got a couple complaints.

  • The re-defined array_key_exists function 1. That is at best confusing since PHP also provides that method (same name) and the implementations are different (yours check if multiple keys exist in an array WHILE PHP checks for just a single key/index ).. I feel it should be renamed.

Another issue with the function is this (), you are making use of function_exists but you didn't take into consideration that the helper functions are namespaced. It should be

if (!function_exists('PayantNG\Payant\array_key_exists') 
{
    //code block
}
  • Another issue is the inconsistency in code style. In some places, braces are on the same line 3 4, while in others, they are placed on the next line 5.

Another here is indentation inconsistency 6 (to be honest, the entire file, but here is a subset ). Might want to look into 7 and 8

  • Unit Tests ?

I don't mean to be an asshole

@JonathanItakpe
Copy link
Owner

JonathanItakpe commented Jan 18, 2017

Thank you for the suggestions, please make a pull Request.

The function is called array_keys_exist - php's inbuilt function is array_key_exists

but feel free to correct and make a PR.

@adelowo
Copy link
Author

adelowo commented Feb 4, 2017

Sorry been busy with other stuffs and didn't check this out.

Not too sure if i have the time to fix that but i should be able to get some free time on monday. Let's see how that goes

@JonathanItakpe
Copy link
Owner

Alright cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants