3
\$\begingroup\$

Here is my parent component: App.vue. I was wondering what improvements I could make to the code here.

Application Description

This application was built off the Vue.js framework and utilizes a Laravel REST API I created to fetch custom data. It stores and tracks your ‘health score’, which is determined by the answers you submit in the test. Each answer has a value. So, the unhealthiest answer has a value of 0, the second-least healthiest 1, second-most healthiest 2, and the healthiest answer 3. The values of the answers you submit are aggregated throughout the test via local storage. After the final question has been answered, your total health score is returned. Your final score will determine the message you receive.

<template>
  <div id="app">
    <div id="nav">
      <!--once the testData array has received its first element, then render the test data
      (this prevents error in dev tools)-->
      <router-view
        v-if="testData.length" 
        :currentTestData = testData[testDataIndex]
        :testDataIndex = testDataIndex
        :nextAndResetIndices="nextAndResetIndices"
        :selectAnswer="selectAnswer"
        :selectedIndex="selectedIndex"
        :submitAnswer="submitAnswer"
        :submittedIndex="submittedIndex"
        :healthScore="healthScore"
        :setStorage="setStorage"
        :getStorage="getStorage"
        :resetStorage="resetStorage"
        :message="message"
        :returnMessage="returnMessage"

      />
    </div>
  </div>
</template>

<script>
export default {
  data() {
    return {
      testData: [],
      testDataIndex: 0,
      selectedIndex: null,
      submittedIndex: null,
      healthScore: 0,
      message: ''
    }
  },  
  methods: {
    nextAndResetIndices() {
      this.testDataIndex++
      this.selectedIndex = null
      this.submittedIndex = null
      // takes user to health score page
      if (this.testDataIndex > 6) {
        window.location.href = '/hs'
      }
    },
    selectAnswer(index) {
      this.selectedIndex = index
      console.log(index)
    },
    submitAnswer(selectedIndex) {
      this.submittedIndex = this.selectedIndex
      console.log(this.submittedIndex)
      // health score increments by the submitted index
      this.healthScore += this.submittedIndex
    },
    resetStorage() {
      localStorage.setItem('SavedHealthScore', 0)
    }
  },
  computed: {
    setStorage() {
      // prevents the stored health score from mimicing the original and reverting back to 0
      if (this.healthScore !== 0) {
        localStorage.setItem('SavedHealthScore', this.healthScore)
      }
    },
    getStorage() {
      this.healthScore = parseInt(localStorage.getItem('SavedHealthScore'))
    },
    toZero() {
      // returns the health score back to 0
      this.healthScore = 0
    },
    returnMessage() {
      if (this.healthScore < 8)
        this.message = 'Get to the hospital now! 🚑🏥😷'
      else if (this.healthScore >= 8 && this.healthScore < 15)
        this.message = "Not bad. You'll probably live! 🤷‍♂️"
      else 
        this.message = "You're very healthy. Congrats! 👨‍⚕️🥗"
    }
  },
  mounted() {
    fetch('http://127.0.0.1:8000/api/testdata', {
    method: 'get'
  })
    .then((response) => {
    return response.json()
  })
    // 'jsonData' refers to the json parsed response
    .then((jsonData) => {
      this.testData = jsonData.data
  })
}}
</script>
\$\endgroup\$
2
  • 1
    \$\begingroup\$ @pacmaninbw I've included a more specific description. I didn't want to phrase the question that way, I wanted to include the word 'improve' but the algorithms wouldn't let me. I hope it's a little clearer now. I'm mainly concerned about which functions should be under methods and which functions should be under computed. \$\endgroup\$ Sep 21 '19 at 13:11
  • 2
    \$\begingroup\$ Everyone wants their code to be better, that's why you don't really have to include it, it's implied for this page. However, you could make it easier for reviewers by perhaps making the example fully runnable, or maybe include a link to the full, runnable version. As it is, there's a lot missing (where and how are questions answered, etc.) and that makes it hard or impossible to suggest meaningful improvements. \$\endgroup\$
    – ferada
    Sep 21 '19 at 14:25
2
\$\begingroup\$

If you wish to keep your implementation scheme, I only have one large critique:

Names are important

