Skip to main content
Code Review

Return to Answer

Rollback to Revision 1
Source Link
Quuxplusone
  • 19.7k
  • 2
  • 44
  • 91
  1. You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

  2. ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

(1) You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

(2) ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

  1. std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

  2. ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see that the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

  3. That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

  4. In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

  5. Nitpick: constify your const methods.

  6. Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

  7. In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided .) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

(3) std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

Let's(4) ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see what we're leftthat the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

(5) That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

(6) In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

(7) Nitpick:constify your const methods.

(8) Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

(9) In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided .) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

So let's see, what are we left with...?

template <class T> struct Holder
{
 T* Read(const std::string& Path) const {
 auto iter = Cache.find(Path);
 return iter == Cache.end() ? nullptr : iter->second.get();
 }
 void Write(const std::string& Path, T* Data) {
 Cache[Path] = std::unique_ptr<T>(Data);
 }
 virtual ~Holder() = default;
 std::unordered_map<std::string, std::unique_ptr<T>> Cache;
};
class ObjectHolder_t : private Holder<Object>
{
public:
 Object* Read(const std::string& Handle) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 return Holder::Read(ObjHandle);
 return GetHolder(ObjHandle)->Read(Leftover);
 }
 void Write(const string& Handle, Object* pObject) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 Holder::Write(ObjHandle, pObject);
 else
 GetHolder(ObjHandle)->Write(Leftover, pObject);
 }
 template <class F>
 void Execute(const string& Handle, F Func) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (ObjHandle.back() == '*') {
 ObjHandle.front() == '@' ? WildcardAlias(Leftover, ObjHandle, Func) : WildcardCache(Leftover, ObjHandle, Func);
 } else {
 Leftover.empty() ? Func(&Cache.find(ObjHandle)->second) : GetHolder(ObjHandle)->Execute(Leftover, Func);
 }
 }
 void WriteAlias(const std::string& Handle, const std::string& Alias) {
 Aliases[Alias] = Handle;
 }
private:
 template <class F>
 void ExecuteSafe(const string& HolderHandle, const string& Handle, F Func)
 {
 if (ObjectHolder_t* pHolder = GetHolder(HolderHandle))
 pHolder->Execute(Handle, Func);
 }
 template <class F>
 void WildcardAlias(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle.substr(1)));
 for (auto i = Aliases.begin(); i != Aliases.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Execute(i->second, Func) : ExecuteSafe(i->second, Leftover, Func);
 }
 template <class F>
 void WildcardCache(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle));
 for (auto i = Cache.begin(); i != Cache.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Func(&i->second) : ExecuteSafe(i->first, Leftover, Func);
 }
 std::string Regexify(const std::string& Wildcard)
 {
 return "^" + Wildcard.substr(0, Wildcard.size() - 1) + ".*";
 }
 std::pair<std::string, std::string> ExtractObjHandle(const std::string& Handle)
 {
 std::pair<std::string, std::string> Result;
 size_t Index = Handle.find('/');
 if (Index != Handle.npos) {
 Result.first = Handle.substr(0, Index);
 Result.second = Handle.substr(Index + 1);
 } else {
 Result.first = Handle;
 Result.second.clear();
 }
 std::string& ObjHandle = Result.first;
 if (ObjHandle.front() == '@' && ObjHandle.back() != '*') {
 Result.second = Aliases[ObjHandle.substr(1)] + "/" + Result.second;
 ObjHandle = ExtractObjHandle(Result.second);
 }
 return Result;
 }
 ObjectHolder_t* GetHolder(const std::string& Handle)
 {
 return (ObjectHolder_t*)(Holder::Read(Handle));
 }
 std::unordered_map<std::string, std::string> Aliases;
};
  1. What if function F throws an exception – what guarantees does your code provide?

  2. What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function , but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

  3. The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

  4. A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iteratorsbegin, end so that your existing caller

     ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
     {
     if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
     pTexture->Move(Time, X, Y);
     });
    

(10) What if function F throws an exception – what guarantees does your code provide?

(11) What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function , but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

(12) The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

