What's wrong with this code?

If you are so good, why not just make plugins and automated things in C++ and sell them on the Unreal Market Place, or even C++ code sniplets, you can sell those too, for various softwares, Unreal is just one of them where you can create C++ and sell in the form of plugins.

Further more you can find job projects, there are tons of freelancer sites where you get hired on project based work, with a deadline, finish the project in time and get your money.

They pay good money in Second Life to make Java Based objects for game that can do stuff, these are just a few.

I feel this is the future, projects, get money. Why do you feel the need to wake up at 6 AM to go to work.
If you are dying of food starvation then get a freaking job at MCDonalds.

Unreal Engine is so easy to use, that even the most technically illiterate people can use it

That is really really bad man, insulting people that way.

It was a compliment to Unreal Engine actually. How do you even considered it insulting to people who use it? If I said that only geniouses can use Unreal Engine, wouldn’t it be an insult to Unreal Engine then and its creators? The world consists of technically illitarate people, my Mom for example. Did I mean to insult her?

Oh, no. SpawnActor most definitely can return null (what would it return if the spawn fails?), and I guess that would be a point lacking in the documentation. You should bring that to Epic’s attention, I guess.

You’re both right and wrong – if you want the system to crash when a nullptr happens, then you don’t bother checking it. If you don’t want it to crash, then you check it. In fact, there’s a macro called check(), which you can use to not-crash but throw a warning on a failure in a build, but will crash instantly when running in debug mode. THAT is what you use when you are positive that there is no reason to expect nullptr but you don’t want a crash on the user side if it fails.

Now, you should check for nullptr in PrintArray, because although there’s no usage of PrintArray outside of immediately after the spawn function, if it were used anywhere after that, it would be subject to failure, because you have no idea what has happened to any of the actors in that array in the subsequent time following their spawning.

You should also check nullptr from the return of SpawnActor, because of course, SpawnActor can and will fail, which causes nullptr. So, then you likely want to bypass the array add.

On top of that, in a real world situation, you should never spawn actors like that, but of course this is just some kind of theoretical test, so there’s not really any way to determine a good method to draw from a pool or instance from the editor or what.

SO, what I see:

TArray should be UPROPERTY, PopulateArray should check null, PrintArray should check null and should have the array passed by reference not by value (and possibly const, although I don’t remember right off if that will generate a compile error converting non-const to const, i tend to always const anything that won’t be modified in the function, and change it if it blows up, because although i work with C++ every day now, until recently it had been several years, and prior to that, it had been 20 years, so i’m a bit rusty on some things)

No class name given in the psuedo code so you don’t know if this is inside an A* class or a U* class, so you might not have access to GetWorld() and I suppose there’s a possibility the even GetWorld() could return null, but I’d have to actually check on that.

well, you can specifically tell it to fail spawning (and i think it defaults to that, unless you tell it otherwise) if object would overlap another object, so that’s a pretty easy failure case that would need to be handled.

And your thing will depend entirely on the engine version. You can most certainly argue that documentation cannot be relied upon, but if you argue that source code also cannot be relied on, what the hell are you doing?

Utter tripe. If you have the source code, you use the source code. Hell, if I were testing someone, I would test their ability to find their way through the relevant source code. But I sure as ■■■■ wouldn’t be building a trick question like what you’re getting at here, whatever the hell it is.

I did an interview that had a code test that had a trick question like that, and the solution involved some obscure knowledge shortcut that was absolutely useless in real life, that involved building a cache of unlimited size, at which point I pointed out that the entire thing was a complete failure, because it would take far more time to allocate the memory to take the shortcut, than it would to actually run the algorithm. The interviewer (who worked for an extremely large company) told me to ■■■■ myself and hung up.

… and now i’ve scrolled to the point in your post where you have the answers.

Well, I got the lack of UPROPERTY lol
I also addressed that you shouldn’t spawn 100k actors at a time, however, if the actual test is “What’s wrong with this code?” then that’s not an error in the code, that’s an error in the design specs.

Yes, absolutely

I just dealt with a situation where my big bad guy was just absolutely disappearing from the world for no obvious reason. I traced it down to another class inside the game (that i didn’t know anything about) that was destroying any AI Character that found itself outside the bounds of the game, and the game bounds had not been extended to include this new area that was dug out of the map. So, you’re not going to get GC’d out of the level as the world has a reference to it, but it absolutely could be destroyed by the next time you go to look at it

