Strings!!!

I just finished writing a simple program that outputs to the user the symbols used in a sentence. If you can look over it and see if there are places of improvements, that would be awesome. Thank you!

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
#include <iostream>
#include <string>
#include <vector>
using namespace std; 

/*Write a program that allows a user to type in tabular data similar to a CSV file, but instead of using commas a 
separtor, you should try to detect valid separators. First, let the user type in the line of tabular data. Then 
detect possible separator characters by looking through the input for non-number, non-letter, non-space characters. 
Find all of these chracters that appear on every single line, and display the user these characters to ask which 
one to use. For example, if you see input like this: Alex Allain, webmaster@cprogramming.com John Smith, 
john@nowhere.com. You should prompt the user to choose between comma, at sign, and period for the separtor. */

int main ()
{	
	vector<int>symbol_index (0); 
	string symbols = "~`!@#$%^&*()-_=+{[}]|:;?/>.<,'\\\"";
	string input = "jae.kim@jhykima, I like icecream & water, #coding.";
	cin >> input; 
	
	int j = 0; 
	for (int i = 0; i < symbols.length (); i++)
	{
		size_t found = input.find_first_of (symbols[i]);
		if (found != string::npos)
		{
			symbol_index.push_back (found);
			j++;    
		}	
	}

	for (int i = 0; i < j; i++)
	{
		cout << input[symbol_index[i]] << " "; 
	}
}

places of improvements
1
2
sring input = "jae.kim@jhykima, I like icecream & water, #coding.";
cin >> input; 
You initialize the string and then overwrite it immediately

using namespace std; That's a bad habit because it pollutes the global namespace.
If you really want to write cout, cin, string and vector like that you should only import std::cout, std::cin, std::string and std::vector, not the whole namespace.
As an alternative you could import the std::namespace in the main function, then it won't pollute the global namespace but only the scope of main.

You got lucky here, and you'll be lucky most of the time but there are several functions in the std::namespace that will shadow yours if you want to make your own version of it, for example if you want to write your own swap, sqrt or similar simple functions.

1
2
3
4
5
6
7
8
9
for (int i = 0; i < symbols.length (); i++)
	{
		size_t found = input.find_first_of (symbols[i]);
		if (found != string::npos)
		{
			symbol_index.push_back (found);
			j++;    
		}	
	}


std::string has a function called find_first_of where you can search the string for ANY of the characters.
You could just write symbols.find_first_of() and if found you could search for that character in input.

like that:
1
2
3
4
5
6
std::size_t found = 0;
do {
    found = input.find_first_of(symbols);
    if(found != std::string::npos)
        symbol_index.push_back(symbols[ symbols.find_first_of(input[found]) ]);
} while(found != std::string::npos);


This should give you a performance boost
Last edited on
Do you really want to save indices of symbols? Because your assigment says otherwise. It looks like you need to store characters themselves.
If some character happens to be in string twice, then you will output it twice. You need to remove duplicates (or use container which does not allows them).
Also there are many symbols which should be captured by your function, but missing from your symbols string. I suggest to go with formal definition from assigment:
non-number, non-letter, non-space characters
and use character classification functions from cctype: isalnum (if character a letter or digit) and isspace (is whitespace character).
And one more: you are using operator >> which stops at first whitespace. If you want to input string containing them, use getline

As a bonus: shortened code using algorithms and set:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
#include <algorithm>
#include <cctype>
#include <iostream>
#include <iterator>
#include <string>
#include <set>


int main ()
{
    std::string input;
    std::getline(std::cin, input);

    std::set<char> symbols;
    std::copy_if(input.begin(), input.end(), std::inserter(symbols, symbols.end()),
                 [](char c) { return !(isalnum(c) || isspace(c)); });
    for (char c: symbols)
        std::cout << c << " ";
}

Last edited on
@gamer2015

Thank you so much for your response. I will definitely start using the std:: now.

@MiiNiPaa

Thank you so much for your response as well. I do agree that I should have used isalnum and isspace. Thank you for your suggestions!
Topic archived. No new replies allowed.