0

I know that the answer will be obvious, probably not, but I'm just not completely sure.

For example somewhere it code in class/struct a have vector with objects in my case is Asset:

class Asset {
  int ID = - 1;
}

std::vector<Asset> GAssets;

And then i also have function that retrive asset by its ID. If asset is found i can return it by reference to modify its actually content inside somewhere in my code, but if asset not found i need to return invalid asset with ID -1. To do this i basically can return new stack object, but here my problem, i think how this would work? It return my asset that it found is right but in case with invalid asset and new stack object, it removes it form stack or return it?

Asset& GetAssetByID(int ID) {
  for(Asset& asset : GAssets) {
     if(asset.ID == ID)
        return asset;
  }

  return Asset(-1); // How this would returns?
}

NOTE: I'll tell you right away I could use pointers, but I don't need them because there are a lot of assets and I don't know when I will need to delete them. It's the same with smart pointers.

Kenny
  • 113
  • 9
  • 4
    You'd need an actual object that lives somewhere outside of the function. Perhaps a `static Asset InvalidAsset(-1);` at file scope – UnholySheep Aug 29 '22 at 21:13
  • 6
    Putting the `static` object inside the function itself might make more sense, if the "invalid" `Asset` is not needed anywhere else. Otherwise, you could just `throw` an exception if the `ID` is not found. Or, change the function to return an `Asset*` pointer instead of an `Asset&` reference, then you can return `nullptr` if the `ID` is not found. Or, you could change the function to return a `std::optional` instead, and then you can return `std::nullopt` if needed. – Remy Lebeau Aug 29 '22 at 21:14
  • "*I'll tell you right away I could use pointers, but I don't need them because there are a lot of assets and I don't know when I will need to delete them.*" What does that have to do with using a pointer? You return a pointer if it's there and `nullptr` if it isn't. Returning "invalid assets" is generally dangerous. – Nicol Bolas Aug 29 '22 at 21:19
  • Like Renny said, you could return a reference to a static: https://onlinegdb.com/UCzP9Bcjt It's not better than his other suggestions, but that's one way to do it. – Jerry Jeremiah Aug 29 '22 at 21:21
  • @UnholySheep Yes, i totally forgot, thanks for quick response:) Only in my case I have 4 functions that receive an asset (according to its different data) and I think it would be better to do **static Asset InvalidAsset** in some class, to not allocate 4 identical objects. – Kenny Aug 29 '22 at 21:21
  • I think an exception is a better choice here, for a few reasons: 1) your `Asset`'s `ID` property is mutable, as well as reference returned by the function, so the client code may end up just changing this value, no matter if it's `-1` or not (you may want to rething such a design) 2) If your asset has any other member variables, they are also carried with the instance of an invalid asset 3) exceptions are more apparent here – The Dreams Wind Aug 29 '22 at 21:21
  • @TheDreamsWind I agree, but my Asset is way more complicated) I just have a physical asset on the disk and its file next to it (its description), this description is read and loaded into GAssets. (In, Type, Relative path, etc.). And when I drag the asset in the engine panel, then I have the physical asset itself and its description transferred to another folder. Then its description will be updated (namely the relative path). And the problem was when I received the asset via GetAssetByID from GAssets, then about changing the relative path in GAssets, not the asset that I found was changed. – Kenny Aug 29 '22 at 21:33
  • @TheDreamsWind But its new copy, which was then destroyed. And in this regard, in the inspector of my engine, I have an asset that moved correctly (and correctly updated the data in its description file) was displayed as invalid since its relative path was old in GAssets. Generally difficult, but I tried to explain) – Kenny Aug 29 '22 at 21:33
  • And what is this? **Question eligible for bounty in 2 days** I never saw this before. – Kenny Aug 29 '22 at 21:35
  • @Kenny [What is a bounty?](https://stackoverflow.com/help/bounty) – The Dreams Wind Aug 29 '22 at 21:36

3 Answers3

1

Originally my idea was to propose returning a statically allocated object, like this:

const Asset& GetAssetByID(int ID) {
    ...
    static const Asset not_found{ -1 };
    return not_found;
}

However it seems to contradict signature of your function, because you apparently need a mutable reference. You may omit const qualifier here for both return type and the not_found variable, but since your ID member is also mutable, it's not really a good design (the client code may change the member variables of the returned value by mistake and never find it was an invalid asset in the first place, let alone this will break the logic for all future calls). So you should consider changing ID to const int if possible:

class Asset {
    const int ID = - 1;
    ...
}

Asset& GetAssetByID(int ID) {
    ...
    static Asset not_found{ -1 };
    return not_found;
}

If it's not an option, please take a look at the next proposal below.


A way more consistent option would be throwing an exception when no asset is found:

Asset& GetAssetByID(int ID) {
    ...
    std::ostringstream os;
    os << "Could not find an asset with ID: " << ID;
    throw std::invalid_argument{ os.str() };
}

The client code can use it like this:

int main() {
    try {
        auto& asset = GetAssetByID(20);
    } catch (std::invalid_argument error) {
        std::cerr << error.what() << std::endl;
    }
    return 0;
}
The Dreams Wind
  • 8,416
  • 2
  • 19
  • 49
  • That's not UB, it straight won't compile: https://godbolt.org/z/nso4vaMhK – Yksisarvinen Aug 29 '22 at 21:17
  • @TheDreamsWind This solution is pretty good for me because i never in my change the **InvalidAsset** or store it some where. I cannot use exceptions because then my engine just throw it, and i need to restart it. If asset not found i just not import it at all and continue importing. – Kenny Aug 29 '22 at 22:02
  • @TheDreamsWind I can use asserts, but only for debugging. – Kenny Aug 29 '22 at 22:05
  • @Kenny if you neither can make `ID` constant, nor use exceptions, then pointers is the way to go, I believe. – The Dreams Wind Aug 29 '22 at 22:08
1

Simply return a pointer instead of a reference, and then you can return nullptr when no asset is found.

Returning a pointer from GetAssetByID does not make any difference to how you delete the assets or not.

user253751
  • 57,427
  • 7
  • 48
  • 90
  • Yes i can do that, but as said early if i return by pointer i need always delete it but, in some cases i dont know where and when this asset need to be deleted. I tried use smart pointers but ended up with errors with DeclRef or something like i not remember. – Kenny Aug 29 '22 at 21:46
  • @Kenny No, returning by pointer does not mean you need to delete it. Why do you think that? – user253751 Aug 29 '22 at 21:47
  • Sorry, I initially misunderstood. Indeed, you can do this and just return **&asset**, and if not found, then nullptr. – Kenny Aug 29 '22 at 21:55
0

This code contains a dangling reference:

Asset& GetAssetByID(int ID) {
  for(Asset& asset : GAssets) {
     if(asset.ID == ID)
        return asset;
  }

  return Asset(-1); // How this would returns?
}

That is, you return a reference to an object on the stack that gets deallocated, which would be undefined behavior, so who knows what this returns? :)

I would suggest a different approach to your problem: instead of storing the asset's ID inside the Asset class, you can address your std::vector of Assets by the ID. In the client code you could then write:

std::vector<Asset> assets;
//...
try {
    auto& a = assets.at(id);
    // do something with the asset
} catch (std::exception const& ex) {
    // asset was not found
}
Ave Milia
  • 599
  • 4
  • 12