4
\$\begingroup\$

I have been programming in C++ for around a month now, just long enough, I figure, to develop bad habits. Can anyone point out where I'm making mistakes and offer topics I should investigate to understand why?

#include <iostream>
#include <cassert>
#include <sstream>
#include <string>

using namespace std;
string get_line();
int long get_integer();
int* flex(long int num);
void test_flex();

int main()
{ 
    //flex(get_integer());
    test_flex();
}

/* 
 * ===  FUNCTION  =============================================================
 *         Name:  flex
 *  Description:  flex takes one integer argument and returns an then number
 *  of odd, even and zero digits.
 *  flex(111022) -> [3,2,1] i.e [odd,even,zero's]
 * ============================================================================
 */
    int*
flex (long int num)
{
    int res;
    int even = 0;
    int odd = 0;
    int zero = 0;

    while(num != 0){
        res = num % 10;
        if(res == 0){
            zero++;
        }
        else if(res % 2 == 0){
            even++;
        }
        else{
            odd++;
        }
        num = (num - (num % 10))/10;
    }
    int oez[3] = {odd,even,zero};
    return oez;
}       /* -----  end of function flex  ----- */

/* 
 * ===  FUNCTION  =============================================================
 *         Name:  test_flex
 *  Description:  test suite for flex function
 * ============================================================================
 */
    void
test_flex ( )
{
    assert(flex(1)[0] == 1); 
    assert(flex(1)[1] == 0);
    assert(flex(1)[2] == 0);

    assert(flex(112220)[0] == 2);
    assert(flex(112220)[1] == 3);
    assert(flex(112220)[2] == 1);

}       /* -----  end of function test_flex  ----- */

/* 
 * ===  FUNCTION  ======================================================================
 *         Name:  get_integer
 *  Description:  checks if input is integer
 * =====================================================================================
 */
    long int
get_integer ()
{
    while(true){
        stringstream converter;
        converter << get_line();

        int long result;
        if(converter >> result){
            char remaining;
            if(converter >> remaining)
                cout << "Unexpected character: " << remaining << endl;
            else{
                return result;
            }
        } else
            cout << "Please enter an integer." << endl;

        cout << "Retry:" << endl;
    }
}       /* -----  end of function get_integer  ----- */


/* 
 * ===  FUNCTION  ======================================================================
 *         Name:  get_line
 *  Description:  wraps getline
 * =====================================================================================
 */
    string
get_line()
{
    string result;
    getline(cin,result);
    return result;
}       /* -----  end of function get_line  ----- */
\$\endgroup\$

2 Answers 2

8
\$\begingroup\$

Do not see the point in: get_line()

get_line(line);

// Vs

std::getline(std::cin, line);

Get Integer is well done. Most people forget to test if there is anything else remaining on the line.

A simple enhancement is:

        char remaining;
        if(converter >> remaining)

        // easier to rewrite as:
        if (!converter.str().empty())

Also there is already a boost function that does something similar:

long value = boost::lexical_cast<long>(line); // Will throw if anything left on line.

Over complicating this:

    num = (num - (num % 10))/10;

You can simplify it too:

    num /= 10;  // Integer division truncates.

I think this is a logical error:

    if(res == 0){
        zero++;
    }
    else if(res % 2 == 0){ // zero is an even number
                           // do you really want that else?

Your one major error is:

int oez[3] = {odd,even,zero};
return oez;

You are returning a pointer to an array that has gone out of scope. Personally I would return a std::vector<int>. Don't worry on a simple returning like this the copy back of the array will be optimized out (look up RVO and NVRO).

std::vector<int> oez = {odd,even,zero};
return oez;
\$\endgroup\$
7
\$\begingroup\$

I presume your code's brace format and function heading format is dictated by instructor and I won't comment on it. The code is presented clearly enough. One comment says problem 8, a couple of others say 10.

I deem it inappropriate to have a get_line routine; for code that would take 2 lines inline and be perfectly clear, you have about 13 lines of subroutine with no obvious reason for same.

The digit-counting code looks rather verbose to me, with tests like num != 0 and tests in awkward order. I'd rewrite the digit-counting code as follows.

  while (num) {
    res = num % 10;
    if (res)
      if (res & 1)
        ++odd;
      else
        ++even;
    else
      ++zero;
    num /= 10;
  }
\$\endgroup\$
1
  • \$\begingroup\$ Yea, i don't know why i made get_line its own function. It's also now obvious to me that num - (num % 10), isn't doing anything. My instructor hasn't dictated anything. I'm bouncing between different styles i see in different texts and what i was comfortable with in python... \$\endgroup\$ Oct 15, 2012 at 4:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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