phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Use common prefix for high-R/high-V circuit elements #936

Closed arouinfar closed 1 year ago

arouinfar commented 1 year ago

Related to #917

We currently use highResistance and highVoltage as prefixes for associated resistors, bulbs, and batteries. In addition to being verbose, these prefixes aren't always accurate. A standard bulb (max R = 120 Ω) can have a larger resistance than a high-R bulb (min R = 100 Ω). A common prefix would simplify the language and result in these elements being grouped together in the tree.

In discussions with @matthew-blackman and @samreid, we tentatively liked "extreme". @samreid please proceed with this change. If you think of any alternatives to "extreme" please let me know. I think it evokes the right idea, but there may be a better term we haven't thought of yet.

samreid commented 1 year ago

It seems anyone could help on this. We want to rename the any tandems beginning with highResistance* or highVoltage* with extreme*. Internal code details (variable names, methods, parameter names, etc should be updated as well). Maybe even class names? Not sure.

samreid commented 1 year ago

This looks good to me, nice work. I'm wondering if we will want to change extreme to isExtreme for the options parameters and class attributes. I was about to recommend that, when I saw that we already have LightBulb.real instead of LightBulb.isReal, but now I'm feeling we should change both. But I looked in the code review checklist, and did not see a convention for it. @matthew-blackman what do you recommend?

matthew-blackman commented 1 year ago

I like the prefix 'is' as a style pattern for boolean options. Will update 'extreme' to 'isExtreme' and 'real' to 'isReal'.

samreid commented 1 year ago

Excellent, looks great, thanks, closing.

samreid commented 1 year ago

I also noticed a constant that looks like it should be renamed to match:

const isExtreme = selectedCircuitElement.resistorType === ResistorType.HIGH_RESISTANCE_RESISTOR;

Perhaps rename ResistorType.HIGH_RESISTANCE_RESISTOR => ResistorType.EXTREME_RESISTOR?

samreid commented 1 year ago

Fixed, closing.