Frankly, this test is not particularly useful, and they should ask questions that actually show that you understand the concepts, not just “What’s wrong with this code?” and expecting you to say “Well, it’s all a total failure because you shouldn’t run this code”.

If you’re claiming knowledge of Unreal, then yes, you probably should know that, even though, you correctly point out that nothing in the documentation specifically states that it may return nullptr. It’s safer to assume (unless you want it to crash on every failure) that anything that can return a pointer can also return null. On top of that, it’s possible anything that returns a pointer, could return a pointer that belongs to an actor that has been marked as being destroyed, but has not yet actually been destroyed, so you should use the IsValid() test moreso than the nullptr test, and usually when i say “nullptr check” i generally mean “IsValid()” since that tests both nullptr and if the item is in the process of being destroyed.

If you’re claiming knowledge of Unreal, knowing the basics of how to work with it should be a given.

While this test sucks, your attitude about what you should know going into it is also needing a check.

I’ve given an example of a well known function that can fail but never returns a null pointer - opeator new.

If a function can return a nullptr under the given conditions it must be checked, otherwise the code is broken.
The problem here is that if someone claims that not checking the pointer for null is a mistake, he must have a substantial reason for such statement. But there is no such reasons exist.

I have decent experience communicating with Epic since a had an advanced support account and they were obligated to communicate with me and help me. I can tell you for sure that they don’t need my or anyone’s suggestions or advice.

This is for debug purpose only and should never be used in a program’s logic. If a function can return a null pointer it shouldn’t be checked with ‘check’. ‘check’ basically sais “the expression must never be true, otherwise the program went into invalid state and must to be continued in debug mode and undefined behaviour in release”. If a function can return a null pointer, and it does, it is a valid state that must be correctly handled.

Yes! You are absolutely right with the statement that I have no idea what has happened to any of the actors in that array in subsequent time. But tell me how did you come to the conclusion that I must check the pointers for null? Since when do raw c++ pointers become null if the object they points to is destroyed? In the real world pointers to destroyed objects become invalid and a simple check for null wont give you a false value. That’s why there is no reason to check it for null once they were put into the array.

Did you check how UPROPERTY defined?
Well, let me save your time:

#define UPROPERTY(…)

Do you know what it means? It means you can write UPROPERTY wherever you want, because it does absolutely nothing. You could use UFUNCTION, USTRUCT or UMETA on you choice. They all do the same - nothing. Is is a keywords for UBT preprocessor, it can be used only within class definition scope in header files and never outside. If you’ll write it outside of class definition scope, you’ll get an error from UBT. Non-header files are not even getting processed so you can write UPROPERTY whereever you want and how many you want.
Even if it was a header files, even if it was a class scope, in the given case it will still do absolutely nothing. And I dont see a point to explain why anyfuther. There is an answer in this thread if you are curious.

:man_facepalming:

It makes me said, because it means there are a lot.

New operator absolutely can return null, although by default it does not – that default has changed over the years. If you specify your own allocator for a particular object, you do not have to throw an exception, in which case the allocation would fail with null.

Absolutely not. Crashing due to receiving a nullptr can absolutely be intended. You should probably use the check() macro for those situations, though.

I find this to be rather unlikely.

When you’re working with Unreal, since the beginning of time. Also, when you’re working with almost any pointer library that is more advanced than just using C++ without any management whatsoever.

You’re really showing with that entire statement, that you actually don’t know anything whatsoever about working with Unreal. Which leads to the doubt above about your claim that “Epic… were obligated to communicate with me and help me”.

… and in the context of using them inside an unreal header, they absolutely do have plenty of effect. You saying anything otherwise is absolutely useless, and again shows that while technically you do know what UBT is doing, you don’t have any idea how to work with the engine in code.

… You have an abyssmally awful attitude, believing yourself to be much more knowledgeable than you are.

This thread has derailed. Cmon guys chill out! :wink:

@ShiftZ and @eblade and @GigiK-K-K please make sure we’re being civil and respectful on the forums.

The link provided descripted the global operator new. It never returns, never returned and never will return null. It’s not a subject of change with years.
Custom allocator has nothing to do with the global operator new.

Wrong. Unreal Engine does not handle, never handled and never will handle raw c++ pointers. Or any of existed libraries. You can make sure yourself in three lines of code.

Library of raw pointers? Do you even know what “raw pointer” means?

Such a bold statement. Are you sure you weren’t in a hurry? There is a great chance of being in an awkward situation.

First of all, there is no given context in the test task. That alone is enough, but there is more.
UPROPERTY does not have plenty of effect in the header context . You can open a header file and write:

UPROPERTY()
TArray<AActor*> ActorsToPrint;

And enjoy the side effect as a plenty of errors.

UPROPERTY has effect only inside a class definition. It has been clearly said in unreal docs: Properties | Unreal Engine Documentation

Reference for creating and implementing properties for gameplay classes

It sais “gameplay classes”. Not “gameplay headers”. There is no other uses outside classes and structures.

Now, let’s check the initial code again:

TArray<AActor*> ActorsToPrint;

How many gameplay classes do you see? One? Two? Plenty?
There is no classes. If you put UPROPERTY() here you’ll get an error:

error : ‘Member variable declaration’ is not allowed before the Class definition

I’ll grant my abysmally awful attitude to anyone who says that I don’t understand how the simplest things work, while at the same time being incapable of substantiating the claims with technically correct arguments. Let me consider “you actually don’t know anything” and “believing yourself to be much more knowledgeable than you are” as not technically correct arguments. Would you?

I’ll grant my grateful benevolence to anyone who will show me that my arguments are technically incorrect.

Do forum rules forbid me doing that?

Very likely. I get confused about it sometimes.

By default operator new throws exception, but you can use overload (std::nothrow) which return nullptr on fail.

You’re absolutely right, but this is true for pure C++.
Epic had the ridiculous idea to add GC to cpp. And thanks to this, the ubiquitous checking of nullptr, the lack of information about who owns the resource, behind-the-scenes nulling, etc.

You’re absolutely right, but this is true for pure C++.
Epic had the ridiculous idea to add GC to cpp. And thanks to this, the ubiquitous checking of nullptr, the lack of information about who owns the resource, behind-the-scenes nulling, etc.

I think it’s a good idea and even mandatory to have GC. Wouldn’t it be a nightmare to manage pointers and objects lifetimes? That would be completely unmanageable in a complex software.

From the epic documentantion:
When an AActor or UActorComponent is destroyed or otherwise removed from play, all references to it that are visible to the reflection system (UProperty pointers and pointers stored in Unreal Engine container classes such as TArray ) are automatically nulled. This is beneficial in that it prevents dangling pointers from persisting and causing trouble down the road, but it also means that AActor and UActorComponent pointers can become null if some other piece of code destroys them

Unreal Object Handling | Unreal Engine Documentation

So it’s possible that the pointers stored in the array become NULL by magic and a check for NULL should be made.

To create UActorComponent you need AActor
To create AActor you need UWorld
UWorld is created by UGameInstance
UGameInstance is created by UEngine

This clearly implies who owns and who is responsible for the lifetime of the object.
If you need to share a given resource then you use shared_ptr,
If it is exclusive to a given object then unique_ptr.
If you are just observing (without transferring ownership) then weak_ptr. And only weak_ptr really needs to be checked for null.

GC completely ruins this logical, widely used approach.
And more. This leads to four different pointer types inside one project:

  • “pure” → user defined types, FSomething, etc. (new/delete)
  • smart → ex. Slate (TSharedPtr/TWeakPtr)
  • managed → UObject derivatives, (requires reflection macros)
  • engine-owned → AActor and UActorCompontent derivatives

And worst of all, three of them are identical = type*.

Unreal Engine is the only engine I know with GC. Nobody in his right mind will introduce GC to a C++ engine.
GC solves kind of problems that were relevant 15 years ago. C++11 introduced standart smart pointers implementation that solve all the problems GC solves. But GC in Unreal Engine was introduced before C++11 implementation become widely available on Windows platform.
Now GC is the problem, but it can not be undone. It’s a curse, not bless.

Yes, they can be nullified. But in the test code there is no visible pointers to the reflection system. Simply adding UPORPERTY() on top of TArray declaration in the global scope doesn’t make it magically visible to a reflection system. It works only with reflected objects of properly declared classes.

Now that one deserves my grateful respectful attitude. :ok_hand:
I couldn’t have said better.
I can only add that GC ruins RAII idiom. If one doesn’t know what it is or does not respect it, he doesn’t know the first thing about software engineering on C++.

Absolutely none of this is practical or useful – Again, I’ve worked on a whole lot of large commercial C++ products, and not a one of them allows usage of the stdlib, including the standard pointer libraries. All of them use smart pointer systems that coincidentally provide for automatic allocation and deallocation – garbage collection.

You are completely missing the point of the given test, you are being willfully bullheaded about you being correct about everything you’re saying.

Unreal doesn’t work in the way you describe. At all.

Garbage collection does not ruin RAII, and if you read the wiki page on RAII, you’ll see that reference counting collection is absolutely part of RAII implementation.

You keep throwing around a lot of smart words, but none of them are reality.

If you test on allocation, and do not add NULL pointers to the array, you do not need to test on use. In fact, testing on use may mask logic bugs elsewhere in your program. You should establish invariants that are as restrictive as possible (e g, “this is never NULL”) and then write your code assuming those variants, to make code that is as clean and well performing and easy to use as possible. If you absolutely need additional belt and suspenders, slap an ASSERT() in there for documentation and debug purposes.

Belt-and-suspenders, try-to-soldier-on code just defers bugs until later, when they are much harder to track down, and much more costly to fix. Establish “only works on correctness” rules, establish what “correctness” is, and use ASSERT() to enforce entry into the system in debug mode. Catch runtime failures right when they can first happen (calling D3D, loading a file, etc) and handle them (or give up) before the fallibility can spread in to the rest of the code.

If SpawnActor() can fail, then handle that right at the call, and then no other part of your code needs to worry about it. Maybe this means you just ignore the null entity returned and don’t add it to the array. But, more likely, this is actually a follow-on error from somewhere else (memory leak, sub-spec system, missing runtime class, …) and thus you would generate corrupted save files if you let the code go on, so testing and failing the entire level/game is probably the right outcome.

That depends on what you’re testing. If you’re testing “are these people familiar with working with the UE4 engine, where looking things up in source needs to be done at times” then that is a reasonable thing to do. Hopefully we’re testing the things people will actually be doing at work, not some adjacent vaguely-related trivia. (Also: all tests should allow unlimited Googling. I always tell candidates this when I interview them.)

Or the job requires someone already familiar with “mobile development” and “unreal engine development,” because they are not looking to train someone up for this position. Not every programmer is a match for every job/role. That’s totally OK. If you think these people were totally unreasonable, remember that the interview goes both ways – if you don’t want to work with unreasonable people, you dodged a bullet here, managing to avoid that trap!

It’s pretty unreasonable, if we’re being given all the details, to ask “What is wrong with this code?” when you give what is effectively psuedo-code, and you’re looking for both “a ‘keyword’ is missing” and “this is completely wrong, you shouldn’t ever do this”.

I doubt we’re being given all the details, though. ShiftZ seems to be here seeking validation that they are right, the test is wrong. The test is wrong, but so is ShiftZ, IMO.

A better test is simply asking “What do you do if you need to spawn a large quantity of objects at once?” … and there are different ways of addressing that – you don’t spawn them all at once, you pull them from a pool, you do it over several ticks, or if you absolutely must do it, you do it at startup behind a load screen.

This peace of text is absolutely irrelevent to the whole topic. Why did you say that? I’m happy to hear of course that you’ve used a proprietary smart pointer library instead of the standart one. I’m sure it’s way better. But how it’s relevant to anything?

I am eager to hear how it actually works from the person who writes irrelevant text.

Reference counting collectction? There was no a single word about reference counting collection. Unreal Engine GC is not based on reference counting:

Unreal implements a garbage collection scheme whereby UObjects that are no longer referenced or have been explicitly flagged for destruction will be cleaned up at regular intervals.

Is it all the same to you?
And yes, it does break RAII, because RAII is based on the determined object life time. Unreal Engine GC does not delive that.

Since you seem to be here only to validate that you’re right that this test sucks, and insult anyone who points out where you are wrong, I’m going to say it again – you’re right, this test sucks.

And I’m not going to say anything more. Good luck in your future endeavours.

:ok_hand: These should’ve been my words. Most poeple would not even see the amount of experience that is required to claim that simple thing.

You have reaaons in your words. But my case definitely doesn’t fall under this explanation.

I am familiar with both, but I’m not familiar with the document. Why would I? Each company uses their own styling guide. I can write in any style if it was described on a paper, even if I find it silly. It’s a matter of 30-60 minutes of reading. It can not be a subject of testing.

Of course. But it’s not the only case. I’ve got a lot of silly feedbacks. I wouldn’t pay attention if it was a single case, but it’s a widespread practice. Technical literacy become a joke to some people in Unreal Engine environment. And the reason why is clear to me.