@Ahmadhassan0Submitted 12 months ago
You can look at my code and If you have any type of feedback on how can I improve my code quality I'll be very happy to look at your feedback😊
JJacobPR
@JJacobPRAll comments
- @JJacobPRPosted 12 months ago
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 helpful1 - @cancomertpaySubmitted 12 months ago
Greetings everyone! Please share your feedback and suggestions for more optimized solutions. Thank you!
@JJacobPRPosted 12 months agoHey,
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 helpful1 - @Maksio07Submitted 12 months ago
Hi everyone, there is my solution for Advice Generator App. Thanks for this challenge I learnt how to use API in my projects. I'll be happy to hear your advices and comments for my solution.
@JJacobPRPosted 12 months agoHey 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 helpful0 - @Fejiro001Submitted 12 months ago
Hi this is Fejiro, and this is my solution to the Advice Generator
- Got it as close to the design
- Made sure it was responsive
- Achieved perfect 100 score on lighthouse
Any suggestions on how I can improve are welcomed
@JJacobPRPosted 12 months agoHey,
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 😀
1 - @opolis8Submitted about 1 year ago
My code is not perfect, I had a hard time how to compute the age if you had your birthday this year. but on the brighter side, i learned so much on this project as I practiced how to call a function inside a function and use it many times.
Any comments and suggestion for me to improve my coding is much appreciated
@JJacobPRPosted about 1 year agoHello,
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 :)
0 - @Julie-GzSubmitted about 1 year ago
Hi Frontend mentor community. This was a wonderful challenge. I'm not sure if the app is 100% accurate, so I would be grateful if someone tested it and gave me feedback. Any other feedback is also appreciated.
Thank you :)
@JJacobPRPosted about 1 year agoHey,
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 :)
1 - @aboobaqrSubmitted about 1 year ago
If you have the time to check please let me know where and how I can improve, I wrote over 230 lines of JavaScript. 😩 I really think it's too much for this project.
@JJacobPRPosted about 1 year agoHey,
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 helpful0 - @a-Rider-mSubmitted about 1 year ago
"This is the solution to the 'age calculator' challenge, it's practically finished, although I have the following issue.
"Depending on the months, an error is generated if the day is not valid, but when entering the day first and then specifying the month, the validation is not performed, which results in February 31st being considered a valid day."
@JJacobPRPosted about 1 year agoHola 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 helpful0 - @ParvizAzerogluSubmitted about 1 year ago
Finding out how many days February has proved to be a bit challenging for me, but I never really needed to know anyway, XD! I just use 31 days as a reference for all the months
@JJacobPRPosted about 1 year agoHey, 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 helpful0
