12
\$\begingroup\$

Just starting, wrote simple program off the very basic knowledge I have learned today. Is this a correct way of implementing this? Any improvement tips?

//Print out the ASCII chart with its corresponding code
#include <iostream>
#include <string>

void printChart(char symbol, int code) {
    std::string ws = "White Space";
    std::cout << "CODE: " << code << " SYMBOL: ";
    if (isspace(code)) {
        std::cout << ws << "\n";
    }
    else
        std::cout << symbol << "\n";
}

int main() {

    char symbol = 0;
    int code = symbol;
    int char_range = 127;

    while (code < char_range) {
        if (isprint(code)) {
            printChart(symbol, code);
        }
        symbol += 1;
        code = symbol;
    }

    std::cout << "Strike any key to continue...";
    getchar();

    return 0;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Strictly speaking, this only prints an ASCII table if the host character encoding happens to be ASCII (or compatible, such as ISO 8859.x or Unicode). For other platforms (notably EBCDIC) you'll get a different table. C++ doesn't mandate any particular mapping between char values and the printable characters, and expects you to do any necessary conversions. This can be a complex area, and soon gets well out of beginner territory, so let's assume that the host environment uses ASCII. \$\endgroup\$ – Toby Speight Aug 11 '17 at 7:59
  • 1
    \$\begingroup\$ Another minor nit: the term "white space" means space (0x20), form feed (0x0c), line feed (0x0a), carriage return (0x0d), horizontal tab (0x09) or vertical tab (0x0b). The character for 0x20 (32 decimal) is called "space". \$\endgroup\$ – Edward Aug 11 '17 at 10:30
  • \$\begingroup\$ @TobySpeight: I think you mean UTF-8 rather than Unicode. Unicode is something abstract. \$\endgroup\$ – Oskar Skog Aug 11 '17 at 11:16
  • \$\begingroup\$ No, I meant Unicode, as we're dealing with code points here, not the external representation. It's permissible for a runtime (with sufficiently wide char type) to represent strings as a sequence of Unicode characters, and to convert between that and UTF-8 or UTF-16 for its I/O. You're right that a sequence of UTF-8 bytes is also an ASCII-compatible form. \$\endgroup\$ – Toby Speight Aug 11 '17 at 11:51
13
\$\begingroup\$

Sure, it works, if the host-characterset extends ASCII, thus a good start. And naturally, we have loads of ideas.

  1. printChart() receives the same input twice, using different types. Duplication is never good. Just pass it once and convert as needed inside.

  2. printChart() always outputs to std::cout. That's fine for a one-off, but if you ever want to reuse it consider making the used output-stream an (optionally defaulted) argument (type std::ostream&).

  3. Try to minimize the number of objects you create, especially those which might need dynamic allocation like std::string. You can just use a string-literal instead of ws, or at least change its type to const char*.

  4. There's no reason not to handle char-values outside the unsigned char-range right if you get them as a char: Make sure to cast to unsigned char before passing to character-classification-functions like isspace and isprint.

  5. If you want to select one of two expressions, consider using the conditional operator (exp ? true_exp : false_exp).

  6. Try to keep to a consistent coding-style, it's a powerful tool for making code more readable:

    • Do single statements need their own block or not?
    • Don't add a new-line after opening a new block. There's a spurious one directly at the start of main().
  7. Use a for-loop instead of emulating it with a while-loop.

  8. You know you handle ordinals 0 to 126, while ASCII goes to 127? No matter, 127 is a control-character, "DEL".

  9. You need to include the headers for isprint(), isspace(), and getchar(). Because you use them without std::-prefix, you need <stdio.h> and <ctype.h>, not <cstdio> and <cctype>. Still, using the latter and adding the prefix arguably leads to slightly more consistent code.

  10. Input might be line-buffered, so "Strike any key to continue..." might actually wait for enter. Also, consider being consistent and using std::cin.get() instead.

  11. return 0; is implicit for main().

\$\endgroup\$
  • 1
    \$\begingroup\$ For point 9, better would be to use the std:: prefix rather than use the C-style include. Either would technically work, but beginners should learn the right way from the beginning. \$\endgroup\$ – Edward Aug 11 '17 at 2:12
  • \$\begingroup\$ I disagree with point 4: it's reasonably to write a function with the same constraints as isspace() &c, if you document it so - accept input as integers representing unsigned character codes. \$\endgroup\$ – Toby Speight Aug 11 '17 at 7:55
  • \$\begingroup\$ I meant "reasonable", not "reasonably" (sorry!). Instead of casting to unsigned char, you can write some_function(int c) and require that c must be representable as unsigned char or be equal to EOF. Just my opinion, and you're entitled to disagree! \$\endgroup\$ – Toby Speight Aug 11 '17 at 9:22
  • \$\begingroup\$ @TobySpeight Ok, now I understand, and you are right with that. Clarified that what I actually meant is the case where a char's value is outside the range of unsigned char. \$\endgroup\$ – Deduplicator Aug 11 '17 at 9:29
  • \$\begingroup\$ Thinking further, it might make sense to reject out-of-range input rather than casting it unconditionally. Consider passing 0x1020 on a platform with 8-bit char; it's probably better to treat that as not printable rather than isprint(0x20). Perhaps something like my_isprint(int c) { return (unsigned char)c == c && std::isprint(c); } would be more correct? \$\endgroup\$ – Toby Speight Aug 11 '17 at 9:35
6
\$\begingroup\$

It's usually advisable to avoid printing inside of simple functions. Functions are easiest to test and use when instead of doing something with their result, they return their results and let the caller do whatever they want with them. You have to print somewhere, but make sure the place you're printing from makes sense.

What if in the future you wanted to use your function to format a string into its codes, but don't want to print the codes? What if you want to actually use them?

Prefer returning results to carrying out side effects with them. If you want to print the results, return them from the function, and print them at the call site. For your code here, you may want to change your printing function to a function that just formats the character. Then, you can cout the return from the function. This still removes the "deciding" code from the main, which can help readability.

This might look something like:

std::string formatChar(char symbol, int code) {
    std::string ws = "White Space";

    // Create a string stream to assist concatenation
    std::stringstream ss;

    // Add strings and numbers to the stream. 
    ss << "CODE: " << code << " SYMBOL: ";

    // Decide what string to add, and add it. 
    if (isspace(code)) {
        ss << ws << "\n";

    } else {
        ss << symbol << "\n";
    } 

    // Turn the string stream into a String, and return it. 
    return ss.str();
}

Then, change your main loop to:

while (code < char_range) {
        if (isprint(code)) {
            std::cout << formatChar(symbol, code);
        }
        symbol += 1;
        code = symbol;
    }

The benefits won't be clear here, but this will make your life significantly easier once you begin working on larger projects. Unfortunately the need for the string stream here kind of convoluted the point, but it's still something that should at least be kept in mind.

\$\endgroup\$
  • \$\begingroup\$ I really like the stringstream idea, I've never used or seen that before \$\endgroup\$ – Anonymous3.1415 Aug 10 '17 at 22:52
  • 1
    \$\begingroup\$ @Anonymous3.1415 They're super handy for making strings. Idk if you know Java, but they're essentially a StringBuilder with nicer syntax. Note, since cout, ss and any other out stream use the insertion operator (<<), you could, if you wanted, pass in any stream (including cout and stringstreams), and insert into it. Then, if you wanted the function to print, you'd give it cout. If you want it to return a string, you give it a stringstream. That would be gross overkill for a situation like this, but it's a neat idea. \$\endgroup\$ – Carcigenicate Aug 10 '17 at 22:58
  • \$\begingroup\$ Very cool idea, I'll make note of that fact when my programs start getting more sophisticated down the line. \$\endgroup\$ – Anonymous3.1415 Aug 10 '17 at 23:02
2
\$\begingroup\$

The name of printChart() misled me for a while. Its purpose is to print a single line of the chart, so perhaps it should be called printLine() (or even createLine() or similar, if you accept Carcigenicate's suggestion). Good names make a huge difference in the comprehension of your code, but inventing them is one of the hardest problems in programming!

Inside the function, there's std::string ws that is constructed on every call, whether it's used or not, and always with the same contents. This is obviously wasteful. One way to reduce the overhead is to to make it static so that it's only initialised once - it's worth making it const as well so you can be sure its value never changes:

    static const std::string ws = "White Space";

Looking at how we use it (streaming it to output), there's no benefit to std::string over an ordinary (null-terminated, C-style) string:

    static const char *const ws = "White Space";

(The two appearances of const on different sides of the * indicate that this is a constant pointer to constant characters - you can't modify ws and you can't modify ws[i]).

Alternatively, as an array:

    static const char ws[] = "White Space";

We can also reduce its scope, as it's used only in a small part of the code:

    if (isspace(code)) {
        static const char ws[] = "White Space";
        std::cout << ws << "\n";
    } else {
        std::cout << symbol << "\n";
    }

At this point, it's probably clearer to inline the one and only use, and not give it a separate name:

    if (isspace(code)) {
        std::cout << "White Space\n";
    } else {
        std::cout << symbol << "\n";
    }

(I agree with most of the suggestions in other answers; no need to duplicate them here).

\$\endgroup\$
0
\$\begingroup\$

UPDATE: I took into suggestion some of the ideas and re wrote the program to be "simpler". I realized that the program task itself being very simple made the use of a user defined function unnecessary. Let me know what you think of the code and if there is anything I may have missed.

//*****************EDITED VERSION********************
//Print out the ASCII chart with its corisponding code
#include <iostream>
#include <cstdio>
#include <cctype>

int main() {

    char symbol = 0;
    int char_range = 128;

    for (int i = 0; i < char_range; i++, symbol++) {
        std::cout << "CODE " << (int)symbol;
        if (std::isspace(symbol)) {
            std::cout << " SYMBOL: WHITE SPACE\n";
        }
        else {
            if (std::iscntrl(symbol)) {
                std::cout << " SYMBOL: CONTROL\n";
            } 
            else {
                std::cout << " SYMBOL: " << symbol << "\n";
                }
            }
        }

    std::cout << "Strike Enter to continue...";
    std::cin.get();

    return 0;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.