Читая последовательность точек и найти ближайший к первому


Мне сказали, что это место, чтобы приехать для ввода/обратной связи на вашем программирования. Я всегда ищу, чтобы улучшить либо личные комментарий или мнение/помощь. Бы здесь сообщество быть готовы предоставить конструктивную критику моего скрипта ниже?

/********************************************************************
 * Author: Mattew Hoggan
 * Description: Write a client program that uses the data type point.
 * Read a sequence of points (pairs of floating-point numbers) from
 * standard input, and find the one that is closest to the first.
 * *****************************************************************/

#include <math.h>                                                  
#include <iostream>                                                
#include <vector>                                                  
#include <istream>                                                 
#include <iterator>
#include <algorithm>
#include <limits.h>

using namespace std;                                               

typedef struct point {                                             
    float x;                                                       
    float y;                                                       
} point;                                                           

float calc_distance( const point *a, const point *b ) {            
    return sqrt( pow( a->x-b->x, 2.0 ) + pow( a->y-b->y, 2.0 ) );      
}                                                                  

void print_point( point *a ) {                                 
    cout << "(" << a->x << ", " << a->y << ") ";
}

void delet_point( point *a ) {
    delete a;
}

vector<point*>* form_pairs( vector<float> &num ) {
    vector<point*> *points=NULL;
    if( ( num.size( ) & 1 ) == 1 ) { 
        cerr << "ERROR: You input: " << num.size( ) 
             << " which is odd, failed to build vector "
             << endl;
        return points;
    } else {
        cout << "Going to build points" << endl;
        points = new vector<point*>;
        for( vector<float>::iterator vit=num.begin( );
            vit!=num.end( ); vit+=2 ) {
            point *in = new point( );
            in->x = *(vit);
            in->y = *(vit+1);
            points->push_back( in );     
        }   
        return points;
    }
}

void pop_front( vector<point*> *pairs ) {
    reverse( pairs->begin( ), pairs->end( ) );
    pairs->pop_back( );
    reverse( pairs->begin( ), pairs->end( ) );
}

void min_euclidean_distance( vector<point*> *pairs ) {
    if( pairs->size( ) == 1 ) {
        cerr << "You already know the answer to this" << endl;
        return;
    }
    point *first = pairs->front( );
    pop_front( pairs );
    point *second = pairs->front( );
    pop_front( pairs );

    for_each( pairs->begin( ),pairs->end( ),print_point );
    cout << endl;

    point *p_min = second;
    float f_min = calc_distance( first,second );
    for( vector<point*>::iterator pit = pairs->begin( );
        pit != pairs->end( ); pit++ ) {
        float tmp = calc_distance( first,(*pit) );
        if( tmp < f_min ) {
            f_min = tmp;
            p_min = (*pit);
        }
    }

    cout << "The closest node to "; print_point( first );
    cout << " is "; print_point( p_min );
    cout << " at " << f_min << " units away " << endl;
    delete first;
    delete second;
}

int main( int arc, char **arv ) {                                
    vector<float> num;
    cout << "Please, input even number of floats: ";
    for( istream_iterator<float> iit (cin);
         iit!=istream_iterator<float>( );iit++ ) {
         num.push_back( (*iit) );
    }

    vector<point*>* pairs = form_pairs( num );
    if( pairs ) {
        min_euclidean_distance( pairs );
        for_each( pairs->begin( ),pairs->end( ),delet_point );
        delete pairs;
    }
}


525
2
задан 31 июля 2011 в 04:07 Источник Поделиться
Комментарии
1 ответ

Пару замечаний.

1) Почему это отдельно стоящая функция, а не член?

float calc_distance( const point *a, const point *b )

Его доступа к членам, которые должны потенциально быть частным. Лучше сделать его членом.
Также почему вы проходя мимо указателя. Лучше передавать по константной.

2) Вы используете указатели, где вы должны использовать объекты.

vector<point*>* form_pairs( vector<float> &num )

Сырые указатели не имеют семантики собственности, связанных с ними. Таким образом, неясно, кто должен очистить их. В этом случае вы убираете затем вверх, но не в исключение безопасным способом. В код на C++ это вообще ошибкой было каких-либо указателей в коде (указатели должны быть завернуты в смарт-указатели или контейнеры).

