Skip to content

[WIP] Disallow complex conditional expressions #21

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

Closed
wants to merge 1 commit into from
Closed

[WIP] Disallow complex conditional expressions #21

wants to merge 1 commit into from

Conversation

carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel changed the title [WIP] Add long long conditional expressions [WIP] Add long conditional expressions Jan 29, 2018
@Majkl578 Majkl578 added this to the 3.0.0 milestone Feb 5, 2018
@alcaeus
Copy link
Member

alcaeus commented Feb 6, 2018

@Majkl578 you added this to the 3.0 milestone - is there a sniff we can use or is it just considered a guideline?

@kukulich
Copy link
Contributor

kukulich commented Feb 6, 2018

I can prepare the sniff. However there are already some new useful sniffs so I do a release of Slevomat CS so everyone can test them and report bugs.

It's also possible to merge some PR here and test them in dev-master together.

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2018

Deferred for now

@Ocramius Ocramius modified the milestones: 3.0.0, 4.0.0 Feb 6, 2018
@kukulich
Copy link
Contributor

kukulich commented Feb 8, 2018

Can someone please make a better explanation what this sniff should do?

Examples would be perfect :)

@carusogabriel
Copy link
Contributor Author

Maybe doctrine/dbal#2980 (comment) and #18 (comment) is something to start but is basically split long conditions (maybe a Sniffers configurable with how many checks are inside it?)

@lcobucci
Copy link
Member

lcobucci commented Feb 9, 2018

@carusogabriel @kukulich it might be quite tricky to implement the automatic fix fo this though...

@Ocramius
Copy link
Member

Ocramius commented Mar 3, 2018

I'm removing the 4.0.0 milestone here.

I don't think this is worth adding an auto-fixer, btw.

@Ocramius Ocramius removed this from the 4.0.0 milestone Mar 3, 2018
@carusogabriel
Copy link
Contributor Author

carusogabriel commented Apr 11, 2018

Should we give up on this one?

@deeky666
Copy link
Member

The question is what does "long" mean here. Does it mean to disallow boolean operators like && and ||?

@greg0ire
Copy link
Member

IMO long means longer than 120 chars. I don't understand what's the proposed replacement, a code sample would help, because the related issues shows something quite simple, that doesn't seem applicable to if statements containing big blocks of code.

@Ocramius
Copy link
Member

IMO long means longer than 120 chars

long as in NPath complexity > 8 or such.

if ($foo === 1 && $bar === 2 && $baz === 3 && $tar === 4) { // too long - 16 paths
if ($foo === 1 && $bar === 2 && $baz === 3) { // acceptable - 8 paths

@greg0ire
Copy link
Member

long as in NPath complexity > 8 or such.

A better wording might be "complex" instead of "long" then :P

@Ocramius Ocramius changed the title [WIP] Add long conditional expressions [WIP] Disallow complex conditional expressions Apr 11, 2018
@Ocramius
Copy link
Member

@greg0ire adapted title 👍

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

Successfully merging this pull request may close these issues.

8 participants