Go Down

Topic: How can i improve my code ? (Read 324 times) previous topic - next topic

kackarli

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.

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.
If you understand an example, use it.
If you don't understand an example, don't use it.

Electronics engineer by trade, software engineer by profession. Trying to get back into electronics after 15 years absence.

Robin2

Two or three hours spent thinking and reading documentation solves most programming problems.

kackarli

#3
Dec 16, 2018, 11:16 am Last Edit: Dec 16, 2018, 11:17 am by kackarli
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 :)

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 :)

Deva_Rishi

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)
To 'Correct' you have to be Correct. (and not be condescending..)

sterretje

Post the code (if required in more than 1 post) then i'll have a look
Why?
If you understand an example, use it.
If you don't understand an example, don't use it.

Electronics engineer by trade, software engineer by profession. Trying to get back into electronics after 15 years absence.

sterretje

#6
Dec 16, 2018, 11:21 am Last Edit: Dec 16, 2018, 11:21 am by sterretje
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.
If you understand an example, use it.
If you don't understand an example, don't use it.

Electronics engineer by trade, software engineer by profession. Trying to get back into electronics after 15 years absence.

Deva_Rishi

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..
To 'Correct' you have to be Correct. (and not be condescending..)

Robin2

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
Two or three hours spent thinking and reading documentation solves most programming problems.

Deva_Rishi

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.
To 'Correct' you have to be Correct. (and not be condescending..)

econjack

Some suggestions:
1. When using #includes for files not part of the standard IDE, add a comment where it can be downloaded:
     #include "RTClib.h"  // https://github.com/adafruit/RTClib
2. When initializing float data type, use 0.0 instead of just 0 to help document its type.
3. 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.
4. 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.)

LowArt

#11
Dec 16, 2018, 04:31 pm Last Edit: Dec 16, 2018, 04:32 pm by LowArt
Code: [Select]

  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.

kackarli

#12
Dec 16, 2018, 07:47 pm Last Edit: Dec 16, 2018, 07:49 pm by kackarli
Some suggestions:
1. When using #includes for files not part of the standard IDE, add a comment where it can be downloaded:
    #include "RTClib.h"  // https://github.com/adafruit/RTClib
2. When initializing float data type, use 0.0 instead of just 0 to help document its type.
3. 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.
4. 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 :D and yes switch case make sense here.Thanks for your suggestions :)

Code: [Select]

  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.

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.

Metallor

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

Code: [Select]
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?

Robin2

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
Two or three hours spent thinking and reading documentation solves most programming problems.

Go Up