PHP coding standards at work: Insane, or am I?

Question

I prefer coding standards to be logical. This is my argument for why the following set of standards are not.

I need to know one of two things: (1) why I'm wrong, or (2) how to convince my team to change them.

camelCase: Functions, class names, methods, and variables must be camelCase.

• Makes it hard to differentiate between variables and classes
• Goes against PHP's lowercase/underscored variables/functions and UpperCamelCase classes

Example:

$customerServiceBillingInstance = new customerServiceBillingInstance(); // theirs
$customer_service_billing_instance = new CustomerServiceBillingInstance();

Functions/methods must always return a value (and returned values must always be stored).

This appears on hundreds of our php pages:

$equipmentList = new equipmentList();
$success = $equipmentList->loadFromDatabase(true, '');
$success = $equipmentList->setCustomerList();
$success = $equipmentList->setServerList();
$success = $equipmentList->setObjectList();
$success = $equipmentList->setOwnerList();
$success = $equipmentList->setAccessList();

The return value is rarely used but always stored. It encourages the use of copy-and-paste.

No static methods

Lines like the following appear thousands of times in the codebase:

$equipmentList = new equipmentList();
$success = $equipmentList->loadFromDatabase();

I would prefer:

$equipmentList = equipmentList::load();

What reason is there to not use static methods or properties? Aren't static methods responsible for non-instance-specific logic? Like initializing or populating a new instance?

Your code is not OOP unless everything returns an object

There's a piece of code that performs a query, checks it several ways for errors, and then processes the resulting array. It is repeated (copied+pasted) several times, so I put it in the base class. Then I was told returning an array is not OOP.

How do you defend these practices? I really do need to know. I feel like I'm taking crazy pills.

If you can't defend them, how do you convince the adamant author they need to be changed?

Answer 1

I would suggest trying to list down the goals of your coding standards, and weigh them down depending on which goals is the most important and which goals are less important.

PS: I don't speak PHP, so some of these arguments may contain blatantly incorrect PHP code.

camelCase: Functions, class names, methods, and variables must be camelCase.

Workplace's Apparent Reason: "consistency" at the cost of "information density in name".

Argument:

1) Since 'new' keyword should always be followed by a class, then you can easily spot illegal instantiation, e.g.:

$value = new functionName();
$value = new local_variable();
$value = new GLOBAL_VARIABLE();

would raise an alarm, because 'new' should be followed by a TitleCase name.

$value = new MyClass(); // correct

Relying on Case makes it easy to spot these errors.

3) Only functions can be called, you can never call variables. By relying on Case Rule, then we can easily spot fishy function calls like:

$value = $inst->ClassName();
$value = $inst->instance_variable();
$value = $GLOBAL_VARIABLE(); 

3) Assigning to a function name and global variables is a huge deal, since they often lead to behavior that is difficult to follow. That's why any statement that looks like:

$GLOBAL = $value;
$someFunction = $anotherFunction;

should be heavily scrutinized. Using Case Rule, it is easy to spot these potential problem lines.

While the exact Case Rule may vary, it is a good idea to have different Case Rule for each different type of names.

Functions/methods must always return a value (and returned values must always be stored).

Workplace's Apparent Reason: apparently another rule born out of blind consistency. The advantage is that every line of code that isn't a flow control (e.g. looping, conditionals) is an assignment.

Argument:

1) Mandatory assignment makes unnecessary long lines, which harms readability since it increases the amount of irrelevant information on screen.

2) Code is slightly slower as every function call will involve two unnecessary operation: value return and assignment.

Better Convention:

Learn from functional programming paradigm. Make a distinction between "subroutine" and "functions". A subroutine does all of its works by side-effect and does not return a value, and therefore its return value never need to be stored anywhere (subroutine should not return error code; use exception if it is really necessary). A function must not have any side-effect, and therefore its return value must be used immediately (either for further calculations or stored somewhere). By having side-effect free function policy, it is a waste of processor cycle to call a function and ignoring its return value; and the line can therefore be removed.

So, there is three type of correct calls:

mySubroutine(arg);            // subroutine, no need to check return value
$v = myFunction(arg);         // return value is stored
if (myFunction(arg)) { ... }  // return value used immediately

you should never have, for example:

$v = mySubroutine(arg);  // subroutine should never have a return value!
myFunction(arg);         // there is no point calling side-effect free function and ignoring its return value

and they should raise warning. You can even create a naming rule to differentiate between subroutine and functions to make it even easier to spot these errors.

Specifically disallow having a "functiroutine" monster that have both a side-effect and return value.

No static methods

Workplace Apparent Reason: probably someone read somewhere that static is evil, and followed blindly without really doing any critical evaluation of its advantages and disadvantages

Better Convention:

Static methods should be stateless (no modifying global state). Static methods should be a function, not subroutine since it is easier to test a side-effect-free function than to test the side-effects of a subroutine. Static method should be small (~4 lines max) and should be self-contained (i.e. should not call too many other static methods too deeply). Most static methods should live in the Utility class; notable exceptions to this is Class Factories. Exceptions to this convention is allowed, but should be heavily scrutinized beforehand.

Your code is not OOP unless everything returns an object

Workplace Apparent Reason: flawed understanding of what is OOP.

Argument:

Fundamental datatypes is conceptually also an object even if a language's fundamental datatype doesn't inherit from their Object class.

Answer 2

If you can't defend them, how do you convince the adamant author they need to be changed?

By giving strong/valid arguments! Still I think you should only change them when your arguments are really strong! Because most of the programmers at work are used to these coding standards which is a big point why to use them.

==

Like others told before this is pretty subjective, but these are mine opinions/arguments.

1. camelCase: Functions, class names, methods, and variables must be camelCase.

I would use the PHP style if I code in PHP and the Camelcase style if I code in Java. But it does not matter which style you choose as long as you stay consistent.

2. Functions/methods must always return a value (and returned values must always be stored).

This is nonsense in my opinion. In almost all programming languages you have some sort of 'void' type. But from a testing point of view most of the times it is useful if your function are side effect free. I don't agree that your production code should always use the return value especially if it does not have any use.

3. No static methods

I would advice you to read static methods are death to testability from misko

During the instantiation I wire the dependencies with mocks/friendlies which replace the real dependencies. With procedural programing there is nothing to “wire” since there are no objects, the code and data are separate.

Although PHP is a dynamic language so it is not really a big problem. Still the latest PHP does support typing so that I still think most of times static methods are bad.

4. Your code is not OOP unless everything returns an object

I believe(not 100% sure) a truly OOP language should do this(return an object), but I don't agree with this like a of like languages like for example Java(which I believe is not trully OOP). A lot of the times your methods should just return primitives like String/Int/Array/etc. When you are copying and pasting a lot of code it should be a sign that something is not totally right with your design. You should refactor it(but first have a tests(TDD) ready so that you don't break any code).