[Enhancement] Correct use searchable names

[peter-gribanov]

Fix for Use searchable names.

This is bullshit

// Declare them as capitalized `const` globals.
interface DateGlobal {
    const SECONDS_IN_A_DAY = 86400;
}
 
addExpireAt(DateGlobal::SECONDS_IN_A_DAY);

You created a global constants. Is not a good.
Any class/function can depend on these constants and it will be difficult to put them into a separate component.
Your constants is not a informative.
Requires each time to create a new constant if you have complex conditions. Example:

interface DateGlobal {
    const SECONDS_IN_A_HOUR = 3600;
    const SECONDS_IN_A_DAY = 86400;
    const SECONDS_IN_A_WEEK = 604800;
    //const SECONDS_IN_A_MONTH = ???;
    //const SECONDS_IN_A_YEAR = ???;
    const SECONDS_IN_A_TEN_MINUTES = 600;
    const SECONDS_IN_A_FOUR_WEEK = 2419200;
    const SECONDS_IN_A_SOMETHING = 1036800;
    // ...
}

If you really want to use constants, you must declare them in the context of use.

class ArticleController
{
    /**
     * By default the response is cached in 3600 seconds (1 hour).
     *
     * @var int
     */
    const DEFAULT_CACHE_TTL = 3600;

    public function showAction(Article $article)
    {
        $ttl = self::DEFAULT_CACHE_TTL;

        // change default TTL in context if need
        // ...

        $response = new Response();
        $response
            ->setMaxAge($ttl)
            ->setSharedMaxAge($ttl)
            ->setExpires(new \DateTime(sprintf('+%d seconds', $ttl)))
        ;

        // ...
    }
}

There is no misunderstanding.

[TomasVotruba]

I think point of this example is "put magical numbers to explicitly named constants".

It's not about time shifting. You could create new example for that though.

[piotrplenik]

Exactly, what @TomasVotruba said. Goal in this point was to give readable variables (date was here only example).

Based on your example, you could do something like that:

class ArticleController
{
    const SECONDS_IN_A_DAY = 86400;

    public function showAction(Article $article)
    {
        $response = new Response();
        $response
            ->setMaxAge(self::SECONDS_IN_A_DAY)
            ->setSharedMaxAge($ttl)
            ->setExpires(new \DateTime(sprintf('+%d seconds', $ttl)))
        ;

        // ...
    }
}

Then you have clear and readable information.
Hope I help.

[peter-gribanov]

It's not good because you lost the context.
The constant SECONDS_IN_A_DAY outside the context does not make sense.

Then it's better this way:

class ArticleController
{
    const SECONDS_IN_A_DAY = 86400;

    const DEFAULT_CACHE_TTL = self::SECONDS_IN_A_DAY;

    public function showAction(Article $article)
    {
        $ttl = self::DEFAULT_CACHE_TTL;

        // change default TTL in context if need
        // ...

        $response = new Response();
        $response
            ->setMaxAge($ttl)
            ->setSharedMaxAge($ttl)
            ->setExpires(new \DateTime(sprintf('+%d seconds', $ttl)))
        ;

        // ...
    }
}

But what is the meaning of the constant SECONDS_IN_A_DAY in this case?
If the code does not carry a payload, it must be deleted.
Just leave a comment here.

[peter-gribanov]

By the way. Some write like this:

addExpireAt(60*60*24*7); // 1 week

I do not approve of this way, but it is also more understandable.

And do not think that all the programmers are idiots.
Every programmer who respects himself knows what the numbers 3600 and 86400 mean.

[TomasVotruba]

By the way. Some write like this:

addExpireAt(60*60*24*7); // 1 week

That could be actually nice in constant value, in case sb would like to add WEEKEND_IN_SECONDS etc.

Every programmer who respects himself knows what the numbers 3600 and 86400 mean.

I do respect myself, but I don't want to think about stuff I don't have to ;)

[peter-gribanov]

I do respect myself, but I don't want to think about stuff I don't have to ;)

Why think, if you can know?
60 * 60 * 24 = 3600 is same as 2 * 2 = 4.
You don't think, you just know.

[TomasVotruba]

Why think, if you can know?

I guess my brain is just structured differently than yours.

