Pages: [1] 2   Go Down
Author Topic: How can I simplify this simple code?  (Read 764 times)
0 Members and 1 Guest are viewing this topic.
Scottsdale
Offline Offline
Newbie
*
Karma: 0
Posts: 27
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

I have this relatively simple function that I believe can be simpler, but I'm not entirely sure how. I'm hoping this will give me more insight as to how I can make my other functions simpler as well.

This function turns IMU roll data received (clockwise roll goes to positive 180 degrees/counterclockwise roll goes to -180 degrees) to 360 degrees of roll. Here's the function:

Code:
float Plane::to360(String dir)
{
  if (current(dir) < 0) {
    absRollAngle = (360 + current(dir));
  }
  else {
    absRollAngle = current(dir);
  }
  return absRollAngle;
}

Thanks in advance,
Zachary
Logged

RadarProject - Arduino Autopilot for RC http://radarproject.wordpress.com/

Central MN, USA
Offline Offline
Tesla Member
***
Karma: 65
Posts: 6908
Phi_prompt, phi_interfaces, phi-2 shields, phi-panels
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Assuming current(dir) returns the numerical angle, you can do this:

absRollAngle=(current(dir)+360)%360;

http://arduino.cc/en/Reference/Modulo
Logged


nr Bundaberg, Australia
Offline Offline
Tesla Member
***
Karma: 121
Posts: 8453
Scattered showers my arse -- Noah, 2348BC.
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
float Plane::to360(String dir) {
  return current(dir) < 0 ?  360 + current(dir) : current(dir);
}

Mind you this doesn't set absRollAngle, is that a global? And if so why return it?

This may be less lines of source code but I'd say the generated code would be the same.

______
Rob
Logged

Rob Gray aka the GRAYnomad www.robgray.com

Seattle, WA USA
Offline Offline
Brattain Member
*****
Karma: 549
Posts: 46154
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Why are you using Strings? Why are you converting the String to a value 3 times?

A ternary operator (there is only one) wold reduce that to 2 lines of code, at the risk of making the code harder to understand.
Logged

Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 239
Posts: 24373
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
if (current(dir) < 0) {
    absRollAngle = (360 + current(dir));
Strings apart, shouldn't that be:
Code:
while (current(dir) < 0) {
    absRollAngle = (360 + current(dir));
or something similar?
Logged

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

Central MN, USA
Offline Offline
Tesla Member
***
Karma: 65
Posts: 6908
Phi_prompt, phi_interfaces, phi-2 shields, phi-panels
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

I think the modulo is easy enough.
Logged


Scottsdale
Offline Offline
Newbie
*
Karma: 0
Posts: 27
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Assuming current(dir) returns the numerical angle, you can do this:

absRollAngle=(current(dir)+360)%360;

http://arduino.cc/en/Reference/Modulo

Since I first started developing this program, I've learned about Strings and how bad they are (Although they make your code so readable).

I really like this method... when I try compiling, though, it says:  invalid operands of types 'float' and 'int' to binary 'operator%'
Logged

RadarProject - Arduino Autopilot for RC http://radarproject.wordpress.com/

Central MN, USA
Offline Offline
Tesla Member
***
Karma: 65
Posts: 6908
Phi_prompt, phi_interfaces, phi-2 shields, phi-panels
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Assuming current(dir) returns the numerical angle, you can do this:

absRollAngle=(current(dir)+360)%360;

http://arduino.cc/en/Reference/Modulo

Since I first started developing this program, I've learned about Strings and how bad they are (Although they make your code so readable).

I really like this method... when I try compiling, though, it says:  invalid operands of types 'float' and 'int' to binary 'operator%'

Since you didn't say what current(dir) does, I assumed it returns integer.


absRollAngle=(int(current(dir))+360)%360;
Logged


Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 239
Posts: 24373
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
I think the modulo is easy enough.
Easy, yes, but insufficient - if the initial value is more negative than -360, it won't work.

e.g. (-540 + 360) % 360 = -180.
Logged

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

Scottsdale
Offline Offline
Newbie
*
Karma: 0
Posts: 27
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

AWOL, current(dir) will never be more than 180 or -180.

Ok... too bad modulo doesn't work with float. What about this piece of code? I have a tolerance level. Basically, I want it to return true if the amount of roll is out of the desired roll with a specific tolerance. Is there a better way to write this?

Code:
boolean Plane::planeDisplaced(String dir, float input) {
/////// Checking To See if Plane is Within Bounderies ///////
      if (current(dir) < (input - tolerance) || current(dir) > (input + tolerance)) {
        return true;
      }
      else {
        return false;
      }
}

Thanks in advance,
Zachary
Logged

RadarProject - Arduino Autopilot for RC http://radarproject.wordpress.com/

UK
Offline Offline
Shannon Member
****
Karma: 184
Posts: 11183
-
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

boolean Plane::planeDisplaced(String dir, float input) {

You seem to be routinely passing numeric values around as Strings (and presumably parsing them again and again each time you need to access the value). You should really consider using numbers instead of strings. You could slightly simplify your bounds check using abs(), but I don't see it making a huge difference. Get rid of those Strings, though.
Logged

I only provide help via the forum - please do not contact me for private consultancy.

SF Bay Area (USA)
Offline Offline
Tesla Member
***
Karma: 106
Posts: 6374
Strongly opinionated, but not official!
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

It depends on what you mean by "simpler."

The "modulo" solution makes for a short expression, but will increase execution time, because "modulo" is a relatively expensive calculation.

The "ternary operator" (x ? y:z) makes for fewer lines of code in the source, but it harder to read (IMO), and is likely to produce the same code as the if/else statement.

As other have said, your use of Strings is inappropriate and inefficient.  Assuming that the argument to your function MUST be a string (ie, cleaning up this function, but not looking beyond it), you can improve things by only doing the string to number conversion once:

Code:
float Plane::to360(String dir)
{
  float dirval = current(dir);   // (or maybe "int" ?)

  if (dirval < 0) {
    absRollAngle = (360 + dirval);
  }  else {
    absRollAngle = dirval;
  }
  return absRollAngle;
}
Logged

Scottsdale
Offline Offline
Newbie
*
Karma: 0
Posts: 27
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Thanks everyone. I have taken your advice and altered my program to use compiler variables (bytes) instead of Strings and chars. I think it will run more efficiently now. Can someone please direct me to an article (Or give me an explanation) on why using String variables are usually bad in C++/Arduino?

Thanks again,
Zachary
Logged

RadarProject - Arduino Autopilot for RC http://radarproject.wordpress.com/

Central MN, USA
Offline Offline
Tesla Member
***
Karma: 65
Posts: 6908
Phi_prompt, phi_interfaces, phi-2 shields, phi-panels
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

There is not an article about using String class in Arduino is bad yet but from experience of many members and the fact that Arduino's free() function is buggy, it is easy to conclude that keep using String class will eventually drain memory to zero and crash the program.
Logged


New Jersey
Online Online
Faraday Member
**
Karma: 49
Posts: 3422
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Read Nick's sticky in this forum - it explains it and has a link to a discussion thread on the topic.
Logged

Pages: [1] 2   Go Up
Jump to: