Kevin Powell - Community•4mo ago

Is it legible?

hello, everyone i am working in a simple age calculator (i havent finished) and i want some advices about the javascript part, am i structuring it correctly? can you understand it? or what resources do you recommend me to improve the quality of the code. All advices al helpfull

18 Replies

GegaOP•4mo ago

const $form = document.querySelector("form")
const $inputDay = document.querySelector(".day")
const $inputMonth = document.querySelector(".month")
const $inputYear = document.querySelector(".year")
const $inputFields = document.querySelectorAll("input")
const $resultYear = document.querySelector(".result-year")
const $resultMonth = document.querySelector(".result-month")
const $resultDay = document.querySelector(".result-day")

const date = new Date()
const currentYear = date.getFullYear()
const currentMonth = date.getMonth() + 1
const currentDay = date.getDate()

let inputYear = 0
let inputMonth = 0
let inputDay = 0

function maskInputRange(element, startValue, endValue) {
    IMask(element, {
        mask: IMask.MaskedRange,
        from: startValue,
        to: endValue
    })
}

function getDaysOfMonth(month, year) {
    return new Date(year, month, 0).getDate()
}

function calculateChronologicalAge(day, month, year) {
    let resultYear = currentYear - year
    let resultMonth = currentMonth - month
    let resultDay = currentDay - day

    if (currentMonth < month || (currentMonth === month && currentDay < day)) {
        resultYear--
    }

    if (resultMonth <= 0) {
        resultMonth += 12
    }

    if (resultDay < 0) {
        resultDay += getDaysOfMonth(currentMonth, currentYear)
        resultMonth--
    }

    $resultYear.textContent = resultYear
    $resultMonth.textContent = resultMonth
    $resultDay.textContent = resultDay
}

function validateInputs() {
    let hasError = false
    $inputFields.forEach((input) => {
        if (input.value.trim() === "" || parseInt(input.value) < 0) {
            hasError = true
            input.parentElement.classList.add("error")
        } else {
            input.parentElement.classList.remove("error")
        }
    })
    return hasError
}

const $form = document.querySelector("form")
const $inputDay = document.querySelector(".day")
const $inputMonth = document.querySelector(".month")
const $inputYear = document.querySelector(".year")
const $inputFields = document.querySelectorAll("input")
const $resultYear = document.querySelector(".result-year")
const $resultMonth = document.querySelector(".result-month")
const $resultDay = document.querySelector(".result-day")

const date = new Date()
const currentYear = date.getFullYear()
const currentMonth = date.getMonth() + 1
const currentDay = date.getDate()

let inputYear = 0
let inputMonth = 0
let inputDay = 0

function maskInputRange(element, startValue, endValue) {
    IMask(element, {
        mask: IMask.MaskedRange,
        from: startValue,
        to: endValue
    })
}

function getDaysOfMonth(month, year) {
    return new Date(year, month, 0).getDate()
}

function calculateChronologicalAge(day, month, year) {
    let resultYear = currentYear - year
    let resultMonth = currentMonth - month
    let resultDay = currentDay - day

    if (currentMonth < month || (currentMonth === month && currentDay < day)) {
        resultYear--
    }

    if (resultMonth <= 0) {
        resultMonth += 12
    }

    if (resultDay < 0) {
        resultDay += getDaysOfMonth(currentMonth, currentYear)
        resultMonth--
    }

    $resultYear.textContent = resultYear
    $resultMonth.textContent = resultMonth
    $resultDay.textContent = resultDay
}

function validateInputs() {
    let hasError = false
    $inputFields.forEach((input) => {
        if (input.value.trim() === "" || parseInt(input.value) < 0) {
            hasError = true
            input.parentElement.classList.add("error")
        } else {
            input.parentElement.classList.remove("error")
        }
    })
    return hasError
}

maskInputRange($inputDay, 1, 31)
maskInputRange($inputMonth, 1, 12)
maskInputRange($inputYear, 1500, currentYear)

$inputDay.addEventListener("input", (e) => {
    inputDay = parseInt(e.target.value) || 0
})

$inputMonth.addEventListener("input", (e) => {
    inputMonth = parseInt(e.target.value) || 0
})

$inputYear.addEventListener("input", (e) => {
    inputYear = parseInt(e.target.value) || 0
})

$form.addEventListener("submit", (e) => {
    e.preventDefault()
    if (!validateInputs()) {
        calculateChronologicalAge(inputDay, inputMonth, inputYear)
    }
})

