Code style fixers should be secondary

[dextermb]

What you would like to change/add

I'd like there to be a sentence or two where it is defined that you should follow the code styles rather than pushing up not up to standard code and waiting for CI to resolve it.

Why you would like to change/add this

I believe that you should write code to the internal standard from the get-go when writing code for the company. Style fixers should be a back up to pick up on anything that you might of missed or didn't know was an internal standard.

• For projects that don't have a style fixer it means that everyone who goes to maintain the project will have a better understanding of the codebase as it follows a pattern seen in every other project.
• For older projects new code should follow the internal standards even if the original codebase doesn't, this again makes it easier to maintain in the future. At some point someone should go through and tidy up older parts of the codebase.

Not following standards does not help with the mindframe of developing or maintaining a project.

Checklist

• I have checked that this is not a duplicate issue.

Edit:

Proposed styleguide TLDR:

Any code that enters the acting master branch must meet the internal code standards. If the project does not have CI setup to fix styling issues you should fix any issues before committing.

Vote

👍 Yes
👎 No

[AlexCatch]

I feel like everyone should write to the standard, the more you do it, the more it becomes second nature.

But at the same time I don't think it matters that much as long as the code in source control follows the style guide, tools like StyleCI & Prettier make this easy, anyone can style their local codebase as and how they like but when it comes to pushing up to source control, Prettier & StyleCI ensure it meets the companies standards.

• For older projects new code should follow the internal standards even if the original codebase doesn't, this again makes it easier to maintain in the future.

Yeah, seeing broken windows doesn't mean you should start throwing rocks.

[ash123456789]

PHPStorm has a bunch of tools that read files within your project root directory to enforce a specific style. E.g. phpcs.xml rulesets. I'm sure there are also tools out there to format CSS/SASS/LESS as well. This will also play nicely with GitLab CI styling checks, as well as other tools around the web.

For those of you not using PHPStorm, switch to it. 👍

[NilesB]

I've got to disagree here. You can't let minor things like which line the curly brace is on affect deadlines, it isn't commercially viable.

I think that CI should always catch styling issues. Code that isn't up to standard is exactly the same as code that isn't but with different spacing etc... there's nothing worse about that code and anything wrong with can be fixed automatically and should be. What we should be focussing on is bad coding practises, inefficient and insecure code. These things cannot be fixed automatically.

[AlexCatch]

@NilesB I completely agree with what you're saying and having a consistent codebase is a really important thing.

If you don't have standards, you don't have consistency, if you don't have consistency, you don't have reliablity. If you don't have reliability, things start to break a lot.

It's very easy for a project to start showing signs of rot if there is 2 or 3 different styles of code being written in the same project, it might not be important in the current deadline but it contributes to the rot of a project which will can increase the time it takes to work on it in the future & impacts deadlines there.

I completely agree that styling should not be a manual thing at all though with tools like Prettier & StyleCI.

[dextermb]

PHPStorm has a bunch of tools

Once of the issues I've noticed is people not bothering to even re-format code using PHPStorm or get a PHPStorm config that follows our internal standards.

For those of you not using PHPStorm, switch to it.

🎉

I've got to disagree here. You can't let minor things like which line the curly brace is on affect deadlines, it isn't commercially viable.

It's not just curly braces though, if people follow the standards and are proud of what you write then good code structure comes naturally from just the way code gets written.

What we should be focussing on is bad coding practises, inefficient and insecure code. These things cannot be fixed automatically.

I agree. These are definitely things that should be looked for and resolved by a developer. Ideally these would never happen.

With the mindset of writing code in a standard I feel that the code structure would naturally follow a pattern and be better. Also makes it easier for people to spot these insecurities when maintaining the code or reviewing it.

[dextermb]

I completely agree that styling should not be a manual thing at all though with tools like Prettier & StyleCI.

It's also worth noting that with PHPStorm you can set it to re-format on save 🙌

[AlexCatch]

@dextermb I've found that doing that causes PHPStorm to re-index on every save, have you found a way around that?

[NilesB]

