What's wrong with this code?

Development Programming & Scripting C++
1

I was recently given a test task for a job in one of mobile gamedev company. I finished it and uploaded. Boring, you say? Yes. But the feedback was NOT boring at all. I couldn’t belive my eyes. But first, try answer this basic question. What’s wrong with this code:

TArray<AActor*> MyActors;

void PopulateArray(int n)
{
    for(int i = 0; i < n; ++i)
    {
        AActor* ActorToAdd = GetWorld()->SpawnActor<AActor>();
        MyActors.Add(ActorToAdd);
    }
}

void PrintArray(TArray<AActor*> Array)
{
    for(auto Actor : Array)
    {
        UE_LOG(LogTemp, Warning, TEXT("Actor's name is: %s"), *Actor->GetName());
    }
}

void DoArrayWork()
{
    PopulateArray(100000);
    PrintArray(MyActors);
}

Any guesses?

2

  1. We don’t know the context. Objects in the MyActors array can be destroyed by the GC at any time.

  2. The array is unnecessarily passed to the PrintArray function by copy. If the function does not modify the object, it is best to pass it by a const-ref (TArray<AActor*> const&).

    • depends
3

Object in the ‘MyActors’ array can not be destroyed at any time. Spawned actors are possessed by the world.

Do you agree?

4

I agree with Emaer, without knowing the context it is hard to tell, but also, whenever a function return a pointer it is necessary to test if it is nullptr before using it, in this case you might be adding a nullptr to the array and when printing its name it will receive a nullptr when calling GetName() which will crash the code. Always test the pointer before its use. Another thing is when that code is used, because depending when, you might not have a World created already, so it will produce a nullptr at GetWorld(), and this will also crash.

5

Well, I’m guessing you know where the problem is and you are testing us.
Ignoring the lack of null pointer tests, I can’t see nothing wrong with the code…
You got me.
Nothing wrong that makes this code crash I mean.
Edit: well, not sure what happens with that array passed by value. It will copy the pointers? Not sure…

6

Why?
No, it is not necessary. There is no such rule. Any function has a specification what kind of values it can return. If it explicitly says that null can be returned among other values, than you must test it for null, otherwise there is no reason to do that.
For example:
https://en.cppreference.com/w/cpp/io/c/fopen
“On error, returns a null pointer.”

https://en.cppreference.com/w/cpp/io/c/tmpnam
“If no suitable filename can be generated, a null pointer is returned.”

Check this out:
https://en.cppreference.com/w/cpp/memory/new/operator_new
“Return value:
1-4) non-null pointer to suitably aligned memory”
Note that 1-4 overloads of operator new can not return null pointer ever. It can fail, but it will not return a null pointer.

“5-8) non-null pointer to suitably aligned memory of size at least size , or null pointer on allocation failure”
And in case of 5-8 overloads it returns null pointer on failure. It explicitely sais so.

Now let’s check SpawnActor documentation:

Not a word regarding returning a null value.

Let’s check another place:

“This function creates a new instance of a specified class and returns a pointer to the newly created Actor.”
Again, nothing regarding returning a null value.

I could not found any documentation or official guide that explicetely sais that SpawnActor can return a null pointer. It doesn’t necessarily mean it can not, because Unreal Engine has quite poor documentation. But you can not design your code assuming that any function returning a pointer can return a null value. Your code will be full of unnecessary checks. You can examine Epic’s templates. They never checks returned pointers for null, unless there is an obvious reason.

Anyway, this is a test task for a programmer. A good programmer must follow documentation or official guides, not his best guess, rumors or someone’s complain that SpawnActor returned a null. Right?

7

I know where the problem is, and the answer is trivial.

No no no. We haven’t finished yet with null pointers tests.

8

There would be something really wrong if SpawnActor could return a null value me thinks…

9

Unfortunately official documentation won’t always contain all the dirty details. UWorld::SpawnActor for example has numerous paths that lead to return NULL; although they are largely to handle error cases. One could argue that it’d be ok to assume the result of a function is a valid pointer given that it would only be called with arguments that wouldn’t end up returning a null pointer. However assumptions like that make code much harder to maintain. Anyone that modifies your code is going to ask themselfes why the pointer isn’t checked. And any modification to SpawnActor will require you to verify that the assumption still holds. You’d have to do this anytime you change engine versions and eventually nobody is going to remember at which point it’s going to crash and customers won’t be happy.

Regarding the non-UPROPERTY MyActors array: Regardless of whether the world holds on to the Actors to prevent them from being garbage collected, someone may use your code and decide they need to delete one of these actors later. Since the engine isn’t aware of the MyActors array it won’t null out the pointer leaving it dangling.

Luckily we don’t need to guess, we can directly look at the official Unreal Engine source code.

10

Checking the UE4 source code, there are paths which will make it return nullptr. The documentation is not perfect. The amount of time expected for testing a pointer is not important because even if you do it 1000x it will cost 0.01ms (negligible), if a code is on the rendering pipeline it will produce impact, but not in gameplay.
Assumptions are bad, you might produce a code that you can test things while in development and remove those portions of code (if timing is an issue) if you are shipping the code, with just a bunch of compilation macro directives you can add on your own.

1 Like
11

Source code changes with engine version, you can never rely on source code.
There is basicaly no relible way to know for sure. It doesn’t exist.
So, there is only one possible reason why would you check a pointer for zero:
one’s guess that SpawnActor can return a null pointer.
Now let’s get back to the test task.
Someone wrote this AActor* ActorToAdd = GetWorld()->SpawnActor(); and didn’t check the pointer for zero, and I’ve been asked what’s wrong with this code.
So, I can assume that someone who wrote this guessed that this exact overload can never return a null pointer, because he can not see a reason why would it. Not enough memory? The program must be shut down immediately then. If memory is sufficiant, then there could be a limit to the number of Actors then? Let’s google it… No, there is no such limit found.
So, why would I think that such guess has no right to exist? How can I state that this code requires a null check? I have no arguments to substantiate this. My own guess is not an argument. I must assume this code is ‘ok’. Am I wrong?