(13) A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iteratorsbegin, end so that your existing caller

ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
{
 if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
 pTexture->Move(Time, X, Y);
});
for (Object *pObject : ObjectHolder.Write(Handle)) {
 if (Texture *pTexture = dynamic_cast<Texture*>(pObject)) {
 pTexture->Move(Time, X, Y);
 }
}

That would be idiomatic C++11 (except for the smelly use of dynamic_cast, of course), and it's not significantly harder than what you're doing now. Give it a try and post a comment if you do! :)

  1. You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

  2. ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

  1. std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

  2. ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see that the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

  3. That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

  4. In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

  5. Nitpick: constify your const methods.

  6. Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

  7. In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided .) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

Let's see what we're left with:

template <class T> struct Holder
{
 T* Read(const std::string& Path) const {
 auto iter = Cache.find(Path);
 return iter == Cache.end() ? nullptr : iter->second.get();
 }
 void Write(const std::string& Path, T* Data) {
 Cache[Path] = std::unique_ptr<T>(Data);
 }
 virtual ~Holder() = default;
 std::unordered_map<std::string, std::unique_ptr<T>> Cache;
};
class ObjectHolder_t : private Holder<Object>
{
public:
 Object* Read(const std::string& Handle) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 return Holder::Read(ObjHandle);
 return GetHolder(ObjHandle)->Read(Leftover);
 }
 void Write(const string& Handle, Object* pObject) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 Holder::Write(ObjHandle, pObject);
 else
 GetHolder(ObjHandle)->Write(Leftover, pObject);
 }
 template <class F>
 void Execute(const string& Handle, F Func) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (ObjHandle.back() == '*') {
 ObjHandle.front() == '@' ? WildcardAlias(Leftover, ObjHandle, Func) : WildcardCache(Leftover, ObjHandle, Func);
 } else {
 Leftover.empty() ? Func(&Cache.find(ObjHandle)->second) : GetHolder(ObjHandle)->Execute(Leftover, Func);
 }
 }
 void WriteAlias(const std::string& Handle, const std::string& Alias) {
 Aliases[Alias] = Handle;
 }
private:
 template <class F>
 void ExecuteSafe(const string& HolderHandle, const string& Handle, F Func)
 {
 if (ObjectHolder_t* pHolder = GetHolder(HolderHandle))
 pHolder->Execute(Handle, Func);
 }
 template <class F>
 void WildcardAlias(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle.substr(1)));
 for (auto i = Aliases.begin(); i != Aliases.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Execute(i->second, Func) : ExecuteSafe(i->second, Leftover, Func);
 }
 template <class F>
 void WildcardCache(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle));
 for (auto i = Cache.begin(); i != Cache.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Func(&i->second) : ExecuteSafe(i->first, Leftover, Func);
 }
 std::string Regexify(const std::string& Wildcard)
 {
 return "^" + Wildcard.substr(0, Wildcard.size() - 1) + ".*";
 }
 std::pair<std::string, std::string> ExtractObjHandle(const std::string& Handle)
 {
 std::pair<std::string, std::string> Result;
 size_t Index = Handle.find('/');
 if (Index != Handle.npos) {
 Result.first = Handle.substr(0, Index);
 Result.second = Handle.substr(Index + 1);
 } else {
 Result.first = Handle;
 Result.second.clear();
 }
 std::string& ObjHandle = Result.first;
 if (ObjHandle.front() == '@' && ObjHandle.back() != '*') {
 Result.second = Aliases[ObjHandle.substr(1)] + "/" + Result.second;
 ObjHandle = ExtractObjHandle(Result.second);
 }
 return Result;
 }
 ObjectHolder_t* GetHolder(const std::string& Handle)
 {
 return (ObjectHolder_t*)(Holder::Read(Handle));
 }
 std::unordered_map<std::string, std::string> Aliases;
};
  1. What if function F throws an exception – what guarantees does your code provide?

  2. What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function , but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

  3. The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

  4. A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iteratorsbegin, end so that your existing caller

     ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
     {
     if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
     pTexture->Move(Time, X, Y);
     });
    
