Mechanical-Advantage/AdvantageKit

LogTable.get() is a bit confussing

Closed this issue · 2 comments

Hi! I got this bug in my code:

@Override
public void fromLog(LogTable table) {
  encoderVelocity = table.get("encoderVelocity", 0.0); // works because 0.0 is double
  encoderVelocity = table.get("encoderVelocity", 0); // does not work because 0 is int
}

So, just a minor suggestion, LogTable.getDouble(String key) might be a little more straightforward than LogTable.get(String key, double defaultValue). Because for LogTable.get(String key, double defaultValue, you might accidentally use the wrong type of default value.

Most of our examples show using the previous value of the field as a default value, in which case the generic get simplifies the need to keep track of types. It seems like this is mostly an issue for types which might otherwise cast to each other (i.e. int and double), though I would argue it's generally good practice to explicitly declare those values as the intended type anyway.

I think this might be reasonable to include if we switch to source templates for generating methods, otherwise it becomes a maintenance burden to maintain both the normal get and explicit getX.

Right, thanks for your help!