В этом случае вы должны просто простых векторных объектов, и вы должны возвращать ссылку. Я знаю, что это выглядит дорого скопировать обратно в вектор, но РВО и NRVO позаботится об этом и, как правило, вектор построен на месте в пункт назначения и копии не требуется.

В качестве вектора число не изменяется. Вы должны передать его по константной ссылке.

std::vector<point>   form_pairs(vector<float> const& num)

3) Что вы пытаетесь получения здесь?

if( ( num.size( ) & 1 ) == 1 )

Вы пытаетесь проверить, что входной сигнал имеет четное число поплавков в качестве входных данных.
Почему ты пытаешься сделать что то загадочное. Если вы должны сделать это, то прокомментировать намерение. Код будет в эксплуатации гораздо дольше, чем требуется, чтобы развиваться. Убедитесь, что вы сделать его легко понять.

5) Вы делаете слишком много работы здесь:

    for( vector<float>::iterator vit=num.begin( );
vit!=num.end( ); vit+=2 )

Здесь вы могли бы использовать стандартный алгоритм. Я бы использовал функции std::копия(). Также с помощью += 2 вы ограничиваете себя итераторами произвольного доступа , т. е. векторы. Вы код должны попробовать и быть нейтральным по типу контейнера. Таким образом ограничьтесь оператор++.

6) динамически выделять указателем.

        point *in = new point( );

а) почему бы не иметь конструктор, который принимает два параметра для выполнения инициализации.
б) кому принадлежит объект. Владельцем является лицо, ответственное за удаление объекта.
C) Если вы были сохранены нормальные точечные объекты в векторе это не было бы проблемой.

7) это просто немного гнида придирчивые:

int main( int arc, char **arv )

Технически агду должен быть объявлен тип char* аргумент argv[].

8) опять слишком много работы:

vector<float> num;
for( istream_iterator<float> iit (cin);
iit!=istream_iterator<float>( );iit++ ) {
num.push_back( (*iit) );
}

Это может быть достигнуто при:

std::copy(std::istream_iterator<float>(std::cin),
std::istream_iterator<float>(),
std::back_inserter(num));

Я бы написал так:
Простота-это ключ!

#include <iostream>
#include <fstream>
#include <cmath>

class Point
{
public:
float distance(Point const& rhs) const
{
float dx = x - rhs.x;
float dy = y - rhs.y;

return sqrt( dx * dx + dy * dy);
}
private:
float x;
float y;
friend std::istream& operator>>(std::istream& stream, Point& point);
friend std::ostream& operator<<(std::ostream& stream, Point const& point);
};

Редактировать

Добавлены комментарии о том, как операторы трансляций работу, основываясь на комментариях ниже.

// This function reads a point from a stream
// It returns the stream object thus allowing chaining.
// std::cin >> p1 >> p2 >> p3 >> etc;
//
// The side affect of returning the stream is that it can also
// be used in conditionals. The stream will auto covert into a tyep
// that can be used in a boolean context which is `true` if the stream
// is still in a good state and `false` if the read failed.
// while(std::cin >> point)
// {
// // Successfully read a point from the stream.
// }
//
std::istream& operator>>(std::istream& stream, Point& point)
{
return stream >> point.x >> point.y;
}

// This function writes a point to a stream
// This just returns the stream object
std::ostream& operator<<(std::ostream& stream, Point const& point)
{
return stream << point.x << " " << point.y << " ";

// The above is just a shorthand way of writting this:
stream << point.x << " " << point.y << " ";
return stream;

// This is because the result of the operator >> and operator <<
// Is always the stream passed as the first parameter.
// Or historically this is what you are supposed to return to prevent confusion.
}

Это делает основной цикл очень легко писать:

int main()
{
std::ifstream data("Plop");

// Trying to find the closest point to this.
Point first;
data >> first;

// The next point is the closest until we find a better one
Point closest;
data >> closest;

float bestDistance = first.distance(closest);

Point next;
while(data >> next) // read a point. If if fails loop not entered.
{
float nextDistance = first.distance(next);
if (nextDistance < bestDistance)
{
bestDistance = nextDistance;
closest = next;
}
}

std::cout << "First(" << first << ") Closest(" << closest << ")\n";
}

5
ответ дан 31 июля 2011 в 05:07 Источник Поделиться