7
\$\begingroup\$

I have a school assignment and would love it if you can criticize or give a couple of pointers/advice about my piece of code (don't give me the answer, breaks the point of the exercise).

enter image description here

Here's my work:

#include <iostream>
#include <iomanip> 

using std::cin;
using std::cout;
using std::endl;
using std::setw;

int main()
{
//Declaring our variables
int number; //Integer input from the user

cout << " Please enter an integer :\n"; //Will display text
cin >> number; //The user's input will now be stored in the variable

//These following lines will compose the ascii art which spells out "c++"


cout << number << number << number << number << number << number << number 
<< number << number << number << number << endl; //line 1

cout << number << number << number << setw(17) << number << number << 
setw(17) << number << number << endl; // line 2

cout << number << number << number << setw(17) << number << number << 
setw(17) << number << number << endl; // line 3

cout << number << number << number << setw(12) << number << number << number 
<< number << number << number 
<< number << number << number << number << number << setw(9) << number << 
number << number << number
<< number << number << number << number << number << number << number << 
endl; //line4

cout << number << number << number << setw(17) << number << number << 
setw(17) << number << number << endl; // line 5

cout << number << number << number << setw(17) << number << number << 
setw(17) << number << number << endl; // line 6

cout << number << number << number << number << number << number << number 
<< number << number << number << number << endl; //line 7

return 0;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Does target platform use monospace font? If it does not, number of spaces might need tweaking to account for different digits having different widths \$\endgroup\$ – Daerdemandt Sep 24 '17 at 7:28
  • 2
    \$\begingroup\$ @Daerdemandt That’s how ascii-art works. I’ve never seen anyone attempt ASCII art with variable-width fonts. \$\endgroup\$ – 200_success Sep 24 '17 at 9:46
  • \$\begingroup\$ @200_success please look carefully at example image in the task. \$\endgroup\$ – Daerdemandt Sep 24 '17 at 11:42
  • 1
    \$\begingroup\$ @Daerdemandt Interesting. The two pluses aren’t even the same width: the last one is wider. \$\endgroup\$ – 200_success Sep 24 '17 at 11:46
  • \$\begingroup\$ Do we assume that user will input a single digit? What should we do otherwise? \$\endgroup\$ – Daerdemandt Sep 24 '17 at 12:54
5
\$\begingroup\$

Graphical representation

From your 'number << number << number' thing it is not immediately obvious what is being drawn, and how to fix it in a specific way when needed.

By using the template where you just replace placeholder characters with what you need, you get more WYSIWYG-like result.

Input validation

Your code assumes that user inputs appropriate value. What if user inputs 1234? (You can get more fancy and guard against inputting non-numbers too)

#include <iostream>

using std::cin;
using std::cout;
using std::endl;
using std::string;
using std::to_string;

const auto ASCII_template = string("\
SSSSSSSSSSS\n\
SSS               SS           SS\n\
SSS               SS           SS\n\
SSS           SSSSSSSSSS   SSSSSSSSSS\n\
SSS               SS           SS\n\
SSS               SS           SS\n\
SSSSSSSSSSS");


string replace_all(const string& base_string, const string& search, const string& replace) {
  string s = string(base_string);
  size_t pos = s.find(search);
  size_t len = replace.size();
  while ( pos != string::npos ) {
     s.replace( pos, len, replace );
     pos = s.find(search);
  }
  return s;
}

unsigned int get_user_digit() {
  unsigned int result;
  do {
    cout << "Please enter a single digit:" << endl;
    cin >> result;
  } while (result >= 10);
  return result;
}

int main() {
  cout << replace_all(ASCII_template, string("S"), to_string(get_user_digit())) << endl;
  return 0;
}

You can try it online here.

I don't remember C++ that much, but something along those lines would work. Depending on how much of С++11 you can use, std::regex_replace would simplify replace_all function. If you can not use std::string, it wouldn't change that much either, although you'd have to use some C-like workaround.

\$\endgroup\$
  • 2
    \$\begingroup\$ +1 This is a good solution, but two points: (1) you don't need line continuations in the pattern string, multiple string literals on adjacent lines are automatically concatenated, this might look clearer (2) replace_all could be implemented a bit more “modern“ with std::transform, maybe even in-place \$\endgroup\$ – Felix Dombek Sep 24 '17 at 23:03
  • \$\begingroup\$ @Thank you guys for helping me out. Sadly I can't really take your advice cause it's an introductory programming course. The TA will, of course, know that this wasn't my original idea. What I thought of doing was creating an if-else command. The program will refuse any number other than single digit numbers. \$\endgroup\$ – Gabriel K Sep 25 '17 at 1:25
  • 2
    \$\begingroup\$ Instead of that "kickstart the cycle" hack, you should use a do-while loop instead :P \$\endgroup\$ – Rakete1111 Sep 25 '17 at 4:41
  • \$\begingroup\$ @Rakete1111 wow that's embarassing:) Fixed \$\endgroup\$ – Daerdemandt Sep 26 '17 at 14:53
  • \$\begingroup\$ @FelixDombek (1) I'll still need "\n"s, so no biggie. (2) std::transform looks super-good indeed (although not available on repl.it). Still, replace_all works on substring level, not char level. It's an overkill, but more reusable and realistically I'll just copy-paste / import it from somewhere where I've already written it. As for in-place - personal preference against side-effects) \$\endgroup\$ – Daerdemandt Sep 26 '17 at 15:31
2
\$\begingroup\$
  1. Good thing you didn't use using namespace std;! :)
  2. I would indent the code in main, but maybe that's because you pasted the code here.
  3. Remove unnecessary comments:

    //Declaring our variables
    int number;
    

    Of course you're declaring a variable, you don't need to write it again.

  4. Use better names so you don't need comments:

    int number; //Integer input from the user
    

    Instead, why not use int user_input; or similar, that way, you don't need the comment. Or leave it be (but remove the comment).

  5. std::endl, along with outputting a newline, flushes the stream, which can be quite costly (although it doesn't matter for non-performance critical pieces of code like yours). Just use '\n' instead if you don't need the flush (on some platforms you might need it).

  6. Technically, you don't need to return 0;, as the compiler will do that for you if you omit it.

Those huge lists of std::cout's bother me a bit. It would be better if you could refactor it to a function. I would do it like this.

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

One thing about programming is also to reduce duplication.

So looking at the required output we can notice, that you have 3 unique lines of text.

So an idea would be to preconstruct these 3 lines of text. Maybe into string variables or make 3 functions returning these strings or 3 functions outputing theses varaibles or ... .

Then output the variables or call the functions in the required order.

\$\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.