Skip to main content
added 350 characters in body
Source Link
Lundin
  • 4.9k
  • 16
  • 29
  • The use of while(*c) is a debated coding style. As such, it is a bit subjective - some think this style is fine as it is "traditional C". Others, like the MISRA-C coding standard (and me) prefer while(*c != '\0') as this is more self-documenting, improves possibilities of static code analysis and prevents mix-ups with c != NULL, which of course means something else entirely.

    The use of while(*c) is a debated coding style. As such, it is a bit subjective - some think this style is fine as it is "traditional C". Others, like the MISRA-C coding standard (and me) prefer while(*c != '\0') as this is more self-documenting, improves possibilities of static code analysis and prevents mix-ups with c != NULL, which of course means something else entirely.

  • Always use compound statements { } after all control and loop expressions, with no exceptions. To skip this when there's just one line of code trailing is bad and dangerous practice. Such coding style caused the most expensive bug ever written in the history of programming, after which there are no arguments left that justifies the style.

  • The use of while(*c) is a debated coding style. As such, it is a bit subjective - some think this style is fine as it is "traditional C". Others, like the MISRA-C coding standard (and me) prefer while(*c != '\0') as this is more self-documenting, improves possibilities of static code analysis and prevents mix-ups with c != NULL, which of course means something else entirely.
  • The use of while(*c) is a debated coding style. As such, it is a bit subjective - some think this style is fine as it is "traditional C". Others, like the MISRA-C coding standard (and me) prefer while(*c != '\0') as this is more self-documenting, improves possibilities of static code analysis and prevents mix-ups with c != NULL, which of course means something else entirely.

  • Always use compound statements { } after all control and loop expressions, with no exceptions. To skip this when there's just one line of code trailing is bad and dangerous practice. Such coding style caused the most expensive bug ever written in the history of programming, after which there are no arguments left that justifies the style.

added 138 characters in body
Source Link
Lundin
  • 4.9k
  • 16
  • 29
