suggest improvement to my code

suggetion to my code

#include <Arduino.h>
#include <Wire.h>
#include <RTClib.h>

int hour1 = 22;
int min1 =28;
int sec1 = 0;
int hour2 = 22;
int min2 = 29;
int sec2 = 0;
int x1;
int x2;
int x3;
int y1;
int y2;
int y3;
const int light = 13;
int buttonState = 0;

RTC_DS1307 RTC;

void setup()
{
Serial.begin(9600);
Wire.begin();
RTC.begin();

pinMode(light, OUTPUT);
pinMode(2, INPUT);
pinMode(3,INPUT);
pinMode(4, INPUT);
x1 ;
y1 ;
}

void loop()
{

DateTime now = RTC.now();

if(digitalRead(2) == HIGH)
{ if(now.hour() == hour1 && now.minute()== min1 && now.second() == sec1)
{
digitalWrite(light, HIGH);
}
if(now.hour() == hour2 && now.minute()== min2 && now.second() == sec2)
{
digitalWrite(light, LOW);
}
}

if(digitalRead(3) == HIGH)
now.hour() == x1;
{ if(now.hour() == x1 )
{
digitalWrite(light, HIGH);
}
if(now.hour() == hour2 && now.minute()== min2 && now.second() == sec2)
{
digitalWrite(light, LOW);
}
}

if(digitalRead(4) == HIGH)
now.hour() == hour1;
{ if(now.hour() == hour1 )
{
digitalWrite(light, HIGH);
}
if(now.hour() == y1 && now.minute()== y2 && now.second() == y3)
{
digitalWrite(light, LOW);
}
}
}

Comments? Code tags? More appropriate forum section? Remove the pointless parts of setup. Give the pins nice names. Use appropriate data types.

  • put your code in [code] tags when you post it on the forum.
  • use structs
struct Time {
  int hour;
  int min;
  int sec;
};

struct Pos {
  int x;
  int y;
};
  • this
x1 ;
y1 ;

does nothing.

  • this
 if(now.hour() == hour1 && now.minute()== min1 && now.second() == sec1)

Will fail if there's any sort of delay.

pranav2016: . . . pinMode(2, INPUT); . . . if(digitalRead(2) == HIGH) . . .

If you are reading switches, it is usually better to wire them between ground and the arduino pin and are, therefore, active LOW. This means you can use the internal pullup resistor on the pin, possibly reducing your component count:

pinMode(2, INPUT_PULLUP);

and this looks quite odd:

if(digitalRead(3) == HIGH)
          now.hour() == x1;
    {   if(now.hour() == x1 )
    . . .

Add comments in your code.

.

[....if(digitalRead(3) == HIGH) now.hour() == x1; { if(now.hour() == x1 ).....] this for storing the button press time in x1, and performing action at x1 instant of time.

Learn how to use this forum. See the thread on top of it.