Advanced Question Set lon/lat position from input

[Mythos]

Hi folks,

this is not a question, I already worked out this solution. But it made me a lot of headache, so it would be useful for others, too.

Input can look like:
"Brighton Beach" -> get pos of that base
"Brighton Beach 1" -> get pos of base's pad 1
"12.34W 56.78N" -> lon lat (DEG)
"98.76S 54.32E" -> swapped
"12.34 -56.78" -> lon lat (DEG)

Headaches with that:
- Not to use sscanf with "%f" for doubles, use "%lf"!
- Problems to get the "E" because of scientific float notation
- Side effect: You may use "O" as "E" (like on german compass)

You may help me to replace rad=0.0 with the shortest way to get rad of surface level instead.

bool StrToEqu(OBJHANDLE planet, char *str, VECTOR3 &lonradlat, bool strcorrection)
{
 // planet: OBJHANDLE to the planet/moon (e.g. planet = vessel->GetSurfaceRef())
 // str: the string to scan (from input box)
 // output lonradlat (RAD): I always store equ positions in vectors like x=lon, y=rad, z=lat
 //  That can easily be added by vectors in vessel orientation (horizon level, north facing)
 // strcorrection: true converts the input string e.g. "brighton beach" into "Brighton Beach" for later
 // returns: true = found sth, false = not found or syntax error

 OBJHANDLE base;
 std::string s;
 int l, pos;
 DWORD pad;
 double d1, d2, dt;
 char c1, c2, ct;
 bool ok;

 l = strlen(str);
 if(l > 0)
 {
  // if whole string is a base name, get that
  base = oapiGetBaseByName(planet, str);
  if(base != NULL)
  {
   oapiGetBaseEquPos(base, &lonradlat.x, &lonradlat.z, &lonradlat.y);
   if(strcorrection)
    oapiGetObjectName(base, str, l+1); 
   return true;
  }

  // input "Brighton Beach 1" would mean Pad1 of that base
  s = str;
  pos = s.find_last_of(' ');
  if(pos > 0)
  {
   s[pos] = char(0);
   if(sscanf(str + pos + 1, "%d", &pad) == 1)
   {
    base = oapiGetBaseByName(planet, (char *)s.c_str());
    if(base != NULL)
    {
     if((pad > 0) && (pad <= oapiGetBasePadCount(base)))
     {
      oapiGetBasePadEquPos(base, pad-1, &lonradlat.x, &lonradlat.z, &lonradlat.y);
      if(strcorrection)
      {
       oapiGetObjectName(base, str, l+1);
       sprintf(str, "%s %d", str, pad);
      }
      return true;
     }
    }
   }
  }

  //input like "12.34E 56.78N"
  s = str;
  for(unsigned int i=0;i<s.length();i++)
  {
   // do uppercase and replace E with O 
   // (E would be interpreted as scientific float notation)
   switch(s[i])
   {
   case 'n':
    s[i] = 'N';
    break;
   case 's':
    s[i] = 'S';
    break;
   case 'w':
    s[i] = 'W';
    break;
   case 'E':
    s[i] = 'O';
    break;
   case 'e':
    s[i] = 'O';
    break;
   case 'o':
    s[i] = 'O';
    break;
   }
  }
  if(sscanf((char *)s.c_str(), "%lf%c %lf%c", &d1, &c1, &d2, &c2) == 4)
  {
   if(((c1 == 'N') || (c1 == 'S')) && ((c2 == 'O') || (c2 == 'W')))
   {
    // swap
    ct = c1;
    c1 = c2;
    c2 = ct;
    dt = d1;
    d1 = d2;
    d2 = dt;
   }
   ok = false;
   if(c1 == 'O')
   {
    if(c2 == 'N')
    {
     lonradlat.x = d1 * RAD;
     lonradlat.z = d2 * RAD;
     ok = true;
    }
    else if(c2 == 'S')
    {
     lonradlat.x = d1 * RAD;
     lonradlat.z = -d2 * RAD;
     ok = true;
    }
   }
   else if (c1 == 'W')
   {
    if(c2 == 'N')
    {
     lonradlat.x = -d1 * RAD;
     lonradlat.z = d2 * RAD;
     ok = true;
    }
    else if(c2 == 'S')
    {
     lonradlat.x = -d1 * RAD;
     lonradlat.z = -d2 * RAD;
     ok = true;
    }
   }
   if(ok)
   {
    lonradlat.y = 0.0; // surface level would be better
    return true;
   }
  }
		
  //input like "12.34 -56.78"
  if(sscanf(str, "%lf %lf", &d1, &d2) == 2)
  {
   lonradlat.x = d1 * RAD;
   lonradlat.z = d2 * RAD;
   lonradlat.y = 0.0; // surface level would be better
   return true;
  }
 }

 // wrong input or nothing found
 return false;
}

 

[Enjo]

Kinda ugly C/C++ code. Here are some C++ tips:

1. If you need to swap values, use std::swap. Saves 4 lines of code.
2. For converting strings into values rather use stringstreams than sscanf, although the latter has its beauty. Stringstreams can be chained, like
   if (ss >> myDouble1 >> myDouble2 >> myInt)
   {
   // successful conversion - ss contained two doubles and an integer.
   }
