From 201c0f5538e93908c813d22fcc618c9443b1384b Mon Sep 17 00:00:00 2001
From: Max Beckenbach <max@bebamail.de>
Date: Mon, 21 Mar 2022 09:49:47 +0100
Subject: [PATCH] high level code review.

---
 gulpfile.js             |  6 ++++++
 src/App.vue             |  4 ++++
 src/main.js             |  5 +++++
 src/router.js           |  3 +++
 src/store.js            |  3 +++
 src/views/AddUser.vue   | 10 ++++++++++
 src/views/AddWork.vue   | 12 ++++++++++++
 src/views/ListUsers.vue |  4 ++++
 src/views/ListWork.vue  |  7 +++++++
 src/views/Login.vue     |  4 ++++
 10 files changed, 58 insertions(+)

diff --git a/gulpfile.js b/gulpfile.js
index 281d350..0039ca3 100644
--- a/gulpfile.js
+++ b/gulpfile.js
@@ -7,6 +7,12 @@
 /* jshint node: true */
 'use strict';
 
+/**
+ * TODO: You should use JS doc comments for all the methods and consts here.
+ * As this file is plain JS, it should also document the params for methods
+ * This will allow much better coding assistence and automated docs generation
+ */
+
 // Date Options
 // Manual: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleDateString
 const options = {
diff --git a/src/App.vue b/src/App.vue
index f8b6b76..d4ac6a7 100644
--- a/src/App.vue
+++ b/src/App.vue
@@ -6,6 +6,10 @@
 // Manual: https://parttimedeveloper.medium.com/how-to-create-a-navigation-bar-in-vue-js-8a70e7f29f80
 // Manual: https://v3.vuejs.org/guide/single-file-component.html#introduction
 
+/**
+ * TODO: This app uses TS. Do you really need to use plain JS in component code? Looks wierd. 
+ */
+
 import Navigation from "./components/Navigation";
 
 export default {
diff --git a/src/main.js b/src/main.js
index 1bfbdee..04cbf54 100644
--- a/src/main.js
+++ b/src/main.js
@@ -7,6 +7,11 @@ import App from "./App.vue";
 import router from "./router";
 import store from "./store.js";
 
+/**
+ * TODO: Isn't there some lazy loading i18n for view? The implementation looks a lot like i18next. 
+ * It doesnt make a lot of sense to hard embed translations. 
+ */
+
 // 1. Ready translated locale messages
 // The structure of the locale message is the hierarchical object structure with each locale as the top property
 const messages = {
diff --git a/src/router.js b/src/router.js
index 134aa0b..d24c446 100644
--- a/src/router.js
+++ b/src/router.js
@@ -9,6 +9,9 @@ import Login from "@/views/Login.vue";
 
 import store from './store.js'
 
+// TODO: JS Docs
+// TODO: If this is a TS app, than why is this plain js?
+
 // Manual: https://www.vuemastery.com/blog/vue-router-a-tutorial-for-vue-3/
 
 const routes = [
diff --git a/src/store.js b/src/store.js
index 02f91da..b4e161a 100644
--- a/src/store.js
+++ b/src/store.js
@@ -5,6 +5,9 @@ import Cookies from 'js-cookie';
 import SecureLS from "secure-ls";
 const ls = new SecureLS({ isCompression: false });
 
+// TODO: JS Docs
+// TODO: If this is a TS app, than why is this plain js?
+
 const store = createStore({
     state() {
       return {
diff --git a/src/views/AddUser.vue b/src/views/AddUser.vue
index d3e65d5..102fc2c 100644
--- a/src/views/AddUser.vue
+++ b/src/views/AddUser.vue
@@ -187,6 +187,11 @@ export default {
     onSubmit() {
 
       // checkbox return value = "true", so replace it with boolean value
+      /**
+       * TODO: This looks like a workarround for bad type safety.
+       * First of all the check can be for truethy. if (this.employee.rights)
+       * Second why do you abuse numeric for boolean?
+       */
       if (this.employee.rights === true || this.employee.rights == 1){
         this.employee.rights = 1;
       }else{
@@ -198,6 +203,8 @@ export default {
       console.log("Rights  : " + this.employee.rights);
      
       try {
+        // TODO: Always use trippe = syntax. This here is not type safe!
+        // TODO: Not sure why you need to check for empty strings here. Can't view do form validation? There is some isRequired method below. Can't this be checked?
         if (this.employee.username != "" && this.employee.password != "") {
           axios
             .post("php/adduser.php", 
@@ -218,6 +225,7 @@ export default {
                 console.log("Data recieved");
               }
 
+              // TODO: I have the strong feeling that response parsing doesnt belong here
               //if (response.data.status === 0) {
               if (response.data[0].msg){
                 this.msg = response.data[0].msg;
@@ -232,6 +240,7 @@ export default {
               }*/
             })
             .catch(function(error) {
+              // TODO: You should check if axios can do global error handling
               if (error.response) {
                 // Request made and server responded
                 console.log(error.response.data);
@@ -246,6 +255,7 @@ export default {
               }
             });
         } else {
+          //TODO: An alert? Honestly? :-D
           alert("Please enter username");
         }
 
diff --git a/src/views/AddWork.vue b/src/views/AddWork.vue
index f4fee33..4336b4e 100644
--- a/src/views/AddWork.vue
+++ b/src/views/AddWork.vue
@@ -129,6 +129,7 @@ export default {
       },
       msg: "",
       msg_type: "info",
+      // TODO: Should use speaking names
       m: new Date().getMonth() + 1,
       employee: {
         worktime: undefined,
@@ -159,9 +160,12 @@ export default {
     isButtonDisabled() {
       // Stackoverflow: https://stackoverflow.com/a/67073622/14331711
       // If isButtonDisabled has the value of null, undefined, or false, the disabled attribute will not even be included in the rendered <button> element.
+      // TODO: Can be simplified with &&
+      // TODO: Architectural this should be a boolean
       return this.employee.worktime >= 0 ? undefined : "disabled";
     },
     ruleWorkTime(value) {
+      // TODO: What? Boolean or output string?
       // min 0
       // max 192
       return value >= 0 && value <= 192
@@ -179,9 +183,11 @@ export default {
       return 'This is required';
     },*/
     getYearMonthFirst() {
+      // TODO: Cold feeling running down my neck. Check out momentjs
       return new Date().getFullYear() + "-01";
     },
     getYearMonthNow() {
+      // TODO: Cold feeling running down my neck. Check out momentjs
       const month = new Date().getMonth() + 1;
       //return new Date().getFullYear() + "-" + new Date().getMonth();
       return (
@@ -189,6 +195,7 @@ export default {
       );
     },
     toFormData() {
+      // TODO: WHY???? Your backend is PHP, it should handle json in 2022
       let formData = new FormData();
       for (let key in obj) {
         formData.append(key, obj[key]);
@@ -201,6 +208,8 @@ export default {
       console.log("Date    : " + this.employee.date);
 
       try {
+        //TODO: Do a type safe check. 
+        //TODO: Why can a date be a string?
         if (this.myUserId != "" && this.employee.date != "") {
           //&& this.employee.worktime != "") {
           axios
@@ -227,6 +236,7 @@ export default {
                 if (response.data.worktime) {
                   //alert("Worktime saved already, you can overrite");
                   this.employee.worktime = response.data.worktime;
+                  //TODO: Not sure if this is vue specific. But such things dont belong to logic layer.
                   this.button.text = "Save";
                 }
               }
@@ -271,6 +281,7 @@ export default {
 
       try {
         if (
+          // TODO: TYPE SAFE CHECKS!!!! TRIPPLE =
           this.myUserId != "" &&
           this.employee.date != "" &&
           this.employee.worktime != ""
@@ -309,6 +320,7 @@ export default {
                 alert("User does not exist");
               }*/
             })
+            // TODO: Can't you use ES6 here? () => {}
             .catch(function(error) {
               if (error.response) {
                 // Request made and server responded
diff --git a/src/views/ListUsers.vue b/src/views/ListUsers.vue
index 84fcbac..7aa884c 100644
--- a/src/views/ListUsers.vue
+++ b/src/views/ListUsers.vue
@@ -150,6 +150,7 @@ export default {
         this.getEmployees();
     },
     methods: {
+        // TODO: You should not shortcut method names. deleteEmployee
         delEmployee: function (user_id) {
 
           try {
@@ -330,6 +331,7 @@ export default {
             });*/
         },
         toFormData() {
+          //TODO: See other comment on formdata
             let formData = new FormData();
             for (let key in obj) {
                 formData.append(key, obj[key]);
@@ -358,6 +360,8 @@ export default {
             return null;
         },*/
         recordByID() {
+          // TODO: Can be simplified with a truethy check
+          // TODO: User IDs should NEVER be numeric for security reasons. Use UUIDs instead.
             if (this.user_id > 0) {
                 axios
                     .get("php/get.php", {
diff --git a/src/views/ListWork.vue b/src/views/ListWork.vue
index ff153cd..4ce78a4 100644
--- a/src/views/ListWork.vue
+++ b/src/views/ListWork.vue
@@ -129,6 +129,7 @@
 
 <script>
 import qs from "qs";
+//TODO: Unused import causes build optimization issues
 import router from "@/router";
 import { computed } from 'vue'
 import * as vuex from 'vuex'
@@ -145,6 +146,7 @@ export default {
       }
     },
     data() {
+      // TODO: Why don't you treat undefined as undefined? Does vue really need empty strings??
       return {
         users: "",
         works: {},
@@ -169,6 +171,7 @@ export default {
     },
     computed: {
       filterWorksByUsers: function(){
+        // TODO: Type safe checks... Truethy checks
         if (this.myUserRights == 1 && this.users != "") {
           return this.works.filter(work => !work.username.indexOf(this.users));
           //return this.works.filter(work => !work.username.indexOf("julian.kasimir"));
@@ -179,6 +182,8 @@ export default {
       },
     },
     mounted() {
+      // TODO: Can be truethy check. Or is this an enum? If it is an enum, why not using string identifiers?
+      // TODO: A great example why you should use typescript by the way. 
       if (this.myUserRights == 1) this.getEmployees();
       this.getworks();
     },
@@ -191,6 +196,7 @@ export default {
           console.log("Date    : " + this.employee.date);*/
 
           try {
+            // TODO: Type safety
             if (this.myUserId != "" && this.myUserRights >= 0) {
             axios
               .post("php/listusers.php",
@@ -447,6 +453,7 @@ export default {
             return formData;
         },
         sendIdentity() {
+          // TODO: Please, please, please, JSON...
             let newworkForm = this.toFormData(this.newwork);
             console.log(newworkForm);
 
diff --git a/src/views/Login.vue b/src/views/Login.vue
index 6716cda..6301ffd 100644
--- a/src/views/Login.vue
+++ b/src/views/Login.vue
@@ -207,6 +207,7 @@ export default {
   },
   methods: {
     checkLogout() {
+      //TODO: You are using some kind of state managent as i understand. Such things dont belong in presentation layer!
       if ( this.$route.params.logout == ':logout' ){
         // logout
         // alert("logout");
@@ -252,6 +253,7 @@ export default {
     login(e) {
       // Manual: https://www.digitalocean.com/community/tutorials/how-to-set-up-vue-js-authentication-and-route-handling-using-vue-router
       e.preventDefault(); // or form @submit.prevent="handleSubmit">
+      // TODO: Form does not validate?
       if (this.password.length >= this.passwordminlength) {
         //this.$http.post('auth.php', {
         axios
@@ -274,6 +276,8 @@ export default {
           .then((response) => {
             console.log(response.data);
             console.log(response.data[0].status);
+
+            // TODO: WTF does this do?
             if (response.data[0].status === 1) {
                 
               if (response.data[0].msg){