for (Object *pObject : ObjectHolder.Write(Handle)) {
 if (Texture *pTexture = dynamic_cast<Texture*>(pObject)) {
 pTexture->Move(Time, X, Y);
 }
}

That would be idiomatic C++11 (except for the smelly use of dynamic_cast, of course), and it's not significantly harder than what you're doing now. Give it a try and post a comment if you do!

(1) You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

(2) ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

(3) std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

(4) ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see that the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

(5) That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

(6) In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

(7) Nitpick:constify your const methods.

(8) Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

(9) In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided .) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

So let's see, what are we left with...?

template <class T> struct Holder
{
 T* Read(const std::string& Path) const {
 auto iter = Cache.find(Path);
 return iter == Cache.end() ? nullptr : iter->second.get();
 }
 void Write(const std::string& Path, T* Data) {
 Cache[Path] = std::unique_ptr<T>(Data);
 }
 virtual ~Holder() = default;
 std::unordered_map<std::string, std::unique_ptr<T>> Cache;
};
class ObjectHolder_t : private Holder<Object>
{
public:
 Object* Read(const std::string& Handle) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 return Holder::Read(ObjHandle);
 return GetHolder(ObjHandle)->Read(Leftover);
 }
 void Write(const string& Handle, Object* pObject) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 Holder::Write(ObjHandle, pObject);
 else
 GetHolder(ObjHandle)->Write(Leftover, pObject);
 }
 template <class F>
 void Execute(const string& Handle, F Func) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (ObjHandle.back() == '*') {
 ObjHandle.front() == '@' ? WildcardAlias(Leftover, ObjHandle, Func) : WildcardCache(Leftover, ObjHandle, Func);
 } else {
 Leftover.empty() ? Func(&Cache.find(ObjHandle)->second) : GetHolder(ObjHandle)->Execute(Leftover, Func);
 }
 }
 void WriteAlias(const std::string& Handle, const std::string& Alias) {
 Aliases[Alias] = Handle;
 }
private:
 template <class F>
 void ExecuteSafe(const string& HolderHandle, const string& Handle, F Func)
 {
 if (ObjectHolder_t* pHolder = GetHolder(HolderHandle))
 pHolder->Execute(Handle, Func);
 }
 template <class F>
 void WildcardAlias(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle.substr(1)));
 for (auto i = Aliases.begin(); i != Aliases.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Execute(i->second, Func) : ExecuteSafe(i->second, Leftover, Func);
 }
 template <class F>
 void WildcardCache(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle));
 for (auto i = Cache.begin(); i != Cache.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Func(&i->second) : ExecuteSafe(i->first, Leftover, Func);
 }
 std::string Regexify(const std::string& Wildcard)
 {
 return "^" + Wildcard.substr(0, Wildcard.size() - 1) + ".*";
 }
 std::pair<std::string, std::string> ExtractObjHandle(const std::string& Handle)
 {
 std::pair<std::string, std::string> Result;
 size_t Index = Handle.find('/');
 if (Index != Handle.npos) {
 Result.first = Handle.substr(0, Index);
 Result.second = Handle.substr(Index + 1);
 } else {
 Result.first = Handle;
 Result.second.clear();
 }
 std::string& ObjHandle = Result.first;
 if (ObjHandle.front() == '@' && ObjHandle.back() != '*') {
 Result.second = Aliases[ObjHandle.substr(1)] + "/" + Result.second;
 ObjHandle = ExtractObjHandle(Result.second);
 }
 return Result;
 }
 ObjectHolder_t* GetHolder(const std::string& Handle)
 {
 return (ObjectHolder_t*)(Holder::Read(Handle));
 }
 std::unordered_map<std::string, std::string> Aliases;
};

(10) What if function F throws an exception – what guarantees does your code provide?

(11) What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function , but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

(12) The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

(13) A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iteratorsbegin, end so that your existing caller

ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
{
 if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
 pTexture->Move(Time, X, Y);
});
for (Object *pObject : ObjectHolder.Write(Handle)) {
 if (Texture *pTexture = dynamic_cast<Texture*>(pObject)) {
 pTexture->Move(Time, X, Y);
 }
}

