Add statistic service and increment usage count #75

alexzerah · 2018-10-02T14:36:59Z

This commit fixes the issue #11
I choose to add incrementing at the finish function so the incrementUsageCount method needs to be add to all Apps (i.e. Slack, Discord, ...)

pyrech · 2018-10-02T14:40:51Z

src/Application/SlackApplication.php

@@ -90,6 +93,8 @@ public function sendAdminMessage(SecretSanta $secretSanta, string $code, string

    public function finish(SecretSanta $secretSanta)
    {
+        $this->statistic->incrementUsageCount();


I would move this logic where the finish method is called instead of duplicating the call and the dependency in each application.

pyrech · 2018-10-02T14:41:43Z

README.md

@@ -28,6 +28,7 @@ Then launch this command:

 The application should now be running on http://127.0.0.1:8000.

+Tests are made with PHPUnit  


missing dot

pyrech · 2018-10-02T14:42:49Z

src/Statistic.php

+{
+    /**
+     * @var Client
+     */


No need for this phpdoc 😉

pyrech · 2018-10-02T14:43:25Z

src/Statistic.php

+    public function incrementUsageCount()
+    {
+        //If the key does not exist, it is set to 0 before performing the operation
+        $this->client->incr('usageCount');


Should we also add a monthly/yearly counter

welcoMattic · 2018-10-02T14:45:05Z

config/services.yaml

@@ -37,6 +37,9 @@ services:
                - '@JoliCode\SecretSanta\Application\SlackApplication'
                - '@JoliCode\SecretSanta\Application\DiscordApplication'

+    Jolicode\SecretSanta\Statistics:
+            public: true


Il y a un niveau d'indentation de trop il me semble

Are you sure it needs to be public ?

Are you sure you need to declare it ? (because there is autodiscovery in this project)

welcoMattic · 2018-10-02T14:45:20Z

src/Application/DiscordApplication.php

+    /**
+     * @var Statistic
+     */
+    private $statistic;


Pas besoin de PHPDoc ;)

lyrixx · 2018-10-02T14:56:57Z

config/services.yaml

@@ -37,6 +37,9 @@ services:
                - '@JoliCode\SecretSanta\Application\SlackApplication'
                - '@JoliCode\SecretSanta\Application\DiscordApplication'

+    Jolicode\SecretSanta\Statistics:
+            public: true


Are you sure it needs to be public ?

Are you sure you need to declare it ? (because there is autodiscovery in this project)

lyrixx · 2018-10-02T14:58:28Z

src/Statistic.php

+
+use Predis\Client;
+
+class Statistic


This name does not really reflect the intent.
I would name it StatisticCollector

this is done

pyrech

Top 👍

lyrixx · 2018-10-04T08:41:17Z

src/Controller/SantaController.php

@@ -31,13 +32,15 @@ class SantaController extends AbstractController
    private $twig;
    private $logger;
    private $applications;
+    private $statistic;


statisticCollector (Consistency is really important - everywhere, everytime)

lyrixx · 2018-10-04T08:44:06Z

src/StatisticCollector.php

+        $currentMonth = date('m');
+
+        //If the key does not exist, it is set to 0 before performing the operation
+        $this->client->hincrby('date:' . $currentYear . '-' . $currentMonth, 'usageCount', 1);


"date: $currentYear - $currentMonth" is more readable than concatenation (same on next line)

I made the change but I need that there is no space so it looks like "date:$currentYear-$currentMonth"

pyrech · 2018-10-05T09:47:55Z

src/Controller/ContentController.php

+    {
+        $statisticValues = $this->statisticCollector->getCounterValues();
+
+//        dump($statisticValues);


dump to remove 🙂

pyrech · 2018-10-05T09:48:39Z

src/Controller/SantaController.php

@@ -50,6 +55,8 @@ public function run(MessageDispatcher $messageDispatcher, Rudolph $rudolph, Requ

        $allUsers = $application->getUsers();

+        dump($clientApplication);


pyrech · 2018-10-05T09:48:57Z

src/Controller/SantaController.php

    }

    public function run(MessageDispatcher $messageDispatcher, Rudolph $rudolph, Request $request, string $application): Response
    {
+        $clientApplication = $application;


why renaming this var?

I delete the $clientApplication variable and I use $application->getCode instead

pyrech · 2018-10-05T09:49:56Z

src/StatisticCollector.php

+        $currentMonth = date('m');
+
+        //If the key does not exist, it is set to 0 before performing the operation
+        $this->client->hincrby('date:$currentYear-$currentMonth', 'usageCount', 1);


doest it really work with single quote?

my mistake, It's done now

pyrech · 2018-10-05T09:50:28Z

src/StatisticCollector.php

+
+    public function getCounterValues()
+    {
+        $counterByDate = $this->getCounter();


return the counter without storing into a temp variable

lyrixx · 2018-10-05T10:24:58Z

templates/content/stats.html.twig

-                    <div class="graph-noData">Sorry, there is no statistic for this section 🙈</div>
-                {% endif %}
+            {% if statisticValues.hashesMonth is not empty %}
+            {% set values = statisticValues.hashesMonth %}


indentation issue

lyrixx · 2018-10-05T10:25:32Z

templates/content/stats.html.twig

-                {% else  %}
-                    <div class="graph-noData">Sorry, there is no statistic for this section 🙈</div>
-                {% endif %}
+            {% if statisticValues.hashesMonth is not empty %}


{% if statisticValues.hashesMonth %} is enought

lyrixx · 2018-10-05T10:26:19Z

src/Controller/ContentController.php

        $content = $this->twig->render('content/stats.html.twig', [
-            'statisticValues' => $statisticValues,
+            'statisticValues' => $this->statisticCollector->getCounters(),


naming matching: values VS counter
(consistancy)

…created)

pyrech · 2018-10-05T12:02:51Z

src/StatisticCollector.php

+        $this->client = $client;
+    }
+
+    public function incrementUsageCount($clientApplication)


Could you add a string typehint and name the parameter application or applicationCode to be accurate?

done with applicationCode name

pyrech · 2018-10-05T12:04:48Z

src/StatisticCollector.php

+        $megaHashes = [];
+
+        $allHashes = [];
+        $allHashes['hashesTotal'] = $this->client->keys('date:total*');


I would simply name the keys total, year, month

I made it, it's clearer

pyrech · 2018-10-05T12:10:02Z

templates/content/stats.html.twig

+            {% if statisticsCounter.hashesYear %}
+                {% set values = statisticsCounter.hashesYear %}
+                {% for value, key in values %}
+                    {% if value matches '/date:\\d+$/'  %}


why do we need this condition?

This serves to check if there is any stats.
If not it shows some message

You can remove the condition.

…er 1px

lyrixx · 2018-10-05T14:39:15Z

templates/content/stats.html.twig

+        <div class="chart chart-month">
+            {% if statisticsCounter.month %}
+                {% set values = statisticsCounter.month %}
+                {% for value, key in values %}


the naming seeming really odd to me. the regular syntaxt of for is for key, value in values. Why are you using the opposite ?

BTW, values is a really bad name. It does not mean anything.

BTW I don't think you need an additional variable

My bad, I made a mistake.
I change the name of key, value and values

lyrixx · 2018-10-05T14:42:42Z

src/StatisticCollector.php

+        $this->client->hincrby("date:total-$ApplicationCode", 'usageCount', 1);
+    }
+
+    public function getCounters()


please add return typehint

lyrixx · 2018-10-05T14:44:50Z

templates/content/stats.html.twig

+        </div>
+    </div>
+
+{% endblock content %}


missing return line (you can configure your IDE to automatically fix that)

pyrech previously requested changes Oct 2, 2018

View reviewed changes

welcoMattic previously requested changes Oct 2, 2018

View reviewed changes

lyrixx reviewed Oct 2, 2018

View reviewed changes

alexzerah added 2 commits October 3, 2018 14:29

Add statistic service and increment usage count

d74d8b9

Fix code review and add counter by month and year

1e75054

alexzerah force-pushed the feature/11 branch from c110a6d to 1e75054 Compare October 3, 2018 12:30

pyrech approved these changes Oct 3, 2018

View reviewed changes

alexzerah requested review from pyrech, welcoMattic and lyrixx October 3, 2018 12:33

lyrixx reviewed Oct 4, 2018

View reviewed changes

bug fixes and add stats page with css design

9b8b0a5

pyrech requested changes Oct 5, 2018

View reviewed changes

code review

2691ba6

lyrixx reviewed Oct 5, 2018

View reviewed changes

alexzerah added 2 commits October 5, 2018 12:28

better acces to $application name (no more $appkicationName variable …

5299b34

…created)

clean stats.html.twig and take off useless code

fa4bb54

pyrech reviewed Oct 5, 2018

View reviewed changes

change variable name, add typehint and make min-height for column und…

ee82189

…er 1px

damienalexandre approved these changes Oct 5, 2018

View reviewed changes

remove useless if

0afd570

lyrixx reviewed Oct 5, 2018

View reviewed changes

alexzerah added 2 commits October 5, 2018 18:22

modificate getDateAndCounters function to be more readable

0c5352b

bug and design details fixes

4e749ed

damienalexandre merged commit ffc7b38 into master Oct 5, 2018

damienalexandre deleted the feature/11 branch October 5, 2018 16:35

		@@ -28,6 +28,7 @@ Then launch this command:

		The application should now be running on http://127.0.0.1:8000.

		Tests are made with PHPUnit

		@@ -50,6 +55,8 @@ public function run(MessageDispatcher $messageDispatcher, Rudolph $rudolph, Requ

		$allUsers = $application->getUsers();

		dump($clientApplication);

Add statistic service and increment usage count #75

Add statistic service and increment usage count #75

Conversation

alexzerah commented Oct 2, 2018 • edited

pyrech Oct 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyrech Oct 2, 2018 • edited

Choose a reason for hiding this comment

pyrech Oct 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyrech left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyrech Oct 5, 2018 • edited

Choose a reason for hiding this comment

alexzerah Oct 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzerah commented Oct 2, 2018 •

edited

pyrech Oct 2, 2018 •

edited

pyrech Oct 2, 2018 •

edited

pyrech Oct 2, 2018 •

edited

pyrech left a comment •

edited

pyrech Oct 5, 2018 •

edited

alexzerah Oct 5, 2018 •

edited