Naming: getters, setters, methods

Yii 3.0 Design discussions
1

Currently in Yii 3 codebase there are many ways of naming methods:

  1. getX() - explicit getters
  2. setX() - explicit setters
  3. isX() or shouldX() - getters for booleans
  4. withX() or withoutX() - immutable “setters”
  5. Action-methods such as EmitterInterface::emit() or ErrorHandler::handleError()
  6. Not explicit setters such as Route::host() or Route::to()
  7. Not explicit getters such as MatchingResult::methods().

What do you think about all these? Should we try to define a convention?

2

Also hasX().
The question is a little unclear, though.
What’s wrong with Yii2 naming conventions ?
And how these can be improved in Yii3 ?

3

There’s nothing wrong with Yii 2 conventions except that method naming is not part of these.

The questions are meant to be:

  1. Are you OK with such naming variety?
  2. Should we regulate that stricter? For example, all getters should have get, all setters should have set (if mutable) or with (if immutable).
4

I don’t think we should force this convention everywhere. Some builders may be hurt by such strict convention ((new Query())->setFrom('table')?).

5 Likes
5

(new Query())->from('table')->getOne() A setter should always return void. I would prefer for functions wtihout set prefix, that they always return the original objet itself.
Except for get*, has*, is*, to* (e.g. toString() )

Google a bit about this: found an interesting opinion, I am not sold on it, have to check it again after having slept over it.


edit: made the example code show how I wished it to be, instead of how it looks like when not.

1 Like
6

i agree, in some cases set/get prefix looks as overhead. and is/has/with etc looks better than get* prefixed like getIsAdmin() vs isAdimn().

2 Likes
7

In this case it was used to provide virtual attribute - intended usage was $object->isAdmin.

8

It just first example that coming in my head

9

My humble opinion:
I like the point 2, in the way code will be much more readable, and stricter means less code error possibilities, but I am not really sure.

10

Here are my opinions

This should be used for strict setters that does nothing but setting/getting

These should reflect exactly what they say. For example isRunning() and shouldRun() should just do that, checking whether it is already running or we are allowed even to run at that point.

Same as above, me thinks

Am fine with name that reflects what it is doing

On Route is should be getHost() really host sounds like a command to host than a getter. So is to which is I don’t know what it does from reading it. Have to consult docs (which is bad. Mehods should try to be self docmenting)

methods should be getMethods()

That is my thoughts

11

Route is currently used like the following:

$routes = [
    Route::get('/test/{id:\w+}')
        ->to(new ActionCaller(SiteController::class, 'testParameter', $container))
        ->name('test_id');
]

With stricter names it will become:

$routes = [
    Route::get('/test/{id:\w+}')
        ->withTo(new ActionCaller(SiteController::class, 'testParameter', $container))
        ->withName('test_id');
]
12

Definitely current version looks better.

1 Like
13

Except you can call if ($obj->isAdmin) on getters starting with get which is a time saver and shorter than if ($obj->isAdmin())

L.E: Nevermind, this has been discussed.

14

I’m totally fine with the current way.

Enforcing standards, just for the sake of it, doesn’t seems a good enough reason for adjusting a perfectly good working system

3 Likes
15

sound good
but for “fluent interface” maybe need special convention

name convention is for better human readable
so the method name should self explain

16

commonly the method name has following pattern

verbNoun

noun is context
if noun is hide, mean use object it self as context
i.e
Log::write() mean Log::writeLog()

use adjective is for more expline method, i.e Log::writeAllLogs()

use single adjective as method name is multiple interpretations

ie. $query->all() , What does it mean?
on current DB, this method fetch all record
it’s more better use $query->fetchAll()
or if all() method not return record but only condition
the code become $query->all()->fetch().

they more readable

17

No, if you want to do something like method chaining (new Object())->setFoo()->setBar();

18

I think for the PHP world immutable setters are still a pretty new practice that it really may be worth to enforce a with prefix - even if the interface is not so nice anymore.

I see tons of mistakes like this:

// Stupid example, I know, but you get the idea
$route = Route::get('/complex', [SomeController::class, 'complex']);
$names = require 'mynames.php';
$route->name($names['complex']);

return [
    $route,
];

In fact the concept of “immutable setters” must be made very very clear and all places where it’s used must have big warning signs or something. I would even question, why they are used in the Route class at all.

19

They’re currently used everywhere except otherwise is needed and the convention is there: https://github.com/yiisoft/docs/blob/master/010-code-style.md#immutable-methods

with could be omitted though where it makes sense to do so.

20

Departure from the naming convention is a source of potential errors. If we accept that an immutable property is prefixed with with, then we must strictly follow this.
If we want to move away from the convention, then perhaps this property should not be made immutable? In fact, I see it as a trend towards everything new. At first glance this seems cool, but in the future it is possible that it will look like a fashion of the past (I mean immutable properties).

Without going into context, you might think that MatchingResult::methods() is a setter.