That would be idiomatic C++11 (except for the smelly use of dynamic_cast, of course), and it's not significantly harder than what you're doing now. Give it a try and post a comment if you do! :)

added 396 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

(1) You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

(2) ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

  1. You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

  2. ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

(3) std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

(4) ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see that the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

(5) That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

(6) In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

(7) Nitpick: constify your const methods.

(8) Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

(9) In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided .) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

  1. std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

  2. ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see that the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

  3. That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

  4. In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

  5. Nitpick: constify your const methods.

  6. Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

  7. In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided .) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

So let'sLet's see, what are wewe're left with...?:

template <class T> struct Holder
{
 T* Read(const std::string& Path) const {
 auto iter = Cache.find(Path);
 return iter == Cache.end() ? nullptr : iter->second.get();
 }
 void Write(const std::string& Path, T* Data) {
 Cache[Path] = std::unique_ptr<T>(Data);
 }
 virtual ~Holder() = default;
 std::unordered_map<std::string, std::unique_ptr<T>> Cache;
};
class ObjectHolder_t : private Holder<Object>
{
public:
 Object* Read(const std::string& Handle) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 return Holder::Read(ObjHandle);
 return GetHolder(ObjHandle)->Read(Leftover);
 }
 void Write(const string& Handle, Object* pObject) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 Holder::Write(ObjHandle, pObject);
 else
 GetHolder(ObjHandle)->Write(Leftover, pObject);
 }
 template <class F>
 void Execute(const string& Handle, F Func) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (ObjHandle.back() == '*') {
 ObjHandle.front() == '@' ? WildcardAlias(Leftover, ObjHandle, Func) : WildcardCache(Leftover, ObjHandle, Func);
 } else {
 Leftover.empty() ? Func(&Cache.find(ObjHandle)->second) : GetHolder(ObjHandle)->Execute(Leftover, Func);
 }
 }
 void WriteAlias(const std::string& Handle, const std::string& Alias) {
 Aliases[Alias] = Handle;
 }
private:
 template <class F>
 void ExecuteSafe(const string& HolderHandle, const string& Handle, F Func)
 {
 if (ObjectHolder_t* pHolder = GetHolder(HolderHandle))
 pHolder->Execute(Handle, Func);
 }
 template <class F>
 void WildcardAlias(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle.substr(1)));
 for (auto i = Aliases.begin(); i != Aliases.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Execute(i->second, Func) : ExecuteSafe(i->second, Leftover, Func);
 }
 template <class F>
 void WildcardCache(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle));
 for (auto i = Cache.begin(); i != Cache.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Func(&i->second) : ExecuteSafe(i->first, Leftover, Func);
 }
 std::string Regexify(const std::string& Wildcard)
 {
 return "^" + Wildcard.substr(0, Wildcard.size() - 1) + ".*";
 }
 std::pair<std::string, std::string> ExtractObjHandle(const std::string& Handle)
 {
 std::pair<std::string, std::string> Result;
 size_t Index = Handle.find('/');
 if (Index != Handle.npos) {
 Result.first = Handle.substr(0, Index);
 Result.second = Handle.substr(Index + 1);
 } else {
 Result.first = Handle;
 Result.second.clear();
 }
 std::string& ObjHandle = Result.first;
 if (ObjHandle.front() == '@' && ObjHandle.back() != '*') {
 Result.second = Aliases[ObjHandle.substr(1)] + "/" + Result.second;
 ObjHandle = ExtractObjHandle(Result.second);
 }
 return Result;
 }
 ObjectHolder_t* GetHolder(const std::string& Handle)
 {
 return (ObjectHolder_t*)(Holder::Read(Handle));
 }
 std::unordered_map<std::string, std::string> Aliases;
};

(10) What if function F throws an exception – what guarantees does your code provide?

(11) What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function , but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

(12) The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

(13) A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iteratorsbegin, end so that your existing caller

ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
{
 if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
 pTexture->Move(Time, X, Y);
});
  1. What if function F throws an exception – what guarantees does your code provide?

  2. What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function , but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

  3. The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

  4. A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iteratorsbegin, end so that your existing caller

     ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
     {
     if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
     pTexture->Move(Time, X, Y);
     });
    
