mirror of
https://github.com/emilybache/GildedRose-Refactoring-Kata.git
synced 2026-02-04 09:11:39 +00:00
AI updated code
This commit is contained in:
parent
69245fd714
commit
c0c548df13
103
Java/PROJECT_ANALYSIS.md
Normal file
103
Java/PROJECT_ANALYSIS.md
Normal file
@ -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 |
|
||||
|
||||
</rewritten_file>
|
||||
96
Java/SECURITY_ANALYSIS.md
Normal file
96
Java/SECURITY_ANALYSIS.md
Normal file
@ -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.
|
||||
13
Java/src/main/java/com/gildedrose/AgedBrieItem.java
Normal file
13
Java/src/main/java/com/gildedrose/AgedBrieItem.java
Normal file
@ -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++;
|
||||
}
|
||||
}
|
||||
20
Java/src/main/java/com/gildedrose/BackstagePassItem.java
Normal file
20
Java/src/main/java/com/gildedrose/BackstagePassItem.java
Normal file
@ -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++;
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -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();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
13
Java/src/main/java/com/gildedrose/NormalItem.java
Normal file
13
Java/src/main/java/com/gildedrose/NormalItem.java
Normal file
@ -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--;
|
||||
}
|
||||
}
|
||||
11
Java/src/main/java/com/gildedrose/SulfurasItem.java
Normal file
11
Java/src/main/java/com/gildedrose/SulfurasItem.java
Normal file
@ -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
|
||||
}
|
||||
}
|
||||
8
Java/src/main/java/com/gildedrose/UpdatableItem.java
Normal file
8
Java/src/main/java/com/gildedrose/UpdatableItem.java
Normal file
@ -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();
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user