first complete program critique

Hello Everyone. This is my first program and I would love criticism, help, suggestions, etc. It uses an Uno to control a robot arm with a joystick.

/* This program uses an Arduino Uno at compares operator input from a potentiometer joystick to control  the position of air cylinders via PWM proportional valves with
 *  a positive feedback loop from potentiometers on the air cylinders.

int aft=0;
int port=0;
int diff=0;
int diff1=0;
int wheel=0;
int brake=0;
int throttle=0;
unsigned long previousMillis = 0;

void setup() {

  pinMode(4,INPUT); //estop switch
  pinMode(5, OUTPUT);//PWM Valve#1
  pinMode(6, OUTPUT);//PWM Valve#2

  pinMode(10, OUTPUT);//Switch#1
  pinMode(11, OUTPUT);//Switch#2
  pinMode(12, OUTPUT);//Switch#3
  pinMode(13, OUTPUT);//Switch#4

  //these are not needed only for reference
  // pinMode(A0,INPUT);//aft cylinder potentiometer Input
  //pinMode(A1,INPUT); //port cylinder potentiometer Input
  //pinMode(A2,INPUT); /throttle joystick control input
  //pinMode(A3,INPUT);//brake joystick control input
  //pinMode(A4,INPUT);// wheel joystick control input

   digitalWrite(10,HIGH);//High Turns off 4 chananel relays - LOW to turn on


void loop() {

      digitalWrite(10,HIGH);//High Turns off 4 chananel relays - LOW to turn on
      digitalWrite(13,HIGH);}//if switch is low do nothing

   aft =analogRead(A0); //read first analog input potentiometer twice to settle DAC
   port =analogRead(A1);//potentiometer
   wheel=analogRead(A4); //potentiometer

/*wheel pot goes 0-850
 * brake joystick 728-849
 * gas joystick 700-850
 * aft pot pneumatic cylinder 300-900
 * port pot penumatic cylinder 389-975
    port=constrain(port,389,975);//needed to constrain actual max & min numbers come from the pots




 unsigned long currentMillis = millis();//this is a timing loop because the relays should fire no more than 20x/s.
   if (currentMillis - previousMillis >= 50) {
        previousMillis = currentMillis;
  /*below we take the difference from where we are (cylinder pots) to where we want to be (joystick pots) and
  send it to the PWM valves. The valves are proportional so the bigger the
  difference the wider the PWM valves open.  It is a non-linear equation so writing it
  with multiple if statements is easier to adjust in the field.

  /////////////////////////Left and Right//////////////////////////////
  if(diff>100 && diff<=200){digitalWrite(12,HIGH);digitalWrite(13,LOW);analogWrite(6,120);}
  if(diff>20 && diff<=100){digitalWrite(12,HIGH);digitalWrite(13,LOW);analogWrite(6,100);}
  if(diff<=20 && diff>=-20){digitalWrite(12,HIGH);digitalWrite(13,HIGH);analogWrite(6,0);}//this is called the deadband
  if(diff<-20 && diff>=-100){digitalWrite(12,LOW);digitalWrite(13,HIGH);analogWrite(6,100);}
  if(diff<-100 && diff>=-200){digitalWrite(12,LOW);digitalWrite(13,HIGH);analogWrite(6,120);} 

  /////////////////throttle and brake//////////////////////////////////////////////////
  diff1=(brake + aft - throttle);
  if(diff1>100 && diff1<=200){digitalWrite(10,HIGH);digitalWrite(11,LOW);analogWrite(5,90);}
  if(diff1>20 && diff1<=100){digitalWrite(10,HIGH);digitalWrite(11,LOW);analogWrite(5,80);}//extend or brake (recover from go fast)
  if(diff1<=20 && diff1>=-20){digitalWrite(10,HIGH);digitalWrite(11,HIGH);analogWrite(5,0);}
  if(diff1<-20 && diff1>=-100){digitalWrite(10,LOW);digitalWrite(11,HIGH);analogWrite(5,80);}
  if(diff1<-100 && diff1>=-200){digitalWrite(10,LOW);digitalWrite(11,HIGH);analogWrite(5,90);}//retract or go fast
  if(diff1<-200){digitalWrite(10,LOW);digitalWrite(11,HIGH);analogWrite(5,120);}//retract or go fast



Not a criticism of the code itself (maybe later), but lines like


are very difficult to read as the code block to be executed if the condition is true does not stand out

if (diff > 200)
  digitalWrite(12, HIGH);
  digitalWrite(13, LOW);
  analogWrite(6, 150);

is easier to read and maintain.

Give a name to your pins

const byte aftPin = A0;

will go a long way for readability

Now for some critique of the code.
Why aren't you giving your pins meaningful names ?
Again, easier to read and maintain.

Your global variables should have more descriptive names and have names starting with a capital letter so people reading the code can tell when you are using globals.

Your pin numbers should be given names since the numbers don't have useful meaning. Do that with 'const byte' variables. Preferably name the constants in ALL_CAPS and end the name with _PIN.

Your comments are labeling output pins as "Switch". Did you mean "Relay"? Switches are generally not a digitally controllable device.

You can (and generally SHOULD) write an initial value to an output pin before setting the mode to OUTPUT.

You only need to read the analog pin twice if you made the mistake of using a pot of over 10k. When you do the extra read you should add a comment to explain why you are reading the pin and ignoring the result.

Your indentation seems to be random. Use the auto-format feature of the IDE to clean up your formatting to make the code easier to read and understand.

Rather than breaking up the range of control into blocks you can use the MultiMap function to do linear interpolation. This will give smoother control.

Set the warning level in Preferences to ALL. Then fix any warnings your code generates. Sometimes fixing those warnings will prevent a logic error. (I have not tried compiling your sketch yet so I don't know if it triggers any warnings.)

I have not examined the ranges being tested, but if some or all of them are mutually exclusive then you could use if/else if/else instead of repeated ifs. That way the program will not check impossible conditions when a matching range has been found.

You guys are so awesome!!!! I will get my act together with all the suggestions and maybe repost the results in a week or so. See how much I learned!!!!

Normally, if you ask for a critique you will get a bunch of nitpicking and people evangelizing their own personal programming style.

But when everyone says the same thing - use descriptive names rather than magic numbers - it's a fair bet that that's what you should do.

Other criticismas include:

Format your code with ctrl-T

Dont use while(digitalRead(4)==LOW), use if(digitalRead(4)==LOW). This allows the loop to exit and return back to the underlying main loop, which does some important stuff.

When you have a chain of ifs that are checking ranges, use an if-else chain:

  else if(diff>100){digitalWrite(12,HIGH);digitalWrite(13,LOW);analogWrite(6,120);}
  else if(diff>20){digitalWrite(12,HIGH);digitalWrite(13,LOW);analogWrite(6,100);}
  else if(diff>=-20){digitalWrite(12,HIGH);digitalWrite(13,HIGH);analogWrite(6,0);}//this is called the deadband
  else if(diff>=-100){digitalWrite(12,LOW);digitalWrite(13,HIGH);analogWrite(6,100);}
  else if(diff>=-200){digitalWrite(12,LOW);digitalWrite(13,HIGH);analogWrite(6,120);}
  else {digitalWrite(12,LOW);digitalWrite(13,HIGH);analogWrite(6,140);}

When you have repeated code, consider breaking it out into a function

void loop() {
  if(diff>200) left(150);
  else if(diff>100) left(120);
  else if(diff>20) left(100);
  else if(diff>=-20) center(); //this is called the deadband
  else if(diff>=-100) right(100);
  else if(diff>=-200) right(120);
  else right(140)

void left(byte amount) {

void center() {

void right(byte amount) 
    analogWrite(6, amount); 

Why not calculate an amount rather than having hard breaks? The standard function "map" is provided for exactly this purpose:

  if(diff>200) left(150);
  else if(diff>20) left( map(diff, 20, 200, 100, 150)  );
  // etc

used with "constrain" function, this becomes:

  if(diff>20) left( constrain(map(diff, 20, 200, 100, 150), 100, 150);
  // etc

Heck - you don't really need those function broken out any more, do you? You are not copy/pasting the same block of code multiple times

  if(diff>20) {
    analogWrite(6, constrain(map(diff, 20, 200, 100, 150), 100, 150));
  else if(diff>-20) {
    analogWrite(6, 0);
  else {
    analogWrite(6, constrain(map(diff, -20, -200, 100, 140), 100, 140));

to build on @johnwasser's point regrding two analog reads... instead of this:

aft =analogRead(A0); //read first analog input potentiometer twice to settle DAC

consider this syntax:

aft =analogRead(A0);

where it explicitly denotes programmer's intention to toss out the first return (and eliminates the need for periphrastic commenting... 'cuz programmers understand).

Oh, I missed that one:

aft =analogRead(A0); //read first analog input potentiometer twice to settle DAC

Does this need a delayMicroseconds() between the two readings?

If analogRead() can be a bit noisy, I like to use a basic low-pass filter:

float runningAverage;

void loop() {
  if(it's been 4ms since I took the last reading) {
    runningAverage = runningAverage * .75 + analogRead(pin) * .25;
    make a note of the time;