for (Object *pObject : ObjectHolder.Write(Handle)) {
 if (Texture *pTexture = dynamic_cast<Texture*>(pObject)) {
 pTexture->Move(Time, X, Y);
 }
}

That would be idiomatic C++11 (except for the smelly use of dynamic_cast, of course), and it's not significantly harder than what you're doing now. Give it a try and post a comment if you do! :)

(1) You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

(2) ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

(3) std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

(4) ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see that the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

(5) That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

(6) In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

(7) Nitpick: constify your const methods.

(8) Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

(9) In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided .) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

So let's see, what are we left with...?

template <class T> struct Holder
{
 T* Read(const std::string& Path) const {
 auto iter = Cache.find(Path);
 return iter == Cache.end() ? nullptr : iter->second.get();
 }
 void Write(const std::string& Path, T* Data) {
 Cache[Path] = std::unique_ptr<T>(Data);
 }
 virtual ~Holder() = default;
 std::unordered_map<std::string, std::unique_ptr<T>> Cache;
};
class ObjectHolder_t : private Holder<Object>
{
public:
 Object* Read(const std::string& Handle) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 return Holder::Read(ObjHandle);
 return GetHolder(ObjHandle)->Read(Leftover);
 }
 void Write(const string& Handle, Object* pObject) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 Holder::Write(ObjHandle, pObject);
 else
 GetHolder(ObjHandle)->Write(Leftover, pObject);
 }
 template <class F>
 void Execute(const string& Handle, F Func) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (ObjHandle.back() == '*') {
 ObjHandle.front() == '@' ? WildcardAlias(Leftover, ObjHandle, Func) : WildcardCache(Leftover, ObjHandle, Func);
 } else {
 Leftover.empty() ? Func(&Cache.find(ObjHandle)->second) : GetHolder(ObjHandle)->Execute(Leftover, Func);
 }
 }
 void WriteAlias(const std::string& Handle, const std::string& Alias) {
 Aliases[Alias] = Handle;
 }
private:
 template <class F>
 void ExecuteSafe(const string& HolderHandle, const string& Handle, F Func)
 {
 if (ObjectHolder_t* pHolder = GetHolder(HolderHandle))
 pHolder->Execute(Handle, Func);
 }
 template <class F>
 void WildcardAlias(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle.substr(1)));
 for (auto i = Aliases.begin(); i != Aliases.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Execute(i->second, Func) : ExecuteSafe(i->second, Leftover, Func);
 }
 template <class F>
 void WildcardCache(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle));
 for (auto i = Cache.begin(); i != Cache.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Func(&i->second) : ExecuteSafe(i->first, Leftover, Func);
 }
 std::string Regexify(const std::string& Wildcard)
 {
 return "^" + Wildcard.substr(0, Wildcard.size() - 1) + ".*";
 }
 std::pair<std::string, std::string> ExtractObjHandle(const std::string& Handle)
 {
 std::pair<std::string, std::string> Result;
 size_t Index = Handle.find('/');
 if (Index != Handle.npos) {
 Result.first = Handle.substr(0, Index);
 Result.second = Handle.substr(Index + 1);
 } else {
 Result.first = Handle;
 Result.second.clear();
 }
 std::string& ObjHandle = Result.first;
 if (ObjHandle.front() == '@' && ObjHandle.back() != '*') {
 Result.second = Aliases[ObjHandle.substr(1)] + "/" + Result.second;
 ObjHandle = ExtractObjHandle(Result.second);
 }
 return Result;
 }
 ObjectHolder_t* GetHolder(const std::string& Handle)
 {
 return (ObjectHolder_t*)(Holder::Read(Handle));
 }
 std::unordered_map<std::string, std::string> Aliases;
};

(10) What if function F throws an exception – what guarantees does your code provide?

(11) What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function , but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

(12) The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

(13) A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iteratorsbegin, end so that your existing caller

ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
{
 if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
 pTexture->Move(Time, X, Y);
});
for (Object *pObject : ObjectHolder.Write(Handle)) {
 if (Texture *pTexture = dynamic_cast<Texture*>(pObject)) {
 pTexture->Move(Time, X, Y);
 }
}