It's not just curly braces though, if people follow the standards and are proud of what you write then good code structure comes naturally from just the way code gets written.

What standards affect code structure? Standards such as PSR-2 generally only mention newline, spaces and line lengths. None of these things affect my mindframe while developing. They don't specify things such as number of lines in functions or minimising number of iterations that actually promote better coding practises.

Also makes it easier for people to spot these insecurities when maintaining the code or reviewing it.

They shouldn't have to opportunity to due to CI. This is a non issue for GitHub due to StyleCI and something similar should be implemented on GitLab.

[dextermb]

@dextermb I've found that doing that causes PHPStorm to re-index on every save, have you found a way around that?

I exclude public from the directories to be index. This seems to help, as it doesn't check all of public if app.js gets re-built for example.

What standards affect code structure?

I believe that if you're focusing on following a standard, then you're more likely to understand what you're writing rather than just going in automatic. Hopefully structuring code to be easier to read too.

They don't specify things such as number of lines in functions or minimising number of iterations that actually promote better coding practises.

Maybe, but what we could do is enable this in phpcs. There's max methods per class, max lines per method and more.

They shouldn't have to opportunity to due to CI. This is a non issue for GitHub due to StyleCI and something similar should be implemented on GitLab.

Older projects that are still being maintained probably don't have style CI set up. Hence CI should be secondary.

[NilesB]

I believe that if you're focusing on following a standard, then you're more likely to understand what you're writing rather than just going in automatic. Hopefully structuring code to be easier to read too.

How does putting the curly brace on the next line or having a trailing comma make you understand your code better?

[dextermb]

I believe that if you're focusing on following a standard, then you're more likely to understand what you're writing rather than just going in automatic. Hopefully structuring code to be easier to read too.

How does putting the curly brace on the next line or having a trailing comma make you understand your code better?

I don't have an example on hand to show you the differences, but there's plenty of articles/posts about how standardizing code makes it easier to understand:

• Writing Beautiful Code
• Top 15+ Best Practices for Writing Super Readable Code
• 7 problems that might prevent your PHP code from being awesome
• ...and more

As mentioned, it's not just about putting a brace on a newline. It's how following a standard changes a developers mindset on how to write code in general.

[AlexCatch]

@NilesB I think what he's getting at is that it removes the guess work - the code is then predictable & you know what to expect in a code base, I do feel there is an aspect in having pride in your work when you make it look presentable though.

But as discussed, it doesn't really matter much as long as the code on source control is formatted to the company standard.

A local dev environment is like a bedroom, it's no one's else's business what goes on in there, just keep it out the local community park.

[NilesB]

@AlexCatch exactly, what I'm trying to put across is that is doesn't matter what the developer writes locally as long as the code is formatted to the company standards when another developer comes to look at it. This is can be achieved automatically and going back to the original issue:

I'd like there to be a sentence or two where it is defined that you should follow the code styles rather than pushing up not up to standard code and waiting for CI to resolve it.

I disagree with this.

[AlexCatch]

@NilesB Agree, anything that can be automated should be.

But it doesn't make the need for the developer to at least attempt to follow standards irrelevant though.

Becoming complacent in that area I feel could lead to an overall lax attitude in standards in other areas.

One of the ways in which I feel you should show pride in your work is by writing good quality efficent code that looks good & works.

In respect to having it in the technologies repo, it's entirely unenforcable, it's incredibly hard to ascertain intent. Can you be sure the dev actually didn't bother fixing it on purpose or that they simply forgot?

It's probably best to have it as part of a bigger talk in relation to pride in your work / the standards you hold yourself to as a developer here.

[ash123456789]

While we're on the topic of CI, considering we're moving over to GitLab it would be worth making the styling checks first in the pipelines before any form of asset building and test running. This is so we can resolve styling issues before anything else rather than having all the other stages pass only for it to fail on the last stage, which would be style checking (using NQ as an example).

TLDR:

Merge Request -> Pipeline starts -> Styling checks -> Asset building -> Run tests.

I'll look into automated styling fixing further, to see if it is at all possible in GitLab.