Your naming scheme should allow a 3rd party to understand your code from a cursory glance. You have a lot of sneaky implementations tucked into your functions that aren't expressed in your naming convention.

All of your operations are stateful, which is technically ok but you use a lot of verbage that makes it sound like they're functional operations with actual return values. All of your functions act on stateful variables. Your naming convention should reflect this through the use of verb phrases, as opposed to nouns.

resetStorage: A good variable name. It is a verb that acts on localStorage

getStorage: Misleading. The traditional usage of the verb get is to retrieve a value or perform a computation (and return it). You are assigning this.healthScore. getStorage doesn't even have a return value. A more apt name would be something more like setHealthScoreFromLocalStorage.

toZero: What exactly is being set to zero? From the rest of your code (looking at resetStorage) I can infer that the value of 0 is the default value. It should be more along the lines of resetHealthScore or clearHealthScore.

returnMessage: It is updating this.message. So something more like updateHealthMessage?

\$\endgroup\$
4
  • \$\begingroup\$ I've renamed 'getStorage' to 'setHealthScoreFromLocalStorage' like you suggested. I know what you mean about 'toZero' being misleading, especially when compared to 'resetStorage'. So, I renamed 'toZero' to 'resetHealthScore' and 'resetStorage' to 'resetHealthScoreInLocalStorage'. And yes, I'm gonna change 'returnMessage' to updateHealthScoreMessage'. Thanks for the feedback. \$\endgroup\$ Oct 3 '19 at 6:19
  • \$\begingroup\$ I've just deployed the project and everything works fine. Except, instead of the app returning the health score it returns NaN. But this only happens during the browser's first time. After that, it works normally. Is there anything in my logic to suggest that this would be an issue after deployment, but not locally? \$\endgroup\$ Oct 3 '19 at 19:07
  • 1
    \$\begingroup\$ @HarrisonGreeves The only thing I see that might be an issue is this line: this.healthScore += this.submittedIndex. submittedIndex starts off as null. You might not be seeing the issue in local because your local call is very fast, while in production it is far slower. Try slowing down your network speed (in the dev console) and seeing if you can replicate that behavior \$\endgroup\$
    – Andrew
    Oct 3 '19 at 19:57
  • \$\begingroup\$ Yep, I've slowed down the network in my dev tools to 'slow 3G' and I'm seeing the same issue. May make this a habit from now on. Thanks. I have no idea what submittedIndex could be if not null. It can't start off as 0 because that would be the index of the first answer for every question. \$\endgroup\$ Oct 4 '19 at 5:28
1
\$\begingroup\$

General feedback

This code looks decent, though as Andrew's answer points out many of the names are misleading. My biggest complaint is that semi-colons aren't used to terminate lines. While they are only required after a handful of statements, it could lead to errors if somehow whitespace got removed. It is a good habit to default to terminating lines with them.

Suggestions

method nextAndResetIndices()

There is a conditional block at the end of the method:

if (this.testDataIndex > 6) {
  window.location.href = '/hs'
}

Perhaps that check should be moved to the beginning of the method, otherwise the line above it (except for the line to increment this.testDataIndex) become useless. The line to increment this.testDataIndex could be combined into the conditional check if it is converted to a prefix increment operation:

if (++this.testDataIndex > 6) {
  window.location.href = '/hs'
}

Arrow function Simplification

The promise callbacks in mounted() can be simplified because arrow functions with a single statement don't need to have braces. Bear in mind that this means that the return value of the statement is always returned but for this purpose that wouldn't lead to any adverse affects for the last callback. Also, parentheses are not required around a single parameter.

This block:

fetch('http://127.0.0.1:8000/api/testdata', {
method: 'get'
})
.then((response) => {
return response.json()
})
// 'jsonData' refers to the json parsed response
.then((jsonData) => {
  this.testData = jsonData.data
})

Can be simplified to the following:

fetch('http://127.0.0.1:8000/api/testdata', {
  method: 'get'
})
.then(response => response.json())
// 'jsonData' refers to the json parsed response
.then(jsonData => this.testData = jsonData.data);
\$\endgroup\$
1
  • \$\begingroup\$ I've implemented all the changes you suggested. Thanks \$\endgroup\$ Oct 3 '19 at 13:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.