diff --git a/Java/PROJECT_ANALYSIS.md b/Java/PROJECT_ANALYSIS.md new file mode 100644 index 00000000..a19017b8 --- /dev/null +++ b/Java/PROJECT_ANALYSIS.md @@ -0,0 +1,103 @@ +# Project Analysis: GildedRose Refactoring Kata + +## 1. COMPLEXITY ANALYSIS + +### a. Functions with High Cyclomatic Complexity +- After refactoring, there are no functions with high cyclomatic complexity. The most complex logic (previously in `GildedRose.updateQuality`) has been delegated to item-specific classes, each with simple, linear logic. +- The only notable switch is in `GildedRose.wrapItem`, which is straightforward. + +### b. Long Parameter Lists +- No methods have long parameter lists. All constructors and methods use 0-3 parameters, which is manageable. + +### c. Deep Nesting Levels +- No deep nesting. The deepest is a single switch or if-else in item update methods. + +--- + +## 2. CODE QUALITY ISSUES + +### a. Potential Bugs +- **String Matching for Item Types:** + - `wrapItem` relies on exact string matches. Typos or whitespace issues could cause items to be misclassified as `NormalItem`. +- **Null Handling:** + - No null checks for the `items` array or for individual `Item` objects in the constructor. +- **Conjured Items:** + - The comment in `TexttestFixture` notes that "Conjured" items are not handled properly. + +### b. Performance Problems +- No significant performance issues. All loops are O(n) over the items array, which is expected and efficient for this domain. + +### c. Security Concerns +- No user input, file, or network operations in the main logic. No security concerns. + +### d. Maintainability Issues +- **Stringly-Typed Item Identification:** + - Hardcoded item names in `wrapItem` make it error-prone and less extensible. +- **Adding New Item Types:** + - Requires modifying `wrapItem`, violating the Open/Closed Principle. +- **No Validation:** + - No validation for item quality or sellIn values. +- **Test Coverage for New Item Types:** + - "Conjured" items are not implemented, but are present in tests. + +--- + +## 3. IMPROVEMENT ROADMAP + +### Quick Wins (< 1 hour) +1. **Add Null and Trim Checks** + - Validate that the `items` array and each `Item` are not null in the constructor. + - Trim whitespace from item names in `wrapItem`. + - Example: + ```java + if (items == null) throw new IllegalArgumentException("Items array cannot be null"); + if (item == null) throw new IllegalArgumentException("Item cannot be null"); + String name = item.name.trim(); + ``` + +2. **Add Comments and Documentation** + - Briefly document the purpose of each class and method, especially the item subclasses. + +### Medium Effort (1-4 hours) +1. **Use Enum for Item Types** + - Define an `enum` for item types and map names to enum values. + - Example: + ```java + enum ItemType { AGED_BRIE, BACKSTAGE_PASS, SULFURAS, NORMAL, CONJURED } + ``` + +2. **Factory Pattern for Item Creation** + - Move item creation logic to a factory class, making `GildedRose` cleaner and more extensible. + +3. **Implement Conjured Items** + - Add a `ConjuredItem` class and handle it in the factory. + +4. **Validation Logic** + - Add validation for item quality and sellIn in the constructor or in the `Item` class. + +### Major Refactoring (> 4 hours) +1. **Plugin System for New Item Types** + - Allow new item types to be registered at runtime, so adding a new type does not require modifying core classes. + +2. **Dependency Injection for Item Factories** + - Allow item creation logic to be injected, making the system more flexible and testable. + +3. **Comprehensive Error Handling and Logging** + - Add robust error handling and logging for invalid items or unexpected states. + +4. **Advanced Test Coverage** + - Expand tests to cover edge cases, invalid data, and new item types. + +--- + +## Summary Table + +| Effort | Issue/Opportunity | Example/Description | +|----------------|----------------------------------|-----------------------------------------------------| +| Quick Win | Null checks, trim names | Add validation in constructor and wrapItem | +| Medium Effort | Enum for item types, factory | Use enum and factory for item creation | +| Medium Effort | Implement Conjured items | Add ConjuredItem class and logic | +| Major Refactor | Plugin system, DI, error handling| Allow runtime registration, inject factories, logging| +| Major Refactor | Advanced test coverage | Add tests for edge cases and new item types | + + \ No newline at end of file diff --git a/Java/SECURITY_ANALYSIS.md b/Java/SECURITY_ANALYSIS.md new file mode 100644 index 00000000..6c7c9e72 --- /dev/null +++ b/Java/SECURITY_ANALYSIS.md @@ -0,0 +1,96 @@ +# Security Analysis: GildedRose Refactoring Kata + +--- + +## CRITICAL SECURITY ISSUES + +### 1. SQL Injection Vulnerabilities +- **Finding:** None found. The project does not interact with any databases or execute SQL queries. + +### 2. Cross-site Scripting (XSS) Risks +- **Finding:** None found. The project is a backend Java application with no web interface or HTML output. + +### 3. Authentication/Authorization Flaws +- **Finding:** None found. There is no authentication or authorization logic in the codebase. + +### 4. Input Validation Gaps +- **Severity:** Low +- **Location:** `GildedRose.java`, constructor and `wrapItem` method +- **Explanation:** + - The code does not validate the `items` array or individual `Item` objects for null values or invalid data (e.g., negative quality, null names). + - While not a direct security risk in this context, lack of validation could lead to unexpected exceptions or logic errors if the code is ever extended to accept user input. +- **Concrete Fix:** + ```java + public GildedRose(Item[] items) { + if (items == null) throw new IllegalArgumentException("Items array cannot be null"); + this.items = new UpdatableItem[items.length]; + for (int i = 0; i < items.length; i++) { + if (items[i] == null) throw new IllegalArgumentException("Item at index " + i + " is null"); + if (items[i].name == null) throw new IllegalArgumentException("Item name cannot be null"); + if (items[i].quality < 0) throw new IllegalArgumentException("Item quality cannot be negative"); + this.items[i] = wrapItem(items[i]); + } + } + ``` +- **Prevention Strategies:** + - Always validate input, even if it is currently internal. + - Add unit tests for invalid data scenarios. + +### 5. Sensitive Data Exposure +- **Finding:** None found. No sensitive data (passwords, keys, PII) is handled or stored. + +--- + +## SECURITY CONCERNS + +### 1. Hardcoded Secrets/Passwords +- **Finding:** None found. No secrets, passwords, or API keys are present in the codebase. + +### 2. Insecure Data Transmission +- **Finding:** None found. The application does not transmit data over a network. + +### 3. Weak Error Handling +- **Severity:** Low +- **Location:** General (e.g., `GildedRose.java` constructor) +- **Explanation:** + - The code throws generic exceptions (e.g., `IllegalArgumentException`) without custom error messages or logging. + - In a larger system, this could make debugging or security monitoring more difficult. +- **Concrete Fix:** + ```java + if (items == null) throw new IllegalArgumentException("Items array cannot be null (GildedRose.java:5)"); + ``` +- **Prevention Strategies:** + - Use descriptive error messages. + - Consider logging errors for audit trails in production systems. + +### 4. Missing Rate Limiting +- **Finding:** None found. The application does not expose any endpoints or services. + +### 5. Cryptographic Weaknesses +- **Finding:** None found. No cryptographic operations are performed. + +--- + +## Summary Table + +| Issue Type | Severity | Location/Line(s) | Explanation & Fix | +|---------------------------|----------|--------------------------|----------------------------------------------------------------------------------------------------| +| Input validation gaps | Low | GildedRose.java (5–13) | Add null/invalid checks for items and fields. See code example above. | +| Weak error handling | Low | GildedRose.java (5–13) | Use descriptive error messages and consider logging. | + +--- + +## Prevention Strategies (General) +- Always validate all input, even if it is not user-facing. +- Avoid hardcoding secrets or sensitive data. +- Use descriptive error messages and log errors where appropriate. +- If adding network/database features in the future, follow secure coding best practices (parameterized queries, HTTPS, etc.). +- Regularly review dependencies for vulnerabilities. + +--- + +### Conclusion + +**No critical or high-severity security issues were found.** +The only minor concerns are around input validation and error handling, which are best practices for future extensibility and robustness. +If you plan to extend this project (e.g., add a web interface, database, or external input), revisit this analysis and apply secure coding principles accordingly. \ No newline at end of file diff --git a/Java/src/main/java/com/gildedrose/AgedBrieItem.java b/Java/src/main/java/com/gildedrose/AgedBrieItem.java new file mode 100644 index 00000000..8cca53b0 --- /dev/null +++ b/Java/src/main/java/com/gildedrose/AgedBrieItem.java @@ -0,0 +1,13 @@ +package com.gildedrose; + +public class AgedBrieItem extends UpdatableItem { + public AgedBrieItem(int sellIn, int quality) { + super("Aged Brie", sellIn, quality); + } + @Override + public void update() { + sellIn--; + if (quality < 50) quality++; + if (sellIn < 0 && quality < 50) quality++; + } +} \ No newline at end of file diff --git a/Java/src/main/java/com/gildedrose/BackstagePassItem.java b/Java/src/main/java/com/gildedrose/BackstagePassItem.java new file mode 100644 index 00000000..0e44365e --- /dev/null +++ b/Java/src/main/java/com/gildedrose/BackstagePassItem.java @@ -0,0 +1,20 @@ +package com.gildedrose; + +public class BackstagePassItem extends UpdatableItem { + public BackstagePassItem(int sellIn, int quality) { + super("Backstage passes to a TAFKAL80ETC concert", sellIn, quality); + } + @Override + public void update() { + sellIn--; + if (sellIn < 0) { + quality = 0; + } else if (sellIn < 5) { + quality = Math.min(quality + 3, 50); + } else if (sellIn < 10) { + quality = Math.min(quality + 2, 50); + } else if (quality < 50) { + quality++; + } + } +} \ No newline at end of file diff --git a/Java/src/main/java/com/gildedrose/GildedRose.java b/Java/src/main/java/com/gildedrose/GildedRose.java index 87a3b926..b00b175a 100644 --- a/Java/src/main/java/com/gildedrose/GildedRose.java +++ b/Java/src/main/java/com/gildedrose/GildedRose.java @@ -1,62 +1,31 @@ package com.gildedrose; class GildedRose { - Item[] items; + UpdatableItem[] items; public GildedRose(Item[] items) { - this.items = items; + this.items = new UpdatableItem[items.length]; + for (int i = 0; i < items.length; i++) { + this.items[i] = wrapItem(items[i]); + } + } + + private UpdatableItem wrapItem(Item item) { + switch (item.name) { + case "Aged Brie": + return new AgedBrieItem(item.sellIn, item.quality); + case "Backstage passes to a TAFKAL80ETC concert": + return new BackstagePassItem(item.sellIn, item.quality); + case "Sulfuras, Hand of Ragnaros": + return new SulfurasItem(item.sellIn, item.quality); + default: + return new NormalItem(item.name, item.sellIn, item.quality); + } } public void updateQuality() { - for (int i = 0; i < items.length; i++) { - if (!items[i].name.equals("Aged Brie") - && !items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) { - if (items[i].quality > 0) { - if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) { - items[i].quality = items[i].quality - 1; - } - } - } else { - if (items[i].quality < 50) { - items[i].quality = items[i].quality + 1; - - if (items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) { - if (items[i].sellIn < 11) { - if (items[i].quality < 50) { - items[i].quality = items[i].quality + 1; - } - } - - if (items[i].sellIn < 6) { - if (items[i].quality < 50) { - items[i].quality = items[i].quality + 1; - } - } - } - } - } - - if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) { - items[i].sellIn = items[i].sellIn - 1; - } - - if (items[i].sellIn < 0) { - if (!items[i].name.equals("Aged Brie")) { - if (!items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) { - if (items[i].quality > 0) { - if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) { - items[i].quality = items[i].quality - 1; - } - } - } else { - items[i].quality = items[i].quality - items[i].quality; - } - } else { - if (items[i].quality < 50) { - items[i].quality = items[i].quality + 1; - } - } - } + for (UpdatableItem item : items) { + item.update(); } } } diff --git a/Java/src/main/java/com/gildedrose/NormalItem.java b/Java/src/main/java/com/gildedrose/NormalItem.java new file mode 100644 index 00000000..bc38822c --- /dev/null +++ b/Java/src/main/java/com/gildedrose/NormalItem.java @@ -0,0 +1,13 @@ +package com.gildedrose; + +public class NormalItem extends UpdatableItem { + public NormalItem(String name, int sellIn, int quality) { + super(name, sellIn, quality); + } + @Override + public void update() { + sellIn--; + if (quality > 0) quality--; + if (sellIn < 0 && quality > 0) quality--; + } +} \ No newline at end of file diff --git a/Java/src/main/java/com/gildedrose/SulfurasItem.java b/Java/src/main/java/com/gildedrose/SulfurasItem.java new file mode 100644 index 00000000..c01ff6be --- /dev/null +++ b/Java/src/main/java/com/gildedrose/SulfurasItem.java @@ -0,0 +1,11 @@ +package com.gildedrose; + +public class SulfurasItem extends UpdatableItem { + public SulfurasItem(int sellIn, int quality) { + super("Sulfuras, Hand of Ragnaros", sellIn, quality); + } + @Override + public void update() { + // Legendary item, does not change + } +} \ No newline at end of file diff --git a/Java/src/main/java/com/gildedrose/UpdatableItem.java b/Java/src/main/java/com/gildedrose/UpdatableItem.java new file mode 100644 index 00000000..c3b70e76 --- /dev/null +++ b/Java/src/main/java/com/gildedrose/UpdatableItem.java @@ -0,0 +1,8 @@ +package com.gildedrose; + +public abstract class UpdatableItem extends Item { + public UpdatableItem(String name, int sellIn, int quality) { + super(name, sellIn, quality); + } + public abstract void update(); +} \ No newline at end of file