That would be idiomatic C++11 (except for the smelly use of dynamic_cast, of course), and it's not significantly harder than what you're doing now. Give it a try and post a comment if you do! :)

  1. You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

  2. ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

  1. std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

  2. ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see that the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

  3. That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

  4. In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

  5. Nitpick: constify your const methods.

  6. Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

  7. In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided .) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

Let's see what we're left with:

template <class T> struct Holder
{
 T* Read(const std::string& Path) const {
 auto iter = Cache.find(Path);
 return iter == Cache.end() ? nullptr : iter->second.get();
 }
 void Write(const std::string& Path, T* Data) {
 Cache[Path] = std::unique_ptr<T>(Data);
 }
 virtual ~Holder() = default;
 std::unordered_map<std::string, std::unique_ptr<T>> Cache;
};
class ObjectHolder_t : private Holder<Object>
{
public:
 Object* Read(const std::string& Handle) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 return Holder::Read(ObjHandle);
 return GetHolder(ObjHandle)->Read(Leftover);
 }
 void Write(const string& Handle, Object* pObject) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 Holder::Write(ObjHandle, pObject);
 else
 GetHolder(ObjHandle)->Write(Leftover, pObject);
 }
 template <class F>
 void Execute(const string& Handle, F Func) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (ObjHandle.back() == '*') {
 ObjHandle.front() == '@' ? WildcardAlias(Leftover, ObjHandle, Func) : WildcardCache(Leftover, ObjHandle, Func);
 } else {
 Leftover.empty() ? Func(&Cache.find(ObjHandle)->second) : GetHolder(ObjHandle)->Execute(Leftover, Func);
 }
 }
 void WriteAlias(const std::string& Handle, const std::string& Alias) {
 Aliases[Alias] = Handle;
 }
private:
 template <class F>
 void ExecuteSafe(const string& HolderHandle, const string& Handle, F Func)
 {
 if (ObjectHolder_t* pHolder = GetHolder(HolderHandle))
 pHolder->Execute(Handle, Func);
 }
 template <class F>
 void WildcardAlias(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle.substr(1)));
 for (auto i = Aliases.begin(); i != Aliases.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Execute(i->second, Func) : ExecuteSafe(i->second, Leftover, Func);
 }
 template <class F>
 void WildcardCache(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle));
 for (auto i = Cache.begin(); i != Cache.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Func(&i->second) : ExecuteSafe(i->first, Leftover, Func);
 }
 std::string Regexify(const std::string& Wildcard)
 {
 return "^" + Wildcard.substr(0, Wildcard.size() - 1) + ".*";
 }
 std::pair<std::string, std::string> ExtractObjHandle(const std::string& Handle)
 {
 std::pair<std::string, std::string> Result;
 size_t Index = Handle.find('/');
 if (Index != Handle.npos) {
 Result.first = Handle.substr(0, Index);
 Result.second = Handle.substr(Index + 1);
 } else {
 Result.first = Handle;
 Result.second.clear();
 }
 std::string& ObjHandle = Result.first;
 if (ObjHandle.front() == '@' && ObjHandle.back() != '*') {
 Result.second = Aliases[ObjHandle.substr(1)] + "/" + Result.second;
 ObjHandle = ExtractObjHandle(Result.second);
 }
 return Result;
 }
 ObjectHolder_t* GetHolder(const std::string& Handle)
 {
 return (ObjectHolder_t*)(Holder::Read(Handle));
 }
 std::unordered_map<std::string, std::string> Aliases;
};
  1. What if function F throws an exception – what guarantees does your code provide?

  2. What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function , but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

  3. The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

  4. A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iteratorsbegin, end so that your existing caller

     ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
     {
     if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
     pTexture->Move(Time, X, Y);
     });
    
for (Object *pObject : ObjectHolder.Write(Handle)) {
 if (Texture *pTexture = dynamic_cast<Texture*>(pObject)) {
 pTexture->Move(Time, X, Y);
 }
}

