Where is the redundancy?
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
I think calling a method thrее times is redundant. But then again, you have to balance. Today’s article is about a code review that at the end took like a few hours in total of different discussions and I believe it is important. This kind of things take time. Failed builds, difficult specs.
The IRL example – Where is the redundancy? Is this DRY?
@dataclass(frozen=True)
class LdrawLine(abc.ABC):
+ default_x: ClassVar[float] = 23
+ default_y: ClassVar[float] = 45
+ default_z: ClassVar[float] = 0
"""
This is the abstract class which every LdrawLine should implement.
"""
@@ -85,11 +88,19 @@ class LdrawLine(abc.ABC):
elif line_args[1] == "STEP":
line_param = _Step()
elif line_args[1] == "ROTSTEP":
- rostep_params = {
- "rot_x": float(line_args[2]),
- "rot_y": float(line_args[3]),
- "rot_z": float(line_args[4])
- }
+ if line_args[2] == "END":
+ rostep_params = {
+ "rot_x": LdrawLine.default_x,
+ "rot_y": LdrawLine.default_y,
+ "rot_z": LdrawLine.default_z,
+ "type": "ABS"
+ }
+ else:
+ rostep_params = {
+ "rot_x": float(line_args[2]),
+ "rot_y": float(line_args[3]),
+ "rot_z": float(line_args[4])
+ }
This piece of code (along with a few other changes in the commit) were the root of a 2 hours discussion in the team. A spec failed because some things were int while we were expecting them to be float.
Calling ‘float’ three times like this is redundant
The reason I think like this is that if you have to change and would like to have a double value or an int value you would have to change the code in three places.
Probably e better solution would be:
+ if line_args[2] == "END":
+ rostep_params = {
+ "rot_x": LdrawLine.default_x,
+ "rot_y": LdrawLine.default_y,
+ "rot_z": LdrawLine.default_z,
+ "type": "ABS"
+ }
+ else:
+ rostep_params = {
+ "rot_x": line_args[2],
+ "rot_y": line_args[3],
+ "rot_z": line_args[4]
+ }
# We are adding a loop to call the float
+ for key in ["rot_x", "rot_y", "rot_z"]:
+ rotstep_params[key] = float(rotstep_params[key])
We call float only once at a single place. Now we have to deal with the fact that we have “rot_x” as a variable in two places, yes, that is true, but this could easily be extracted and we can iterate over the rostep_params values. But now we have consistency.
Is it harder to read? Probably it is a little harder. Instead of simple statements you now have a loop. So you lose something, but you gain something. A float function that is called at a single place.
What are we doing with these rotsteps?
ROTSTEP is a command in the LDR format. We support LDR for 3D building instructions. Here is one example with a FabBrix Monster that uses the LDR ROTSTEP as a command:
FabBRIX Monsters, Cthulhu in 3D building instructions
Reply
You must be logged in to post a comment.