ok code?

Started by
6 comments, last by Nanook 17 years, 6 months ago
My book told me to make a program that first reads strings into a vector. Then I was gona output how many strings there where and how many times each word apears. The code below is how I did it.. is this ok? or is it another easyer way to do it?


#include "read.h"
#include <iostream>
#include <vector>
#include <string>

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

int main()
{
    //ask for,read and stores the words in vector words.
    cout << "Pleas enter some words. Press enter after each word and when you"
         << " are finnished you enter end-of-file: " << endl;
    vector<string> words;
    read(cin,words);
    
    //outputs how many words there are.
    cout << "The number of words are: " << words.size() << endl << endl;

    //counts how many times one word apear.
    typedef vector<string>::size_type vec_sz;
    vector<string> counted_words;    

    for (vec_sz i = 0;i != words.size();++i){

        //check if the words is counted allready
        bool counted = 0;
        for (vec_sz j = 0;j != counted_words.size();++j){
            if (words == counted_words[j]){
               counted = 1;
            }
        }

        //if its not counted count it.    
        if (!counted){
           counted_words.push_back(words);
           int count = 0;
           for (vec_sz j = 0;j != words.size();++j){
               if (words == words[j])
                  count++;
           }
           cout << words << " Appear's " << count << " times." << endl;
        }
    }
}



The read() function reads from cin into a vector..
Advertisement
Quote:
for (vec_sz i = 0;i != words.size();++i){

Have your book introduced you to iterators yet? When looping through a vector you should use iterators.

Quote:bool counted = 0;

You should use true and false, not 0 and 1. We want to be as expressive as possible, and 0 and 1 express that counted is an integer, not a boolean, people might think it is a counter to see how many words have been counted.

Quote:
if (words == counted_words[j]){               counted = 1;            }

You should break out of the loop when the condition is meet, it will never be encountered again, and if it would it wouldn't make a difference since it would do the same thing.

Quote:
int count = 0;           for (vec_sz j = 0;j != words.size();++j){               if (words == words[j])                  count++;           }           cout << words << " Appear's " << count << " times." << endl;

This seems to be in the wrong place, you should place it after you're done counting.

We need to know at what level you're at before we can make suggestions, for example could you use std::find to search the vectors?
nope.. not into iterators or std::find yet..

what do you mean that its in the wrong place? it is the counting..

thx :)
Quote:Original post by Nanook
nope.. not into iterators or std::find yet..

what do you mean that its in the wrong place? it is the counting..

thx :)


Functionality wise, it's correct, but you should try to seperate functionality so, your code will only get more confussing if you at some point need to add something. You should instead in the first loop not do anything, but filling up counted_words. Then you should create a new loop going through all elements of counted_words, and count the number of words for each of these. Since this is just a "did you understand it" excersize, which it definitely look like you did, then don't worry, you'll learn about this stuff later on just move on, your code is quite nice considering the experience you have.
hehe ok.. thx.. good to hear.. :) I forgot to ask how I quit the loop when the if statment is true..
As CTar has already pointed out this is a good attempt. But you should know that the c++ standard library is huge, and there are a lot of tools/utilities it provides that can make things much easier. When creating an algorithm, usually the underlying data structure plays a big factor in how effective the algorithm is.

Unfortuanetly you havent gone though iterators yet, but iterators is a concept from the standard library that defines how to access elements inside a standard container. For example your "vector" is one of the standard containers. It is also the structure you are using to perform your string counting algorithm.

There is one container called "map". What it does, it maps keys to values. Using a map would make things much easier. Because if you treated the strings as "keys" and had an integer in the value spot, then conting the number of strings in your vector becomes next to natural.

map[key]++;

The above line, will increment the "value" that "key" is stored at. So for example you do this:

map["hello"]++;
map["byebye"]++;
map["hello"]++;

you'll end up with map["hello"] having a value of 2 and map["byebye"] having a value of 1. This is of course assuming that the values themselves are initialized at 0 before you plusplus them.

I suggest toying around with std::map to count your strings instead of the current method you are using now. Just for learning purposes if nothing else. Iterators are not that difficult of a subject to learn about, at least not the basics.

There're always tools within the standard library that people don't know about and can be used in various situations. For example your entire main can probably be reduced to something similar to this (if not more or less this) by making use of the standard library.

std::vector<std::string> v;std::map<std::string, int> m;// this would read in from the input stream and store the values in vstd::copy( std::istream_iterator<std::string>(std::cin),            std::istream_iterator<std::string>(),            std::back_inserter(v)            );// this can apply a function to store each vector element in the map containerfor_each( v.begin(), v.end(), MapCountFunctor() );// this will print out each element of the map. Since a map is a key/value // pair, you can print in the format "<map.key> was typed <map.value> times."for_each( m.begin(), m.end(), MapDispplayFunctor() );


But this will come much later of course after you've spent time in the guts of what c++ already provides with you. For now I think changing your code above to use an std::map will add a lot to you skill-base.

[offtopic]
i was in a chatty mood :)
[/offtopic]
[size=2]aliak.net
hey a break statment will quite the loop when its executed and continue will go to the next iteration of the loop.
Yeah.. I guess the map thingy is the way to go.. but I think I've learned what the book wanted me to.. so I'll guess I'll learn in a later chapter.. thanks a bunch to all of you..

This topic is closed to new replies.

Advertisement