That would be idiomatic C++11 (except for the smelly use of dynamic_cast, of course), and it's not significantly harder than what you're doing now. Give it a try and post a comment if you do!

Source Link
Quuxplusone
  • 19.7k
  • 2
  • 44
  • 91

The code is a bit verbose, which makes it hard to review in a tiny StackOverflow window... but here are my thoughts. I'll start with the nitpicks and then see about bigger issues.

(1) You use unadorned string and map, which means you're relying on using namespace std;. Don't do that. It's Just Not Done in typical C++ code.

(2) ObjectHolder_t has a virtual destructor, which is nice, but it already inherits from Holder<T>, whose destructor isn't virtual. (It's private inheritance, so I guess that's sort-of okay... but again it's bad style. It sends up red flags for the reader (me), which wastes my time even if they're false alarms.)

Also, speaking of the verbosity of this code, it's good style to put those empty braces on a single line instead of spread over three lines. Use virtual ~Holder() {} or virtual ~Holder() = default;, and remove the now-redundant destructor from ObjectHolder_t.

(3) std::map is a code smell; code using it can usually be optimized by replacing it with std::unordered_map. Unless of course your algorithm requires that the map be sorted, in which case, I'd add a // comment explaining that, right at the point where the reader is going to be smelling that std::map.

(4) ExtractObjHandle mutates its argument, in addition to returning another string by value. That's a big smelly code smell, and indeed we can see that the one place you use this subroutine (in Read, Write, and Execute), you have to explicitly copy the argument string because you don't want the original to be mutated. The proper way to return a pair of strings from a method is to change the return type to std::pair<std::string, std::string>. At that point you can also consider changing the parameter type from const std::string& to std::string if it would eliminate an explicit copy operation.

(5) That MapDeleter cruft seems like a buggy reimplementation of std::unique_ptr: you're just looking for something that holds a pointer and deletes it when it's destroyed. You can safely make the substitution.

(6) In fact, this whole "cache" thing is a bit boned from the start, because the Read method allows a pointer to "escape" from the cache's map. The caller could store that pointer somewhere, and then the cache gets destroyed (via ~Holder), which destroys the pointed-to object... and you end up with undefined behavior the next time you try to access the object through the original pointer. To fix this, the textbook approach is to use std::shared_ptr to manage lifetimes, and the real-world approach is "don't do that then". I'll assume your code doesn't do that, and that returning raw pointers to managed objects is totally fine by you.

(7) Nitpick: constify your const methods.

(8) Another nitpick: prefer Handle.npos to std::string::npos; it's just one fewer thing to worry about whether you matched up the types correctly. Modern C++ is quickly moving in the direction of "generic programming", which in a nutshell means never having to explicitly write out your data types if you don't need to. See also Herb Sutter's concept of "Almost Always Auto".

(9) In Regexify, you return string(...); where ... represents an expression of type string. The correct idiom here is simply return ...;. The former means "Evaluate ..., move-construct another string from it, and then move-construct the return value from that string." (The last move-construction may be elided.) As opposed to return ...;, which means "Evaluate ..., and then move-construct the return value from it." (The last move-construction may be elided.)

So let's see, what are we left with...?

template <class T> struct Holder
{
 T* Read(const std::string& Path) const {
 auto iter = Cache.find(Path);
 return iter == Cache.end() ? nullptr : iter->second.get();
 }
 void Write(const std::string& Path, T* Data) {
 Cache[Path] = std::unique_ptr<T>(Data);
 }
 virtual ~Holder() = default;
 std::unordered_map<std::string, std::unique_ptr<T>> Cache;
};
class ObjectHolder_t : private Holder<Object>
{
public:
 Object* Read(const std::string& Handle) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 return Holder::Read(ObjHandle);
 return GetHolder(ObjHandle)->Read(Leftover);
 }
 void Write(const string& Handle, Object* pObject) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (Leftover.empty())
 Holder::Write(ObjHandle, pObject);
 else
 GetHolder(ObjHandle)->Write(Leftover, pObject);
 }
 template <class F>
 void Execute(const string& Handle, F Func) {
 std::string ObjHandle, Leftover;
 std::tie(ObjHandle, Leftover) = ExtractObjHandle(Handle);
 if (ObjHandle.back() == '*') {
 ObjHandle.front() == '@' ? WildcardAlias(Leftover, ObjHandle, Func) : WildcardCache(Leftover, ObjHandle, Func);
 } else {
 Leftover.empty() ? Func(&Cache.find(ObjHandle)->second) : GetHolder(ObjHandle)->Execute(Leftover, Func);
 }
 }
 void WriteAlias(const std::string& Handle, const std::string& Alias) {
 Aliases[Alias] = Handle;
 }
