How to improve code?

Hello would anybody be able to check out my code below (for two sets of traffic lights), and be able to improve or show me how to get the same results in a much shorter and more efficient way of writting code? guidence would be appreciated, thanks.

//two sets of traffic lights int led1 = 13; int led2 = 12; int led3 = 11; int led4 = 10; int led5 = 9; int led6 = 8;

int delayPeriod = 5000;

void setup() { // This code will only run once, after each powerup or reset of the board pinMode (led1, OUTPUT); pinMode (led2, OUTPUT); pinMode (led3, OUTPUT); pinMode (led4, OUTPUT); pinMode (led5, OUTPUT); pinMode (led6, OUTPUT); }

void loop() { // This code will loops consecutively digitalWrite (led1, HIGH); digitalWrite (led6, HIGH); delay (delayPeriod); digitalWrite (led1, HIGH); digitalWrite (led6, LOW); digitalWrite (led5, HIGH); delay (delayPeriod); digitalWrite (led1, LOW); digitalWrite (led5, LOW); digitalWrite (led3, HIGH); digitalWrite (led4, HIGH); delay (delayPeriod); digitalWrite (led3, LOW); digitalWrite (led2, HIGH); delay (delayPeriod); digitalWrite (led2, LOW); digitalWrite (led4, LOW); digitalWrite (led1, HIGH); digitalWrite (led6, HIGH); delay (delayPeriod);


Use code tags, look for this in your available tools, #

I don't know your level of programming, but you could either make a function for the lights OR use Direct port manipulation with an array.

See the blink without delay example.


(guess I am slow typing ...) You could put the states of your 6 outputs in an array, and update the output based on the array data. You are using outputs that are all in PORTB, so take advantage of that.

// add to pre-setup code
byte dataArray[] = {B00000000,  B00001111, B00111111, B00110011, B00001100,};  // whatever the 5 states are (0,1,2,3,4)

// void setup stays the same

// new void loop in blink without delay format
void loop(){
currentMillis = millis();  // capture the "time"
if ( (currentMillis - previousMillis) >= delayPeriod) {  // time passed, ready for next action?
previousMillis = previousMillis + delayPeriod;   // reset for next time interval
outputState = outputState +1;    // update for the next output state
  if (outputState == 5){ outputState = 0;}  // reset if reached the end
PORTB = dataArray[outputState]; // output the next set of data
} // end time check
// do other stuff you gonna do while waiting for next interval to come around
} // end loop

im brand spanking new to programming, and im still reading\learning about functions and arrays - thought i'd try the lazy route first and see if i could understand it easier if someone showed me from one of my own examples.

great stuff - thanks guys!

There’s not a great deal wrong with it as it is, if all you want is to blink some lights in a fixed sequence…but (you knew that was coming, didn’t you?) if you need to make it react to events like passing traffic, then the "delay()"s really have to go.
If you want to model more lights, then you need to learn about arrays, and if you want your code more readable, and therefore more maintainable, then you’re going to have to start giving names to things.
The practical outcome of all of this, is that you will probably end up typing MORE code than you have now, but that code will be more flexible and adaptable.

" if someone showed me from one of my own examples."

Isn't that what I did? You only need to make the output states in the array match the On/Offs you are using:

byte dataArray[] = {B00000000, B00001111, B00111111, B00110011, B00001100,}; // whatever the 5 states are (0,1,2,3,4)

bit7 not used bit 6 not use bit 5 = led 1 bit 4 = led 2 bit 3 = led 3 bit 2 = led 4 bit 1 = led 5 bit 0 = led 6

Yes, many thanks Crossroads. I'm still just a little bit (led, LOW) at programming. Thanks for the help :)

#include <SPI.h>

//Assign the Chip Select signal to pin 10.
int CS=10;

//This is a list of some of the registers available on the ADXL345.
//To learn more about these and the rest of the registers on the ADXL345, read the datasheet!
char POWER_CTL = 0x2D;	//Power Control Register
char DATA_FORMAT = 0x31;
char DATAX0 = 0x32;	//X-Axis Data 0
char DATAX1 = 0x33;	//X-Axis Data 1
char DATAY0 = 0x34;	//Y-Axis Data 0
char DATAY1 = 0x35;	//Y-Axis Data 1
char DATAZ0 = 0x36;	//Z-Axis Data 0
char DATAZ1 = 0x37;	//Z-Axis Data 1

//This buffer will hold values read from the ADXL345 registers.
char values[10];
//These variables will be used to hold the x,y and z axis accelerometer values.
int x,y,z;

void setup(){ 
  //Initiate an SPI communication instance.
  //Configure the SPI connection for the ADXL345.
  //Create a serial connection to display the data on the terminal.
  //Set up the Chip Select pin to be an output from the Arduino.
  pinMode(CS, OUTPUT);
  //Before communication starts, the Chip Select pin needs to be set high.
  digitalWrite(CS, HIGH);
  //Put the ADXL345 into +/- 4G range by writing the value 0x01 to the DATA_FORMAT register.
  writeRegister(DATA_FORMAT, 0x01);
  //Put the ADXL345 into Measurement Mode by writing 0x08 to the POWER_CTL register.
  writeRegister(POWER_CTL, 0x08);  //Measurement mode  

void loop(){
  //Reading 6 bytes of data starting at register DATAX0 will retrieve the x,y and z acceleration values from the ADXL345.
  //The results of the read operation will get stored to the values[] buffer.
  readRegister(DATAX0, 6, values);

  //The ADXL345 gives 10-bit acceleration values, but they are stored as bytes (8-bits). To get the full value, two bytes must be combined for each axis.
  //The X value is stored in values[0] and values[1].
  x = ((int)values[1]<<8)|(int)values[0];
  //The Y value is stored in values[2] and values[3].
  y = ((int)values[3]<<8)|(int)values[2];
  //The Z value is stored in values[4] and values[5].
  z = ((int)values[5]<<8)|(int)values[4];
  //Print the results to the terminal.
  Serial.print(x, DEC);
  Serial.print(y, DEC);
  Serial.println(z, DEC);      

//This function will write a value to a register on the ADXL345.
//  char registerAddress - The register to write a value to
//  char value - The value to be written to the specified register.
void writeRegister(char registerAddress, char value){
  //Set Chip Select pin low to signal the beginning of an SPI packet.
  digitalWrite(CS, LOW);
  //Transfer the register address over SPI.
  //Transfer the desired register value over SPI.
  //Set the Chip Select pin high to signal the end of an SPI packet.
  digitalWrite(CS, HIGH);

//This function will read a certain number of registers starting from a specified address and store their values in a buffer.
//  char registerAddress - The register addresse to start the read sequence from.
//  int numBytes - The number of registers that should be read.
//  char * values - A pointer to a buffer where the results of the operation should be stored.
void readRegister(char registerAddress, int numBytes, char * values){
  //Since we're performing a read operation, the most significant bit of the register address should be set.
  char address = 0x80 | registerAddress;
  //If we're doing a multi-byte read, bit 6 needs to be set as well.
  if(numBytes > 1)address = address | 0x40;
  //Set the Chip select pin low to start an SPI packet.
  digitalWrite(CS, LOW);
  //Transfer the starting register address that needs to be read.
  //Continue to read registers until we've read the number specified, storing the results to the input buffer.
  for(int i=0; i<numBytes; i++){
    values[i] = SPI.transfer(0x00);
  //Set the Chips Select pin high to end the SPI packet.
  digitalWrite(CS, HIGH);
[code]#include "SIM900.h"
#include <SoftwareSerial.h>
//If not used, is better to exclude the HTTP library,
//for RAM saving.
//If your sketch reboots itself proprably you have finished,
//your memory available.
//#include "inetGSM.h"

//If you want to use the Arduino functions to manage SMS, uncomment the lines below.
#include "sms.h"

//To change pins for Software Serial, use the two lines in GSM.cpp.

//GSM Shield for Arduino
//this code is based on the example of Arduino Labs.

//Simple sketch to send and receive SMS.

int numdata;
boolean started=false;
char smsbuffer[160];
char n[20];

void setup() 
  //Serial connection.
  Serial.println("GSM Shield testing.");
  //Start configuration of shield with baudrate.
  //For http uses is raccomanded to use 4800 or slower.
  if (gsm.begin(2400)){
  else Serial.println("\nstatus=IDLE");
    //Enable this two lines if you want to send an SMS.
    if (sms.SendSMS("+971505393365", "Arduino SMS"))
      Serial.println("\nSMS sent OK");


void loop() 
    //Read if there are messages on SIM card and print them.
    if(gsm.readSMS(smsbuffer, 160, n, 20))