Please critique my code

Hi everyone, I made a sketch to use a potentiometer to turn on an led at about halfway on the potentiometer. My first attempt to write code without looking at any references so feel free to critique away. All info if good info.

//This is a sketch to turn on an led attached to in 13
//once a potentiometer gets to a certain level


const int ledPin = 13;      //led attached to pin 13
const int potPin = A0;      //potentimeeter attahed to pin A0

void setup()
{
  pinMode(ledPin,OUTPUT);   //led is an output
  pinMode(potPin,INPUT);     //potentiometer is an input
  Serial.begin(9600);            //start serial monitor
}

void loop()
{
  digitalWrite(ledPin,LOW);                               //start with led pin off
  int potRead = analogRead(potPin);                 //read input from potentiometer
  int potValue = map(potRead,0,1023,0,255);   //convert input from potentiometer value between 0 and 255
    if(potValue >= 125){                         //if potentiometer gets turned to a certain lever turn on the led
      digitalWrite(ledPin,HIGH);
    }
    Serial.print(potValue);                       //print the value of the potentiometer
}
//This is a sketch to turn on an led attached to in 13
//once a potentiometer gets to a certain level

const byte ledPin = 13;      //led attached to pin 13
const byte potPin = A0;      //potentiometer attached to pin A0

void setup()
{
  pinMode(ledPin,OUTPUT);   //led is an output
  Serial.begin(9600);            // initialise serial port
}

void loop()
{
  int potRead = analogRead(potPin);       //read input from potentiometer
  int potValue = potRead / 4;                  //convert input from potentiometer value between 0 and 255
  digitalWrite(ledPin, potValue >= 125 ? HIGH : LOW);
  Serial.println(potValue);                       //print the value of the potentiometer on a new line
}

I have not compiled nor run this code.

Thanks, I like this line:

int potValue = potRead / 4;

When. the pot has a value of >125, the LED will be turn off and then on again - you won’t get the full brightness. The brightness will depend on how much time your code takes to run between when it is turned off and when it is turned on. This will not be insignificant, because the map() involves doing an integer division.

There’s no reason not to write to the pin each loop unconditionally. It boils down to a write to a memory location (a port), which is pretty fast.

void loop()
{
  int potRead = analogRead(potPin);                 //read input from potentiometer
  int potValue = map(potRead,0,1023,0,255);   //convert input from potentiometer value between 0 and 255
  digitalWrite(ledPin, potValue >= 125 ? HIGH : LOW);
  Serial.print(potValue);                       //print the value of the potentiometer
}

You can avoid having to do the map by comparing against the raw value rather than rescaling the pot value and comparing to a rescaled value. This also means that you keep all the precision in the raw value.

void loop()
{
  int potRead = analogRead(potPin);                 //read input from potentiometer
  digitalWrite(ledPin, potRead >= 125*4 ? HIGH : LOW);
  Serial.print(potRead);                       //print the raw value of the potentiometer
}

Avoid magic numbers in your code. Use const variables with meaningful names. The compiler will optimise it down to the same thing as putting the number directly in your code.

const int ON_THRESHOLD = 125 * 4; // *4 because analog inputs are 0-1023

void loop()
{
  int potRead = analogRead(potPin);                 //read input from potentiometer
  digitalWrite(ledPin, potRead >= ON_THRESHOLD ? HIGH : LOW);
  Serial.print(potRead);                       //print the raw value of the potentiometer 
}

But try to stick to a single style of variable names. A pretty common way is: thisForVariables AndThisForConstVariables ALL_CAP_FOR_MACROS

I have few questions and queries: 1. Is ledPin a symbolic name for DPin-13 or ledPin is a variable to which you are assigning a value of 13?

2. analogRead(); always returns a positive value; there is no scope for it to be negative. If so, why have you declared the variable as int potRead; rather than unsigned int potRead;?

3. You have said that the the LED will become ON when the pot-wiper arrives at its mid-point which is about 1023/2 = 511. You know this value; then, why have you transformed the pot-range ( 0 to 1023) into (0 to 255) using map() function?

4. In the comment you have said that LED is an output. Is it an output line or an output device?

5. Literate Programming Style says that the brace ({) should always be at the new line position and alone. Why have you placed it at the same line of the control structure -- if(potValue >= 125){ ?

We appreciate your courage for facing questions!

3) If you want to calculate it exact: 512 ;)

5) Because that's the default style of Arduino ;) And you can argue, but I like that more as well.

But hey, as long as somebody is consistent with it's styles (variable names, brackets and space use etc) I'm fine with that.

And you can argue, but I like that more as well.

Absolutely, no argue. The salute is always there! :)