private:
 template <class F>
 void ExecuteSafe(const string& HolderHandle, const string& Handle, F Func)
 {
 if (ObjectHolder_t* pHolder = GetHolder(HolderHandle))
 pHolder->Execute(Handle, Func);
 }
 template <class F>
 void WildcardAlias(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle.substr(1)));
 for (auto i = Aliases.begin(); i != Aliases.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Execute(i->second, Func) : ExecuteSafe(i->second, Leftover, Func);
 }
 template <class F>
 void WildcardCache(const string& Leftover, const string& ObjHandle, F Func)
 {
 std::regex Regex(Regexify(ObjHandle));
 for (auto i = Cache.begin(); i != Cache.end(); ++i)
 if (std::regex_match(i->first, Regex))
 Leftover.empty() ? Func(&i->second) : ExecuteSafe(i->first, Leftover, Func);
 }
 std::string Regexify(const std::string& Wildcard)
 {
 return "^" + Wildcard.substr(0, Wildcard.size() - 1) + ".*";
 }
 std::pair<std::string, std::string> ExtractObjHandle(const std::string& Handle)
 {
 std::pair<std::string, std::string> Result;
 size_t Index = Handle.find('/');
 if (Index != Handle.npos) {
 Result.first = Handle.substr(0, Index);
 Result.second = Handle.substr(Index + 1);
 } else {
 Result.first = Handle;
 Result.second.clear();
 }
 std::string& ObjHandle = Result.first;
 if (ObjHandle.front() == '@' && ObjHandle.back() != '*') {
 Result.second = Aliases[ObjHandle.substr(1)] + "/" + Result.second;
 ObjHandle = ExtractObjHandle(Result.second);
 }
 return Result;
 }
 ObjectHolder_t* GetHolder(const std::string& Handle)
 {
 return (ObjectHolder_t*)(Holder::Read(Handle));
 }
 std::unordered_map<std::string, std::string> Aliases;
};

...I didn't get all the way through the code, but hopefully this gives you some ideas and maybe I'll do a second round later.

Parting thoughts:

(10) What if function F throws an exception – what guarantees does your code provide?

(11) What is function F supposed to look like, anyway? A block comment explaining the expected input would be nice. (You should be aware of std::function, but there are perfectly valid reasons not to use it in a lot of code. Cough efficiency cough.)

(12) The expression Func(&Cache.find(ObjHandle)->second) sends up red flags: what would happen if ObjHandle weren't in the Cache? What if Cache.find(ObjHandle) returned nullptr? Et cetera. I'd prefer to see such a complicated raw-pointery expression broken down into simple steps, with each step checked for failure before proceeding to the next step.

(13) A major architectural suggestion: Consider trying a complete rewrite, where instead of passing F Func all the way down the stack, you instead make Read and Write return pairs of iterators begin, end so that your existing caller

ObjectHolder.Execute(Handle, [Time, X, Y] (Object** ppObject)
{
 if (Texture* pTexture = dynamic_cast<Texture*>(*ppObject))
 pTexture->Move(Time, X, Y);
});

ends up looking more like

for (Object *pObject : ObjectHolder.Write(Handle)) {
 if (Texture *pTexture = dynamic_cast<Texture*>(pObject)) {
 pTexture->Move(Time, X, Y);
 }
}

That would be idiomatic C++11 (except for the smelly use of dynamic_cast, of course), and it's not significantly harder than what you're doing now. Give it a try and post a comment if you do! :)

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /