I ended up refactoring GetReachLine into one function rather than two without using a struct or out parameters.
What I did was create an enum (at first I was going to use bool but figured an enum was more readable) and used that as the input parameter to the function, and then returned the correct position depending on which enum value was past in.
If you are reading this: let me know if you think its a good way to code this kind of thing, and please leave any constructive criticism
It might seem like a good idea at the start to combine several functions into one, but there’s a big reason NOT to do it whenever you can help it, and that is - clarity. In a project of such small scale like this (at least in the beginning), you might not immediately see the benefit of keeping functions separate and single purposed, but when you work in huge code base (and video game project is gonna become a massive one eventually), it helps a lot when functions are clearly named and they only serve single purpose. It’s much easier to understand the code for the programmer, especially someone who didn’t write that code (if there’s multiple people working on the project).
One other thing, if you put “// else -> Location == EReeachLineLocation::END” comment like that, you’ve already spent space and time writing that line, just use it as an actual code, the comment is redundant and it’s much more clear what value you’re expecting to run that block of code. Don’t assume the state of a parameter, check it. Another big reason to always check your parameters (especially enums!) is because later on you might want to add another value to the enum, and then that “else { do this }” will run on more than one enum value! If you always check the state of the parameter, it will save you headaches and time wasted debugging in future!
It’s all about learning the right programming habits for the future, even if either way works fine at the moment.
I personally would prefer to have a function that returns a struct with End and Start positions inside it, as one of the ways suggested in the lesson, if we’re talking about combining functionality.
First of all: cheers for taking the time to write up your reply. I really appreciate it! I’ve had a couple years experience with Java, but a lot of what has been taught to me has come without advice like this! (So thanks).
I can see how someone else looking at this function would be more confused than if It had been left as two functions. As soon as I made the change I felt it needed comments as beforehand it was self explanatory.
Ah I didn’t even think about that! That makes a lot of sense, definitively something I’ll remember for future reference.
So you think the use of a struct would be a good alternative - no repeated code and a bit more clarity?
“As soon as I made the change I felt it needed comments as beforehand
it was self explanatory.”
That’s exactly it. If it makes you say that, then, if possible, you should
find another way to accomplish your original goal (in a clearer way). You
should always go with “self explanatory” if it doesn’t incur additional costs.
And the latter is also really important (the cost bit)
“So you think the use of a struct would be a good alternative - no
repeated code and a bit more clarity?”
Yeah, either return a struct, or make a function with two out parameters.
TBH, i don’t mind out parameters as much as others do. Seems like the
lecturer really dislikes them, and i can see that they can be a bit too
confusing and easy to miss, but as long as you label them properly in teh
function parameters (e.g. “float& OutPlayerPosition, float&
OutLineEndPosition”), i think it’s fine.
I think this is a good point to bring up that you need to keep in mind the
cost/efficiency of your code as well. In commercial game development cost
and efficiency always comes first, only then clarity and ease of
understanding. I don’t know how experienced you are with the programming,
and if not a lot, then you shouldn’t worry about this right now at
all, but I’ll mention it just for the sake of argument. So for example, if you combine
two (or more) functions into one that returns a struct that has bunch of
different values, but if every time you call the function you only need one
of those or two (or simply, if you dont need everything it returns every
time you call it), then you’re wasting space/memory/processing time and
power by creating the struct with bunch of values that you won’t need. In
your original example, you would need to call your function twice in a row,
once to get teh player position, and then again to get end line position.
Each function call will perform
"GetLocalPlayerController()->GetViewPointPsitionAndRotation(pos,rot)" each,
when you actually could just call it once, use the values it returns once
to calculate what you need and then return both at the same time (either in
a struct or out parameters).
In this particular example the extra cost is completely negligible, but it
illustrates the problem or a general idea, of where the cost goes, and it
could get really costly really fast when you start dealing with huge
objects that store a lot of things, or performance intensive functions
(bunch of loops done multiple times, etc.). Like i said, if you’re a
beginner, you shouldn’t worry about such things, but thought I’d explain it
if you’re interested in this kind of thing.
I agree. A function shouldn’t dotwo things… However. A method called GetReachLine would clearly state that it is getting both edges of the line. And since we have to work with this void getter method that changes values of its two parameters that the developers gave us, a method that works with two value seems to make sense.
Still the argument of clarity is a valid one.
However there are two arguments that weigh against Ben’s two methods.
First, they repeat code, it’s not DRY.
Second, that solution calls the same method twice in a tick, instead of once. the infamous GetPlayerViewPoint
What’s more, that method update two variables twice.
Since it was a refactoring lesson, I did some and my solution was to create two member variables in the header file. ReachLineStart and ReachLineEnd and call a setter. For people who are new to programming it’s a void method that changes the value of a member variable. I could have called two setters but that would have forced me to call GetPlayerViewPoint twice, which sounds absurd to me. That way those two variables are available in every method of the UGrabber class and are only updated once, when setter is called by tick method, which makes sense to me. The only drawback is that the two variables are available even when player is not grabbing anything but only updated when grabbing.
