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;
}
4 Answers 4
Sure, it works, if the host-characterset extends ASCII, thus a good start. And naturally, we have loads of ideas.
printChart()
receives the same input twice, using different types. Duplication is never good. Just pass it once and convert as needed inside.printChart()
always outputs tostd::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 (typestd::ostream&
).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 ofws
, or at least change its type toconst char*
.There's no reason not to handle
char
-values outside theunsigned char
-range right if you get them as achar
: Make sure to cast tounsigned char
before passing to character-classification-functions likeisspace
andisprint
.If you want to select one of two expressions, consider using the conditional operator (
exp ? true_exp : false_exp
).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()
.
Use a
for
-loop instead of emulating it with awhile
-loop.You know you handle ordinals 0 to 126, while ASCII goes to 127? No matter, 127 is a control-character, "DEL".
You need to include the headers for
isprint()
,isspace()
, andgetchar()
. Because you use them withoutstd::
-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.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.return 0;
is implicit formain()
.
-
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– Edward2017年08月11日 02:12:23 +00:00Commented Aug 11, 2017 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– Toby Speight2017年08月11日 07:55:38 +00:00Commented Aug 11, 2017 at 7:55 -
\$\begingroup\$ I meant "reasonable", not "reasonably" (sorry!). Instead of casting to
unsigned char
, you can writesome_function(int c)
and require thatc
must be representable asunsigned char
or be equal toEOF
. Just my opinion, and you're entitled to disagree! \$\endgroup\$Toby Speight– Toby Speight2017年08月11日 09:22:29 +00:00Commented Aug 11, 2017 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 ofunsigned char
. \$\endgroup\$Deduplicator– Deduplicator2017年08月11日 09:29:36 +00:00Commented Aug 11, 2017 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-bitchar
; it's probably better to treat that as not printable rather thanisprint(0x20)
. Perhaps something likemy_isprint(int c) { return (unsigned char)c == c && std::isprint(c); }
would be more correct? \$\endgroup\$Toby Speight– Toby Speight2017年08月11日 09:35:49 +00:00Commented Aug 11, 2017 at 9:35
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.
-
\$\begingroup\$ I really like the stringstream idea, I've never used or seen that before \$\endgroup\$Anonymous3.1415– Anonymous3.14152017年08月10日 22:52:36 +00:00Commented Aug 10, 2017 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, sincecout
,ss
and any other out stream use the insertion operator (<<
), you could, if you wanted, pass in any stream (includingcout
and stringstreams), and insert into it. Then, if you wanted the function to print, you'd give itcout
. 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– Carcigenicate2017年08月10日 22:58:18 +00:00Commented Aug 10, 2017 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– Anonymous3.14152017年08月10日 23:02:49 +00:00Commented Aug 10, 2017 at 23:02
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).
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;
}
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\$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\$