maskInputRange($inputDay, 1, 31)
maskInputRange($inputMonth, 1, 12)
maskInputRange($inputYear, 1500, currentYear)

$inputDay.addEventListener("input", (e) => {
    inputDay = parseInt(e.target.value) || 0
})

$inputMonth.addEventListener("input", (e) => {
    inputMonth = parseInt(e.target.value) || 0
})

$inputYear.addEventListener("input", (e) => {
    inputYear = parseInt(e.target.value) || 0
})

$form.addEventListener("submit", (e) => {
    e.preventDefault()
    if (!validateInputs()) {
        calculateChronologicalAge(inputDay, inputMonth, inputYear)
    }
})

lko•4mo ago

Are you using jquery?

GegaOP•4mo ago

no just javscript

lko•4mo ago

Why did you add $ before the variable name?

GegaOP•4mo ago

i used $ to make a difference between a "dom variables" from logical ones

ἔρως•4mo ago

the $ is usually used for jquery

Joao•4mo ago

I think it's pretty good overall, except for the $ in front. It's not like it's a crime, but you can see that it throws people off, as it's a well established convention already. Instead, use either more descriptive names (not needed in this case in my opinion) or append with an underscore variables that are local in scope. For example resultYear can be used globally as the DOM selector, while _resultYear is the variable that only applies inside the calculateChronologicalAge function. This is a much more common pattern that you can often times see in libraries, where one function, let's call it transform is exposed to the public API, while the actual logic uses _transform internally.

GegaOP•4mo ago

oh i never heard about that, thank you

Joao•4mo ago

I also see a few problems here:

$form.addEventListener("submit", (e) => {
    e.preventDefault()
    if (!validateInputs()) {
        calculateChronologicalAge(inputDay, inputMonth, inputYear)
    }
})

$form.addEventListener("submit", (e) => {
    e.preventDefault()
    if (!validateInputs()) {
        calculateChronologicalAge(inputDay, inputMonth, inputYear)
    }
})

First, the function name is confusing: validateInputs. If I validate something using this function, but true means errors, so I have to negate the return value in my if statement. 🤔 Not good. If it returns false, it should mean that it failed the validation. That's not just my opinion, it's a common pattern that you'll see all around. Secondly, instead of nesting the "happy path" that the function will take when there are no errors, you should return early when you spot them.

$form.addEventListener("submit", (e) => {
    e.preventDefault()

    if (!validateInputs()) {
      throw new Error('validation failed');
    }

    calculateChronologicalAge(inputDay, inputMonth, inputYear)
})

$form.addEventListener("submit", (e) => {
    e.preventDefault()

    if (!validateInputs()) {
      throw new Error('validation failed');
    }

    calculateChronologicalAge(inputDay, inputMonth, inputYear)
})

This way, the function reads from top to bottom as "these are the conditions that need to happen in order for this function to run properly" You can also split this into its own function, where you can have other input parsing and validation. This also avoids the need for global variables, as you can extract the input elements of a form directly from its reference variable. There's a very good example here: https://developer.mozilla.org/en-US/docs/Learn/Forms/Sending_forms_through_JavaScript#associating_a_formdata_object_and_a_form

GegaOP•4mo ago

i will take a look at it thx

ἔρως•4mo ago

also, you don't need to grab all the inputs directly from the dom you can validate by using a FormData, for example also, why do you have 3 inputs for a date, when there's <input type="date">?

GegaOP•4mo ago

i used 3 because they were part of the challenge

Joao•4mo ago

I also prefer regular text inputs for things like dates. Dates can be annoying specially when having to select a year far back in the past. Easier to validate, and restrict input with regular expressions too. I would make sure however that once a field is considered valid, it automatically jumps to the next. Same as it happens when paying using credit cards for example

ἔρως•4mo ago

this is the missing context that's extremely important to always share yeah, but it's hard to check if a year has 366 days with regular expressions i love regular expressions, but don't think that it is the right tool to validate it

Joao•4mo ago

RegExp are not for validation in this case, but to enforce strict input format. 9999 is a valid year format, and the date input would allow it as well. Validation comes afterwards anyway.

ἔρως•4mo ago

that is a lot easier then

Joao•4mo ago

I find that using text inputs is a lot easier in most common use cases just because of this extra step that needs to happen anyway. The only downside is accessibility

ἔρως•4mo ago

it makes sense

Gaming

Programming

Is it legible?