JJacobPR
@JJacobPRAll comments
- @Ahmadhassan0@JJacobPR
Hey!
This is a very short app so it is difficult to spot any major problems with your code but what I would definitely suggest as it is a standard, is to remove any logs and stuff for development before let's call it pushing to production :)
Marked as helpful - @cancomertpay@JJacobPR
Hey,
Nice solution you have here but I found a problem. The leap year is not considered and you can check 29/02 for each year.
My suggestion is that you can create Date object based on year (it will know if it is leap year) and validate properly!
Good Luck!
Marked as helpful - @Maksio07@JJacobPR
Hey Maksym,
Good job implementing this task! I have two things you might consider improving:
- The UI when user first enters the app can be more intuitive like "Roll the dice for advice" so user knows what to do
- As you open your app you may see that you will get the same advice. It is the fault of API but as you are learning you can try to work it out to generate it randomly from the frontend perspective. Hint: Check the documentation for the route that requires advice id and handle the randomness on frontend.
Good Luck!
Marked as helpful - @Fejiro001@JJacobPR
Hey,
I love your dice animation. What I would improve is the random data generation. As this API tends to get a little bit sloppy, you can use the documentation to find route for a specific advice and generate the ids yourself in frontend to ensure that user would get random advice. I know that normally it would be backend's fault but here we are just practicing, aren't we 😀
- @opolis8@JJacobPR
Hello,
As I've check that you eventually didn't implement the "this year birthday case", let me give you a hint.
If I were you, I would add additional condition that checks if input year is current year and if it is, additional checks for month and day, since then the provided day cannot be bigger than current day and the same with month.
If it was not the problem and you had had this idea but simply couldn't implement it, just ask in the reply and I'll send you a snippet. You can also just check in my code of course :)
- @Julie-Gz@JJacobPR
Hey,
I did a little bit of testing as you suggested and things that (in my view) should be enhanced:
- Input that is not a number doesn't give error
- After providing correct day and month, having error before, the error state remains
- The date is calculated partially even if some parts of input data are invalid
- And probably the sneakiest - you should add special validation for current year case, as for example although 15.12.2023 is a valid date, for now it is in the future
Hope, this feedback is what you wanted :) Should you have more questions, just ask :)
- @aboobaqr@JJacobPR
Hey,
as you are probably beginner, don't worry that your code is that long, you will improve as you practice. But here are 2 major tips for your programming habits not only js but overall.
- Separation of concerns Your click function is too long. All of your comment should be functions. It will make code more readable and easier to change
- Hard coded values If you can, don't use hard coded values. In this code, firstly it is longer then needed and it doesn't cover case when February has 29 days.
if (monthInput.value == 2 && dayInput.value > 28)
When you can, try to use other methods. Here recommended are inbuilt Date functions that you should explore. This is my example of validating day by inbuilt functions:
const dayValidator = () => { //Checking if day is okay in current year case if (year === currentYear && month === new Date().getMonth() + 1 && day > new Date().getDate()) return false; //Checking if day is okay based on particular month length if (day >= 1 && day <= new Date(year, month, 0).getDate()) return true; else return false; };
Marked as helpful - @a-Rider-m@JJacobPR
Hola amigo,
I think that I found the problem with your code :) Let's take a look at this snippet
switch(e.target.id) { case 'day': validateField('day', 'label-day','error-text-day' , e.target, maxDays, 2); break; case 'month': validateField('month', 'label-month','error-text-month', e.target, 12, 2); break; case 'year': validateField('year', 'label-year','error-text-year', e.target, 2024, 4); break; }
Here you are checking field based on event.target.id. But only one field each time, only on day, month or year. So when you put day 31, it checks on your default case:
} else { maxDays = 31; }
But when you put in the month, since you use switch case, it doesn't recheck the day, only the month. The same goes for year. The easiest solution would be to check all fields each time. It is a small app so it won't be a problem.
Another thing, I would consider changing, is the way you check day amount per month. Remember the February has 28 or 29 days. I know it is a bit more of a challenge but that's what these are for.
Hope that I helped and good luck!
Marked as helpful - @ParvizAzeroglu@JJacobPR
Hey, maybe this snippet will help you. It checks if day exists by creating Date object based on year and month input and extracting day count of needed month.
if (props.day >= 1 && props.day <= new Date(props.year, props.month, 0).getDate()) return true; else return false;
Marked as helpful