3. Instead of making so many switches, looking for the char, better use this:
   std::transform(s.begin(), s.end(), s.begin(), (int (*)(int))std::toupper);
   and then just check if any char is E, which would be converted to O. Saves about 20 lines of code.
4. Never cast string.c_str() to char *. This may break your app, in case the function receiving the char * modifies it. Do it only if you have to, like in case of Orbiter API, but it's a mistake in the API to accept char * instead of const char *. It's called const correctness.
5. Use [ame="http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization"]Resource Acquisition Is Initialization[/ame] . Declaring variables at the beginning of scope is a requirement for C (I think), but a bad habit for C++. Declaring variables only when they are needed increases readability and RAII saves you from uninitialized variables (amongst proper cleanup).

Don't take it personal, because very few people write good C++ code, but I wouldn't hire somebody who showed me this as a production code.

 

[Mythos]

I'm more used to delphi programming. This C++ string manipulation is a pain in the ass.

If you need to swap values, use std::swap. Saves 4 lines of code.

I searched some minutes for sth like that and then decidet to spend only 10 seconds to do it the obvious, code intensive but not performance eating way.

For converting strings into values rather use stringstreams than sscanf, although the latter has its beauty. Stringstreams can be chained, like
if (ss >> myDouble1 >> myDouble2 >> myInt)
{
// successful conversion - ss contained two doubles and an integer.
}

I used that in my ReadScenario, less better readability and more difficult to read same string twice or to read rest of the line. The scanf'ed string only exists in this method so all is under full control.

Instead of making so many switches, looking for the char, better use this:
std::transform(s.begin(), s.end(), s.begin(), (int (*)(int))std::toupper);
and then just check if any char is E, which would be converted to O. Saves about 20 lines of code.

Transforming the string twice (first uppercase, then replace E with O) would be bad. Writing a replace function (with no sense of reusage anywhere else) that does both changes in one run would surely not save any lines in summary. Because syntax error input has not to be uppercased and only 4 (5) chars are ok, the switch is just fit to this special case.

Never cast string.c_str() to char *. This may break your app, in case the function receiving the char * modifies it. Do it only if you have to, like in case of Orbiter API, but it's a mistake in the API to accept char * instead of const char *. It's called const correctness.

The string is only scanned as a char * and only manipulated before. I don't feel very will with that, too. But I had a string and that needed a char *. I wanted to avoid having another local char array with some length that has to fit to the input and therefor a length had to be in paramter list also.

Use Resource Acquisition Is Initialization . Declaring variables at the beginning of scope is a requirement for C (I think), but a bad habit for C++. Declaring variables only when they are needed increases readability and RAII saves you from uninitialized variables (amongst proper cleanup).

It's all within one single method. Spreading them within the lines using them and declaring twice a "OBJHANDLE base" would be better readable?!

 

[Enjo]

I used that in my ReadScenario, less better readability and more difficult to read same string twice or to read rest of the line(...)

You need to reinitialize the stringstream with str() to reuse it. Well, sscanf isn't bad.

Transforming the string twice (first uppercase, then replace E with O) would be bad. Writing a replace function (with no sense of reusage anywhere else) that does both changes in one run would surely not save any lines in summary.

I would do:
std::transform(s.begin(), s.end(), s.begin(), (int (*)(int))std::toupper);
for(unsigned i=0; i<s.length(); ++i)
{
// replace E with O
// (E would be interpreted as scientific float notation)
if (s[i] == 'E' )
s[i] = 'O';
}
Still many lines of code saved.

Because syntax error input has not to be uppercased and only 4 (5) chars are ok, the switch is just fit to this special case.

You are working with a copy of the original string, so you can safely uppercase the whole string. You're not using it later other than checking the uppercased chars.Don't worry about performance here. It's damn fast, plus you use it only for reading input, so not like 1000 times per second.

The string is only scanned as a char * and only manipulated before. I don't feel very will with that, too. But I had a string and that needed a char *. I wanted to avoid having another local char array with some length that has to fit to the input and therefor a length had to be in paramter list also.

I understand the point of local char arrays. As I said, do it if you have to, and know for sure that the your char array won't be modified, like in case of Orbiter API. It's not your mistake. But for sscanf you shouldn't be doing it (it shows that you don't know what you are doing)

It's all within one single method. Spreading them within the lines using them and declaring twice a "OBJHANDLE base" would be better readable?!

As you can see, it's arguable. Declaring this variable twice may seem unreadable to you but if you did it, you'd have to declare it as locally as possible (good, because helps in memory management ... in all languages, as well as reduces namespace pollution), and using RAII would save you from accidentally using it uninitialized (even better...).


In general, an argument that everything is contained in a function is not convincing if the code is still hard to read for somebody else. You've only done the first step in structural programming - removing global variables. That's good but not enough to make the code readable and easy to maintain.


To answer your original question, you're already obtaining the base's height. It's the planet's radius.

 

[Mythos]

To answer your original question, you're already obtaining the base's height. It's the planet's radius.

It's clear for base-by-name input, but for lon/lat input I put 0.0 to the rad element. And I want it to be defined also for planets where there is no base at all (otherwise anyone else than us would read rad of any base quick and dirty).