-
Couldn't load subscription status.
- Fork 12
Description
So, what is it about?
Issue Description
Current Behavior 📝
The current implementation of the Discount Coupon system is functional but relies on older C++ practices that can lead to bugs and maintenance challenges.
-
Manual Memory Management: The project extensively uses raw pointers (
*) with manualnewanddeletecalls. For example,Product,Cart, and allCouponderivatives are allocated on the heap inmain(), but theCouponManagerdoes not take ownership or clean up the registered coupons, leading to memory leaks. Similarly, theDiscountStrategyManagerfactory returns a raw pointer, forcing the caller (SeasonalOffer,LoyaltyDiscount, etc.) to manuallydeletethe strategy object. This approach is error-prone. -
Non-Thread-Safe Singletons: The
CouponManagerandDiscountStrategyManagersingletons use a non-thread-safe initialization pattern (if (!instance) { instance = new ...; }). If two threads callgetInstance()simultaneously for the first time, it could lead to a race condition where two instances are created, violating the singleton pattern and potentially causing crashes. The functional requirements explicitly list "Thread Safe" as a goal, which is not currently met.
Expected Behavior ✅
The codebase should be refactored to adopt modern C++ idioms for improved safety, robustness, and readability.
-
Automatic Memory Management with Smart Pointers: All dynamic memory ownership should be handled by smart pointers.
- Use
std::unique_ptrfor exclusive ownership (e.g., theDiscountStrategywithin aCoupon, or thenextcoupon in the Chain of Responsibility). - This eliminates the need for manual
deletecalls and prevents memory leaks by design, embracing the RAII (Resource Acquisition Is Initialization) principle.
- Use
-
Thread-Safe Singleton Implementation: The singleton classes should be updated to use the thread-safe "Meyers' Singleton" pattern, which relies on a static local variable. This is guaranteed to be initialized safely and only once in a multi-threaded context since C++11.
Motivation for Change 💡
This refactoring is essential for several reasons:
- Safety: It will eliminate an entire class of bugs related to memory management (leaks, double frees, dangling pointers).
- Maintainability: The code becomes cleaner and easier to reason about when resource ownership is clear and automatic.
- Correctness: It fulfills the explicit requirement of making the system thread-safe.
- Modernization: It aligns the project with current C++ best practices, making it a better learning resource and a more robust application.
Suggested Implementation 🛠️
-
Update Header: Include the
<memory>header where smart pointers are needed. -
Refactor Singletons for Thread Safety:
- Change the
getInstance()methods to return a reference and use a static local variable. This is simpler and thread-safe.
// In CouponManager.h / .cpp class CouponManager { public: static CouponManager& getInstance() { static CouponManager instance; // Thread-safe initialization return instance; } // ... remove static instance pointer and make constructor/destructor private };
- Change the
-
Use
std::unique_ptrfor Ownership:- Update the
Couponclass to use a smart pointer for the next link in the chain.
// In Coupon.h #include <memory> class Coupon { private: std::unique_ptr<Coupon> next; public: void setNext(std::unique_ptr<Coupon> nxt) { next = std::move(nxt); } // ... update other methods accordingly };
- Update factory methods like
DiscountStrategyManager::getStrategyto return astd::unique_ptr.
// In DiscountStrategyManager.h std::unique_ptr<DiscountStrategy> getStrategy(StrategyType type, double param1, double param2 = 0.0) const { if (type == StrategyType::FLAT) { return std::make_unique<FlatDiscountStrategy>(param1); } // ... etc. return nullptr; }
- Update the client code in
main()to usestd::make_uniqueandstd::movewhen creating objects and passing ownership.
// In main() auto& mgr = CouponManager::getInstance(); // Use reference mgr.registerCoupon(std::make_unique<SeasonalOffer>(10, "Clothing")); mgr.registerCoupon(std::make_unique<LoyaltyDiscount>(5)); // ... no more manual delete calls are needed at the end of main!
- Update the
Code of Conduct
- I agree to follow this project's Code of Conduct