I try to find lowest common denominator so most people could understand the knowledge with minimal thinking possible.

[peter-gribanov]

Is it too difficult for you? Okay.

Let's return to the original problem.

The use of global constants contradicts the statement: Don't write to global functions
Global constants are just as bad as a global functions.

Hence, we do not have to use global constants.
This is also a global constant: DateGlobal::SECONDS_IN_A_DAY

Hence, we can use constants only in the context of their use.
Hence the meaninglessness of constants of type SECONDS_IN_A_DAY follows, since their meaning is understandable from the context.

Is not context enough to understand what these magic numbers mean?

class ArticleController
{
    const DEFAULT_CACHE_TTL = 3600;

    // ...
}

Perhaps you don't have enough experience to read this code.
Let's consider another example, if this one is not enough for you.

class DoctrineContestantRepository implements ContestantRepository
{
    public function totalAwaitingContestants()
    {
        return $this->repository->countOf(
            new ContestantAwaitingModerationSpec(),
            new Cache(600)
        );
    }
}

Is it not clear what new Cache(600) means?
In my opinion, the context is more than enough.
Usually, SQL queries cached in seconds. So it's a 600 seconds.

If in doubt. You can always check.
This class is very often used in repositories, so it is quickly remembered if it's not immediately understandability.

Another example.
This is the factory that creates and configures the schedule.

class ScheduleFactory
{
    public static function createSchedule()
    {
        return new Schedule([
            new WeekdayRule(0, 2, 1000),
            new WeekdayRule(2, 7, 1200),
            new WeekdayRule(7, 10, 500),
            new WeekdayRule(10, 19, 400),
            new WeekdayRule(19, 22, 500),
            new WeekdayRule(22, 24, 700),
            new HolidayRule(0, 3, 700),
            new HolidayRule(3, 9, 1300),
            new HolidayRule(9, 19, 400),
            new HolidayRule(19, 23, 500),
            new HolidayRule(23, 24, 700),
        ]);
    }
}

The third parameter for classes WeekdayRule and HolidayRule is the number of seconds.
Do you seriously think that creating constants to describe seconds will add understandability?
It is possible to move the schedule parameters to a config file, but does this add understandability?

PS: The configuration options for the schedule are not included in the configuration file because this is an unnecessary overcompliance.

[peter-gribanov]

If such an entry is not clear to you

new Cache(600)

Can such a entry be more clear to you?

new Cache('1 hour 10 minutes')

https://3v4l.org/NmiYt

This of course perversion, but.

[peter-gribanov]

class Cache
{
    private $ttl;

    public function __construct($time)
    {
        $zero = new \DateTime('01-01-1970', new \DateTimeZone('UTC'));
        $this->ttl = $zero->modify($time)->getTimestamp();
    }

    public function ttl()
    {
        return $this->ttl;
    }
}

Usage

$cache = new Cache('1 hour 10 minutes');

var_dump($cache->ttl());

Print

int(4200)

It's just an idea. This is not a call to action.

I and everyone i've ever worked with, still believes that the numbers are fairly clear.

[peter-gribanov]

By the way, what about chmod?

// What the heck is 0644 for? 
chmod($filename, 0644);

This entry is understandable for you or maybe you also need to use constants?

[scaytrase]

I and everyone i've ever worked with, still believes that the numbers are fairly clear.

I think this is a problem of context. 0664 for chmod is related to permissions, also the parameter says that this should be a permission constant, so if you know the POSIX permissions - you can easily use needed const

But for time is not always clear. what is 600? 10 minutes? 10 hours? you should deep into documentation (if it exists) to learn which values is represented by this number. Having named constant is a bit easier for this cases to match what you mean and what is needed.

This entry is understandable for you or maybe you also need to use constants?

TBH I'd rather use here bitwise operations, like

PERMISSION_OWNER_READ | PERMISSION_OWNER_WRITE | PERMISSION_GROUP_READ | PERMISSION_OTHER_READ

This will make this clear and portable even for non-POSIX permission systems

[peter-gribanov]

@scaytrase I changed example to use bit masks #80.
I think that such example will be easier and will cause less controversy.

[peter-gribanov]

This problem is solved in #80

[TomasVotruba]

👍