bool stralpha(const char *c)
{
  const char* original = c;if(*c //== needed'\0')
 to handle{
 the special case ofreturn anfalse;
 empty string}

  while(*c != '\0' && isalpha(*c))
  {
    c++;
  } 

  return *c == '\0' && *original != '\0';
}
bool stralpha(const char *c)
{
  const char* original = c; // needed to handle the special case of an empty string

  while(*c != '\0' && isalpha(*c))
  {
    c++;
  }
  return *c == '\0' && *original != '\0';
}
bool stralpha(const char *c)
{
  if(*c == '\0')
  {
    return false;
  }

  while(*c != '\0' && isalpha(*c))
  {
    c++;
  } 

  return *c == '\0';
}
added 138 characters in body
Source Link
Lundin
  • 4.9k
  • 16
  • 29

The truly fastest way is some manner of look-up table, as demonstrated in another answer. Such a table could even be generated at compile time:

const bool is_alpha [256] =
{
  ['A'] = true,
  ['B'] = true,
  ...
  ['Z'] = true,
  ['a'] = true,
  ...
};

Elements that aren't set to true are guaranteed to be default initialized to 0 = false.


That being said, here is my code review:

Bugs

  • bool alphabetic = true; means that your function will return true if the first element of the array is \0.

  • You don't stop once you find an invalid character, so the whole function is needlessly slow.

Coding style

  • The !! is fine. The other reviews fail to grasp that the C standard only guarantees: (C11 7.4.1)

The functions in this subclause return nonzero (true) if and only if the value of the argument c conforms to that in the description of the function.

This text has remained unchanged since C90, as in before C had a boolean type. So it does not mean that the function necessarily returns boolean true, but rather that it returns anything non-zero.

However, the code would be more readable if you write is_alpha(...) != false. The use of !! is often criticised as obfuscation.

  • The use of ++ (pre or postfix) mixed with other operators in the same expression is widely recognized as bad practice. There are many dangers with this, both operator precedence bugs and risk of invoking poorly-defined behavior. In addition, it makes the code hard to read.

Suppose for example that someone comes in and maintains the code, want to allow spaces, is aware that the is... functions return anything non-zero, and therefore writes something like !!(isalpha(*c++) | isspace(*c)). Boom, severe bug, undefined behavior.

Therefore, never write things such as *c++ even though this happen to be a very commonly used C trick. There is no benefit what-so-ever of mixing ++ with other operators, only dangers.

  • The use of while(*c) is a debated coding style. As such, it is a bit subjective - some think this style is fine as it is "traditional C". Others, like the MISRA-C coding standard (and me) prefer while(*c != '\0') as this is more self-documenting, improves possibilities of static code analysis and prevents mix-ups with c != NULL, which of course means something else entirely.

A fixed version:

bool stralpha(const char *c)
{
  const char* original = c; // needed to handle the special case of an empty string

  while(*c != '\0' && isalpha(*c))
  {
    c++;
  }
  return *c == '\0' && *original != '\0';
}

The truly fastest way is some manner of look-up table, as demonstrated in another answer. Such a table could even be generated at compile time:

const bool is_alpha [256] =
{
  ['A'] = true,
  ['B'] = true,
  ...
  ['Z'] = true,
  ['a'] = true,
  ...
};

Elements that aren't set to true are guaranteed to be default initialized to 0 = false.


That being said, here is my code review:

Bugs

  • bool alphabetic = true; means that your function will return true if the first element of the array is \0.

  • You don't stop once you find an invalid character, so the whole function is needlessly slow.

Coding style

  • The !! is fine. The other reviews fail to grasp that the C standard only guarantees: (C11 7.4.1)

The functions in this subclause return nonzero (true) if and only if the value of the argument c conforms to that in the description of the function.

This text has remained unchanged since C90, as in before C had a boolean type. So it does not mean that the function necessarily returns boolean true, but rather that it returns anything non-zero.

However, the code would be more readable if you write is_alpha(...) != false. The use of !! is often criticised as obfuscation.

  • The use of ++ (pre or postfix) mixed with other operators in the same expression is widely recognized as bad practice. There are many dangers with this, both operator precedence bugs and risk of invoking poorly-defined behavior. In addition, it makes the code hard to read.

Suppose for example that someone comes in and maintains the code, want to allow spaces, is aware that the is... functions return anything non-zero, and therefore writes something like !!(isalpha(*c++) | isspace(*c)). Boom, severe bug, undefined behavior.

Therefore, never write things such as *c++ even though this happen to be a very commonly used C trick. There is no benefit what-so-ever of mixing ++ with other operators, only dangers.

  • The use of while(*c) is a debated coding style. As such, it is a bit subjective - some think this style is fine as it is "traditional C". Others, like the MISRA-C coding standard (and me) prefer while(*c != '\0') as this is more self-documenting, improves possibilities of static code analysis and prevents mix-ups with c != NULL, which of course means something else entirely.

A fixed version:

bool stralpha(const char *c)
{
  while(*c != '\0' && isalpha(*c))
  {
    c++;
  }
  return *c != '\0';
}

The truly fastest way is some manner of look-up table, as demonstrated in another answer. Such a table could even be generated at compile time:

const bool is_alpha [256] =
{
  ['A'] = true,
  ['B'] = true,
  ...
  ['Z'] = true,
  ['a'] = true,
  ...
};

Elements that aren't set to true are guaranteed to be default initialized to 0 = false.


That being said, here is my code review:

Bugs

  • bool alphabetic = true; means that your function will return true if the first element of the array is \0.

  • You don't stop once you find an invalid character, so the whole function is needlessly slow.

Coding style

  • The !! is fine. The other reviews fail to grasp that the C standard only guarantees: (C11 7.4.1)

The functions in this subclause return nonzero (true) if and only if the value of the argument c conforms to that in the description of the function.

This text has remained unchanged since C90, as in before C had a boolean type. So it does not mean that the function necessarily returns boolean true, but rather that it returns anything non-zero.

However, the code would be more readable if you write is_alpha(...) != false. The use of !! is often criticised as obfuscation.

  • The use of ++ (pre or postfix) mixed with other operators in the same expression is widely recognized as bad practice. There are many dangers with this, both operator precedence bugs and risk of invoking poorly-defined behavior. In addition, it makes the code hard to read.

Suppose for example that someone comes in and maintains the code, want to allow spaces, is aware that the is... functions return anything non-zero, and therefore writes something like !!(isalpha(*c++) | isspace(*c)). Boom, severe bug, undefined behavior.

Therefore, never write things such as *c++ even though this happen to be a very commonly used C trick. There is no benefit what-so-ever of mixing ++ with other operators, only dangers.

  • The use of while(*c) is a debated coding style. As such, it is a bit subjective - some think this style is fine as it is "traditional C". Others, like the MISRA-C coding standard (and me) prefer while(*c != '\0') as this is more self-documenting, improves possibilities of static code analysis and prevents mix-ups with c != NULL, which of course means something else entirely.

A fixed version:

bool stralpha(const char *c)
{
  const char* original = c; // needed to handle the special case of an empty string

  while(*c != '\0' && isalpha(*c))
  {
    c++;
  }
  return *c == '\0' && *original != '\0';
}
Source Link
Lundin
  • 4.9k
  • 16
  • 29
Loading