Прошу ревью кода (С++, stl, ~140 строк)

Question

[datalink]

Написал реализацию графа в виде шаблона с использованием стандартных контейнеров. Ссылка.

Попинайте, пожалуйста, за возможные ошибки. Спасибо!

Пример использования

#include <iostream>
#include "graph.h"

using namespace std;

const int VERTICE_COUNT = 3;

int main()
{
    Graph<int> G(VERTICE_COUNT);
    for(int i = 0; i < VERTICE_COUNT; i++)
    {
        G.addVertice( ::make_shared<int>(i) );
    }
    for(int i = 1; i < VERTICE_COUNT; i++)
    {
        G.connect(0, i, i);
    }
    ::cout << G;
    return 0;
}

Answer 1

[Андрей Кравчук]

Давно с C++ дел не имел, но вроде у вас перегруженный оператор вывода в поток с ошибкой. Он всегда выводит в cout, даже если левый опреанд будет файлом или еще каким потоком.

Answer 2

[Mezomish]

1. Строка 43: вы возвращаете «сырой» указатель, а не shared pointer — так и задумано?

2. Общее замечание: в единственном числе «вершина» будет «vertex».

3. Не реализован метод удаления вершины.

Answer 3

[xanep]

Кроме озвученного выше, еще добавлю
— Из конструктора можете смело выкидывать capacity.
— Переменные классы лучше именовать так, чтоб отличались от локальных — mVertices, vertices_, _vertices… whatever.
— Vertice* vertice(int index) const — это плохой метод. Константный метод, возвращающий неконстантный указатель — это явно что-то не то. Думаю, вам нужно возвращать значение, либо константную ссылку.

Comments

[xanep]

«Переменные классы» следует читать «переменные класса».

Answer 4

[AxisPod]

— Проблемы с именованием, используете C++, stl, так и именуйте в стиле stl, давайте методам осмысленные имена.
— Не забывайте о typedef для value_type и подобных
— Зачем тип вершины заворачивать в shared_ptr? Это приведет к оверхеду, это приведет к снижению производительности
— vertices.at(x); используется совсем не по назначению, если надо кинуть out_of_range, так и проверяйте с размером и кидайте руками.
— Как написано выше operator<< выводить должен в stream, а не std::cout
— Vertice* vertice(int index) const необходимо возвращать ссылку, либо кидать исключение out_of_range, а не указатель.

Comments

[AxisPod]

Ну и возьмите boost.graph )

Answer 5

[Razario777]

В детали не вдавался, то что бросилось в глаза сразу:

if(x > y)
            return std::make_pair(y, x);
        else
            return std::make_pair(x, y);

заменить на

return x > y ? std::make_pair(y, x) : std::make_pair(x, y);

или на

return std::make_pair(x > y ? y : x, x > y ? x : y);

Так же кое где пишите auto, где то явно указываете например int? Должна быть единая стилистика.

Comments

[wander]

Чем тернарный оператор лучше в данном случае? Тем, что записан в одну строку? Это чистый субъективизм. Хотя, если честно, мне тоже импонирует вариант с ним, но я абсолютно точно знаю, что это лишь мое субъективное восприятие. Эффективности такая запись не добавит точно.

Насчет auto и единой стилистики — скорее вредный совет, чем полезный. С++ пока еще статически типизированный язык, потому сокрытие всех типов за auto ведет в снижению читаемости кода. Там, где тип сложен в записи, но в тоже время для понимания кода не нужно знать конкретное выражение типа, auto подходит. В остальных случаях лучше писать типы явно.