Skip to content

Update app.js#22

Closed
jasontwuk wants to merge 1 commit intojohn-smilga:masterfrom
jasontwuk:master
Closed

Update app.js#22
jasontwuk wants to merge 1 commit intojohn-smilga:masterfrom
jasontwuk:master

Conversation

@jasontwuk
Copy link
Copy Markdown

At the moment, const hours and const minutes haven't been reformat to the desired format.
(For example, if the value is 0, it will show '0' instead of '00')
If we move format() out of getRemaindingTime(), it can be used on const hours and const minutes to get the job done.

@aaramini
Copy link
Copy Markdown

aaramini commented Apr 16, 2022

I found a similar presentation issue with the giveaway.textContent format. If minutes value is 0, it will show '0' instead of '00'.
Also, there is an issue that it always shows "am" as coded.

I propose that the issues I pointed out be fixed and then 24 hour time format (without am/pm) is used or add code to convert the default 24 hr time format to 12 hour time with checks do display 12:00AM if Hours == 0. I am relying on the constant variables Hours and Minutes to do this in my fix proposal.

Your pull request would invalidate my code. If format() is moved out of getRemaindingTime(), I think it would be better to pass Hours and Minutes to it via function call of format(), since it is a function, rather than alter the current constant variables by sticking format() in there. Assign the format calls to new variables (if necessary) rather than altering the constant variables my fixes rely on for a similar issue elsewhere in the code.

That's one of the reasons for declaring something as a constant. So that the constant is immutable, but you can make a copy of it and change and work with the copy. In other words, assign the constant to a variable and alter that variable for the purposes of your piece of the puzzle, and therefore not break other parts of code where others might be relying on the constant to be a constant value in their piece of the puzzle. If constants are frequently needing to be changed at the assignment level, they should not have been declared as a constant to begin with because they effectively aren't "constant" so a var should have been used instead.

In very small projects with a small handful of maintainers and excellent communication, or your own code, there is no real impact. But could you imagine a very large project where there are teams of people working on different parts of the code, and the constants are always being altered as in your PR? It would be a continual, and "constant" mess of bugs... (pun intended) and break other parts of the code people are working on.

ex:

constant hours = futureDate.getMinutes();

a = format(hours); // the return value is now in 'a', so use 'a' instead of 'hours'.

b = format(minutes); // the return value is now in 'b', so use 'b' instead of 'hours'.

see #61 for clarification on what bugs I'm referring to in project 12 - Countdown - final/app.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants