Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solution #55

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Solution #55

wants to merge 3 commits into from

Conversation

trinidaa
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Thank you for your hard work on this task! 🎉 While your solution meets most of the requirements, there are a couple of critical issues that need to be addressed before we can approve the pull request:

  1. Data Type for WarehouseAmount: The WarehouseAmount column is currently defined as VARCHAR(50). This column should ideally be an INT type to accurately represent the quantity of products available in the warehouse. Using an integer type will ensure that the data is stored correctly and can be used for numerical operations.

  2. Data Type for Price: Similarly, the Price column is defined as VARCHAR(50). Prices should be stored as a numeric type, such as DECIMAL, to allow for proper arithmetic operations and maintain data integrity. This change will help in performing calculations like total order value or applying discounts.

Please make these adjustments to ensure the database schema aligns with typical database design practices. Once these changes are made, we can proceed with approving your pull request. Keep up the great work, and don't hesitate to reach out if you have any questions! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

task.sql Outdated
Comment on lines 9 to 10
Price VARCHAR(50),
WarehouseAmount VARCHAR(50),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'Price' column is defined as VARCHAR(50). Typically, prices should be stored as a numeric type, such as DECIMAL, to allow for proper arithmetic operations and to ensure data integrity.

task.sql Outdated
Comment on lines 10 to 11
WarehouseAmount VARCHAR(50),
PRIMARY KEY (ProductID)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'WarehouseAmount' column is defined as VARCHAR(50). This should likely be an integer type, such as INT, to accurately represent the quantity of products available in the warehouse.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on making the necessary changes to the task.sql file to meet the task requirements! 🎉 The WarehouseAmount column is now correctly defined as INT, and the Price column is also defined as INT. However, to fully align with typical database design practices for handling prices, it's recommended to update the Price column to use the DECIMAL(10,2) data type. This change will allow for more accurate representation of monetary values, accommodating fractional amounts which are common in pricing.

Your efforts are commendable, and this decision is made to help you move forward with your learning. Please review the comments to improve your code further. Addressing these issues in the future will help you avoid similar problems and enhance your coding skills. Keep up the good work and continue learning from your experiences! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

task.sql Outdated
ProductID INT AUTO_INCREMENT,
Name VARCHAR(50),
Description VARCHAR(50),
Price INT,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing the Price column from INT to DECIMAL(10,2) to accommodate fractional values, which are typical for prices. This will allow for more accurate representation of monetary values.

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.

2 participants