Hi.
I am working on a project which measure temperature,logs data to SD card and use RTC. I show the datas on LCD screen. it logs datas in every 10 seconds. it works properly now. But i am not satisfied about my code exactly. I would appreciate if you help me to improve my code.
Used hardwares.
Arduino uno
Sd card and RTC shield
Lcd kaypad shield
DHT 22
the code is in attachment.
sonDeneme.ino (14.9 KB)
Write more functions that you can call from loop(). In the old days, functions should fit an a page of A4 paper or less. Your loop() is far too long. E.g. the whole serial handling at the end of loop() can go in an individual function that you can call from there.
Get rid of the String class (capital S) in favor of c-strings. Strings (capital S) can cause memory fragmentation and the result can be unexpected run-time behaviour.
else if should start on a new line (in my opinion).
Just some thoughts.
These links may help
Planning and Implementing a Program
Serial Input Basics
...R
sterretje:
Write more functions that you can call from loop(). In the old days, functions should fit an a page of A4 paper or less. Your loop() is far too long. E.g. the whole serial handling at the end of loop() can go in an individual function that you can call from there.Get rid of the String class (capital S) in favor of c-strings. Strings (capital S) can cause memory fragmentation and the result can be unexpected run-time behaviour.
else if should start on a new line (in my opinion).
Just some thoughts.
I will do a search about String and c-strings and will try to get rid of them.i will take whole serial in an individual function. Thanks for reply sterretje 
Robin2:
These links may help
Planning and Implementing a Program
Serial Input Basics...R
waw... The next thing is: I will read your chapters Robin2. Thanks for your reply
Post the code (if required in more than 1 post) then i'll have a look, i agree with what has been said already. imo loop() should not have much more then 10 lines of code (not counting braces)
Deva_Rishi:
Post the code (if required in more than 1 post) then i'll have a look
Why?
kackarli:
i will take whole serial in an individual function.
The serial stuff was just an example; there are other blocks of code that can easily go in a function.
sterretje:
Why?
because like this i'd have to download it first... just to have a look. Also there are people that use their phone to look at posts on the forum, they can not open the attachment usually..
Deva_Rishi:
because like this i'd have to download it first... just to have a look. Also there are people that use their phone to look at posts on the forum, they can not open the attachment usually..
Posting a long program as separate parts introduces a great risk of errors when the two parts are joined. IMHO it is much better just to post it as an attachment.
...R
Robin2:
Posting a long program as separate parts introduces a great risk of errors when the two parts are joined. IMHO it is much better just to post it as an attachment.
Whatever... one could do both.
Some suggestions:
I prefer #define to const because a #define is typeless.
switch/case does one test and then a jump table, which is usually more efficient.
5. Use consistent naming convention on your functions. I prefer starting the function IDs I write with a capital letters and camel notation thereafter. It documents at a glance that I wrote it and it's not part of a library. I do the same thing for methods I write for a class I designed. (Most imported libraries are pretty consistent about using lowercase letter for class methods. By capping it, I know I wrote the class.)
if (! saatIslemcisi.begin()) {
ekran.clear();
ekran.setCursor(0, 0);
ekran.print(F("saat baslamiyor"));
ekran.setCursor(0, 1);
ekran.print(F("Serivise gonderin"));
while (1);
}
I personally prefer to do all my naming of functions and writing on displays in English. Half of the naming is any way in English (clear, setCursor…) and that way its consistent and you do not have to rape your language (baslamiyo / başlamiyo.
But the main reason why I like doing this is that I have wasted quite some time in the past trying to get German letters like “öäüß” or in your case Turkish case “şğ” right. In my personal opinion everything that’s not in the normal ASCIIchart will always cause problems.
econjack:
Some suggestions:
- When using #includes for files not part of the standard IDE, add a comment where it can be downloaded:
#include "RTClib.h" // GitHub - adafruit/RTClib: A fork of Jeelab's fantastic RTC Arduino library- When initializing float data type, use 0.0 instead of just 0 to help document its type.
- Avoid magic numbers:
if (saat != 23 && dakika != 59) {
- logKaydet = true;*
- }*
I prefer #define to const because a #define is typeless.
- #define LOWERLIMIT 23*
- #define UPPERLIMIT 59*
You might prefer const.
- A switch/case is often more efficient than a series of if statement blocks:
- switch (mesa) {*
- case '1':*
- // your code...*
- break;*
- // more cases...*
- default:*
- Serial.print("Shouldn't be here. mesa = ");*
- Serial.print(mesa);*
- break;*
- }*
switch/case does one test and then a jump table, which is usually more efficient.
5. Use consistent naming convention on your functions. I prefer starting the function IDs I write with a capital letters and camel notation thereafter. It documents at a glance that I wrote it and it's not part of a library. I do the same thing for methods I write for a class I designed. (Most imported libraries are pretty consistent about using lowercase letter for class methods. By capping it, I know I wrote the class.)
i will get rid of magic numbers 
and yes switch case make sense here.Thanks for your suggestions
LowArt:
if (! saatIslemcisi.begin()) {
ekran.clear();
ekran.setCursor(0, 0);
ekran.print(F("saat baslamiyor"));
ekran.setCursor(0, 1);
ekran.print(F("Serivise gonderin"));
while (1);
}
I personally prefer to do all my naming of functions and writing on displays in English. Half of the naming is any way in English (clear, setCursor…) and that way its consistent and you do not have to rape your language (baslamiyo / başlamiyo. But the main reason why I like doing this is that I have wasted quite some time in the past trying to get German letters like “öäüß” or in your case Turkish case “şğ” right. In my personal opinion everything that’s not in the normal [ASCIIchart](https://www.arduino.cc/en/Reference/ASCIIchart) will always cause problems.
i will make all variable and function names in engish. The persons who will use the project, dont know english. So i decided to show LCD information in my language. Thanks for your reply
I am reading Robin2's chapters. After that i will update the sketch and upload here again.
Robin2:
These links may help
Planning and Implementing a Program
Serial Input Basics...R
Robin, I am reading through and enjoying your "Planning and Implementing a Program" series. In chapter 6 you have
if (button0state == LOW) {
ledAoffMillis = ledAbaseInterval;
ledBoffMillis = ledAbaseInterval;
}
if (button1state == LOW) {
ledAoffMillis = ledAbaseInterval >> 1;
ledBoffMillis = ledAbaseInterval >> 1;
}
but shouldn't ledB use ledBbaseInterval and not ledAbaseInterval?
Metallor:
but shouldn't ledB use ledBbaseInterval and not ledAbaseInterval?
I think you are correct but I want to test the program to be sure. It's a long time since I wrote it. I'll try and do it later today.
Nice to see at least one person read the code carefully
...R