12

It doesn’t matter whether it returns or not. A test task can NOT be written so the participant must check the source code of anything to give the right answer. Moreover, if you say something like “you know, i’ve checked the source code of std::operator new and found out that…”, it will be a good reason to reject your candidacy right away.

13

You showed us the code, ok. You didn’t give us the question/task asked to you. We can only be working on guesses here. Quite unfair imho.

As we don’t know the premises, but only the code, the only things that would produce a bad response to that code are:

  • lack of pointer checks: which includes GetWorld() because we don’t know the context where DoArrayWork() will be called and a valid World might not exist;
  • you assume the array inside PrintArray has valid elements and are not checking if the element there is valid, so *Actor->GetName() would fail. Anyone which would use PrintArray() might not know that the array elements were inserted using SpawnActor() in first place, meaning you are using an assumption, which might be invalid.
  • it would be a better code if you receive the World pointer as parameter to know where to create actors at;
  • array passed by value, surely is a performance issue there, it could be a reference or a const value;

Those from all everyone posted here up to now.

PS: I saw that after this post you fixed the question

14

I did. The task was “What’s wrong with this code”. I’ve just realized that this is not obvious that it was exactly the task. So, now it’s clear.

15

Oh really? heh

16

Yes, my bad.

17

Any potential future employer has every right to ensure their future employees are able to read and research an extensive existing code base such as Unreal Engine, especially if Unreal Engine was part of the job description.

18

Ok, my answer was this:

  • GetWorld() refers nowhere
  • Unnecessary copy by value in PrintArray function. It will trigger a copy of quite a big array.
    I didn’t include pointer check because I cant substantiate it.

Ok, now the funny part. I’ve received this feedback:

  • the candidate missed out on the main issue, which is the lack of UPROPERTY() macro. A couple of mistakes were not pointed out as well (spawn of 100k actors at the same time, potential nullptr Actor, const keywords)

UPROPERTY macro missed… It was the main issue…
Nobody here is quilified enough to work in this beautiful company.

Check out the other mistakes: “spawning 100k actors.”
The author of the code must have been told to spawn 99 999 actors instead. But he spawned 100k. Obvious mistake, I feel ashamed, how could I miss it.

“const keywords” - ¯_(ツ)_/¯

I requrested a more detailed feedback of course, and check this out:

1) ”spawn of 100k actors at the same time"
This function was not addressed as expected. UE4 does not have limitations when it comes to the amount of actors that spawn. However, this outcome would have a significant impact on the device’s resources. It is always important to evaluate whether your solutions will have an impact on the performance of the game when working with any given device’s limitations.

So, if you do something that have a significant impact on the device’s resources - it’s a mistake. Remember this, guyes.

2) ”potential nullptr Actor"

spawnActor returns a pointer that can be collected by GC at any moment if it is not secured properly. It is always a good idea to check actor’s pointer validity. Actor could get destroyed between the moment we spawn it and we print the array.

*spawnActor returns a pointer that can be collected by GC

So, you can spawn a monster that attacks the player and disappears 2 meters away because it was GCed. Everyday’s situation I must say.

that can be collected by GC at any moment

Can you imagine that? At any moment. GC must be running in a parallel thread, so the object could be GCed even before you assigned the pointer to anything. It can be GCed even after you have checked it existed, so you have to double check it, but better tripple check, just for sure.

It is always a good idea to check actor’s pointer validity.

Yeah. It’s good idea to check if an object was GCed or not by checking a raw pointer. Good suggestion. I’ll note that.

Actor could get destroyed between the moment we spawn it and we print the array.

Oh my god! It can be destroyed even before SpawnActor() exit!

  1. ”const keywords"

This is an example of an area of improvement according to the code standard that was shared in the description of the technical test. Here are some examples of where this could have been considered: (doesn’t matter)

Yeah, it was shared indeed. Coding Standard | Unreal Engine Documentation
So, I was supposed to figure out that the given example must follow the linked coding standart, and read a thousand lines document within 15 minutes that was given for the task.
I admit, I failed.

  1. ”UPROPERTY() macro"

Usually we consider code example as a UOBJECT and use reflection macroses properly, however, your example requires a few improvements to avoid misinterpretation.

What? My example? What my example? What’s an UBOJECT? You consider code example as a UOBJECT? How cares what you consider? My example requires a few imrovements? So, your code example does not require a few improvements and it can not be misinterpreted?
I can not understand. I need a translator from Indian to English.

Now, I share it because I found it not just stupid, but ridiculously stupid.

1 Like
19

The UPROPERTY is not needed as we discussed earlier.

100k actors… I agree it is weird, even that being a issue, because the program will slow down, but even thou… as far as someone can tell, they do want to spawn 100k.

The question which really pops into my mind is: What they want to test in regards to anyone’s ability?

Does this test proof anything at all?
Does this test guarantee the candidate won’t produce code which will blow at the customer’s side?

both answers are NO and NO, no need to think much… .haha

20

I’ve been looking for a new job for a half a year now, and I can make the following conslusion:
Unreal Engine is so easy to use, that even the most technically illiterate people can use it. I’ve been doing games on c++ for 15 years now, and I can not find a new job because the interviewers level is far below a reasonable level. They will never let me pass.
Each feedback I recieve just blows my mind.