Форум программистов, компьютерный форум CyberForum.ru

Покритикуйте код - C++

Восстановить пароль Регистрация
 
 
Рейтинг: Рейтинг темы: голосов - 46, средняя оценка - 4.80
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
02.10.2011, 23:28     Покритикуйте код #1
Есть класс Студенты (реализован через односвязный список), хотел бы услышать критику по поводу его улучшения, если кому не лень разбираться в столь поздний час Сам код естественно полностью рабочий и предупреждений тоже компилятор не выдаёт (если не считать в main в условии while, но там всё ок) так что хотелось бы услышать Ваши замечания если что можно сделать лучше.

Students.h
C++
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
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
#ifndef STUDENTS_H
#define STUDENTS_H
 
#include <string>
using std::string;
 
#include <iostream>
using std::cout;
using std::cerr;
 
#include <iomanip>
using std::setw;
using std::left;
using std::right;
 
#include <stdexcept>
using std::out_of_range;
 
class Students
{
    struct ListItem
    {
        string fullname;
        string group;
        int year;
        int average;
        ListItem *Next;
        ListItem(string ="",int =0,int =0,string ="",ListItem* =0);
    };
 
    ListItem *Head;
    ListItem *Tail;
    ListItem *Current; //указатель на текущий элемент
    int count; // всего элементов
 
public:
    class iterator
    {
        Students::ListItem* current;
 
        //функция для проверки что current инициализирован
        void correct(){if(!*this) throw Students::Exception("access denied");}
 
        public:
        string first(){correct();return current->fullname;}
        string second(){correct();return current->group;}
        int third(){correct();return current->year;}
        int four(){correct();return current->average;}
 
        iterator() {current=0;}
 
        iterator(const Students &right){*this=right;}
        iterator(Students::ListItem* right){*this=right;}
        iterator& operator=(Students &right);
        iterator operator=(Students::ListItem* right);
 
        iterator& operator++();
        iterator operator++(int);
        bool operator==(const iterator& right) const;
        bool operator!=(const iterator& right) const;
        iterator* operator*(){return this;}
        operator bool(){return current!=0;}
    };
 
    class Exception
    {
        string str;
        public:
        Exception(string data) :str(data){};
        string what(){return str;}
    };
 
 
 
    Students(): Head(0), Tail(0), Current(0), count(0) {}
    Students(string data, int y, int a, string g);
    ~Students(){this->deleteAll();}
 
 
    void addToTail(string data, int y, int a, string g);
    void addToHead(string data, int y, int a, string g);
 
 
    void deleteFromHead(bool mode=1);//1 - с предупреждением(исключением) если список пуст
    void deleteFromTail();
    void deleteAll();
 
    //сортировка, принимает функцию для сравнения элементов
    void sort(bool cmp(string&,string&));
 
    //методы возвращающие итераторы
    iterator begin(){return Head;}
    iterator end(){return iterator(0);}
};
 
 
#endif
Students.cpp
C++
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
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
#include "Students.h"
 
//////////////////////STUDENTS/////////////////
 
Students::ListItem::ListItem(string name,int y,int aver,string gr,ListItem* next)
{
    fullname=name;
    group=gr;
    average=(aver>=0 && aver<=100 ? aver : -1);
    year=(y>=1950 && y<=2012 ? y : -1);
 
    if(average==-1 || year==-1)
     throw Exception("wrong data");
    Next=next;
}
 
Students::Students(string data, int y, int a, string g)
{
    Head=Tail=new ListItem(data,y,a,g);
    Current=0;
    count=1;
}
 
 
void Students::addToTail(string data, int y, int a, string g)
{
    if (Head)
    {
        Tail->Next=new ListItem(data,y,a,g,0);
        Tail=Tail->Next;
        count++;
    }
    else
        Head=Tail=new ListItem(data,y,a,g);
}
 
 
void Students::addToHead(string data, int y, int a, string g)
{
    if (Head)
    {
        ListItem* temp=Head;
        Head=new ListItem(data,y,a,g);
        Head->Next=temp;
        count++;
    }
    else
        Head=Tail=new ListItem(data,y,a,g);
}
 
void Students::deleteFromHead(bool mode)
{
    if(Head->Next)
    {
        ListItem *temp=Head;
        Head=Head->Next;
        count--;
        delete temp;
    }
 
    else if (Head)
    {
        ListItem *temp=Head;
        Head=Tail=Current=0;
        delete temp;
        count=0;
    }
 
    else if(mode)
     throw Exception("list is empty");
}
 
void Students::deleteFromTail()
{
    if(Tail != Head)
    {
        ListItem *temp=Head;
        while(temp->Next!=Tail)
            temp=temp->Next;
 
    ListItem *toDelete=temp->Next;
    Tail=temp;
    temp->Next=0;
 
    delete toDelete;
    count--;
    }
 
    else if(Tail==0)
     throw Exception("list is empty");
 
    else
     Head=Tail=0;
}
 
 
void Students::deleteAll()
{
    while (Head)
     deleteFromHead(0);
 
    count=0;
}
 
void Students:: sort(bool cmp(string&,string&))
{
    ListItem* new_begin=NULL;
    ListItem* new_end=NULL;
    ListItem* sprev=NULL;
 
    for(ListItem *scur=this->Head;scur!=NULL;scur=this->Head)
    {
        ListItem *smin=NULL;
        ListItem *sminprev=scur;
        string min_name=scur->fullname;
        for(ListItem *gp=scur->Next;gp!=NULL;gp=gp->Next)
        {
            if(cmp(gp->fullname,min_name))
            {
                min_name=gp->fullname;
                smin=gp;
                sprev=sminprev;
            }
            sminprev=gp;
        }
        if(smin==NULL)
        {
            smin=scur;
        }
        else if(smin==scur->Next)
        {
            scur->Next=scur->Next->Next;
        }
        else
        {
            sprev->Next=smin->Next;
        }
        if(new_begin!=NULL)
        {
            new_end->Next=smin;
            new_end=smin;
        }
        else
        {
            new_begin=smin;
            new_end=smin;
        }
        if(smin==this->Head)
         this->Head=smin->Next;
    }
    this->Head=new_begin;
    this->Tail=new_end;
 
}
 
 
//////////////////////ITERATOR/////////////////
Students::iterator& Students::iterator::operator++()
{
    current=current->Next;
     return *this;
}
 
Students::iterator Students::iterator::operator++(int)
{
    iterator temp(*this);
    current=current->Next;
     return temp;
}
 
bool Students::iterator::operator==(const iterator& right) const
{
    return (current==right.current);
}
 
bool Students::iterator::operator!=(const iterator& right) const
{
    return !(*this==right);
}
 
Students::iterator Students::iterator::operator=(Students::ListItem* right)
{
    current=right;
    return *this;
}
 
Students::iterator& Students::iterator::operator=(Students &right)
{
    current=right.Current;
    return *this;
}
main.cpp
C++
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
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
#include <iostream>
using std::cout;
using std::cin;
using std::endl;
using std::string;
#include <windows.h>
#include <string>
#include "Students.h"
#include "StudentsIterator.h"
#include "algorithm"
using std::for_each;
 
 
bool cmp( string &str1, string &str2 )
{
    return strcmp( str1.c_str(), str2.c_str() ) < 0;
}
 
 
void print(Students::iterator *ita)
{
    cout<<setw(15)<<left<<ita->first()
     <<setw(7)<<ita->second()
     <<setw(5)<<ita->third()
     <<setw(7)<<ita->four()<<"\n";
 
}
 
Students::iterator findElement(string toFind, Students::iterator begin, Students::iterator end)
{
    for(;begin!=end;++begin)
    {
        if (begin.first().find(toFind)!=(size_t)-1)
         return begin;
    }
    //если ничего не найдено - вернуть итератор указывающий за конец списка ( на NULL)
    return 0;
}
 
 
int main()
{
    SetConsoleCP(1251);
    SetConsoleOutputCP(1251);
 
 
    Students test("Иванов Иван",1992,100,"КС-09");
    test.addToTail("ЛЛЛ ФФ",1991,100,"КС-09");
    test.addToTail("БББ ВВ",1992,100,"КС-09");
    test.addToHead("Иванов Василий",1993,100,"КС-08");
    test.addToHead("Иванов Петр", 1990,100,"КС-07");
    test.addToHead("ЯЯЯ ММ",1993,100,"КС-08");
    test.addToTail("ЮЮЮ ЛЛЛ",1994,100,"КС-07");
    test.sort(cmp);
 
    cout<<"All list(after sort):\n";
    for_each(test.begin(),test.end(),print);
    test.deleteFromHead();
    test.deleteFromTail();
    cout<<endl<<"After delFromHead + delFromTail:\n";
    for_each(test.begin(),test.end(),print);
 
    cout<<endl<<"All Students with name \"Иванов\" (searching from begin to end of list):\n";
    Students::iterator it=test.begin();
    while(it=findElement("Иванов",it,test.end()))
     print(*it++);
}
Комментариев только в коде совсем мало, но функции и переменные несут смысловую нагрузку так что надеюсь код не очень сложный в чтении получился.
После регистрации реклама в сообщениях будет скрыта и будут доступны все возможности форума.
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
02.10.2011, 23:39     Покритикуйте код #2
Gepar, поехали. Первое, что бросается в глаза:

C++
1
2
3
4
5
6
7
8
void print(Students::iterator *ita)
{
    cout<<setw(15)<<left<<ita->first()
     <<setw(7)<<ita->second()
     <<setw(5)<<ita->third()
     <<setw(7)<<ita->four()<<"\n";
 
}
А что, если я захочу вывести в файл через std::ofstream?

Я бы сделал так:

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
class ListItem
{
   // ...
   public:
   // ...
 
   friend std::ostream & operator<<( std::ostream &os, const ListItem &s ); 
};
 
// ...
 
std::ostream & operator<<( std::ostream &os, const Students::ListItem &s )
{
    // вывод
    os << s.fullname << " (" << s.group << ") " << year << "; avg = " << average;
    return os;
}
Jupiter
Каратель
Эксперт C++
6543 / 3963 / 226
Регистрация: 26.03.2010
Сообщений: 9,273
Записей в блоге: 1
Завершенные тесты: 2
02.10.2011, 23:40     Покритикуйте код #3
Цитата Сообщение от Gepar Посмотреть сообщение
bool cmp( string &str1, string &str2 )
C++
1
bool cmp(const string &str1, const string &str2 )
вместо strcmp - http://cplusplus.com/reference/string/string/compare/

Цитата Сообщение от Gepar Посмотреть сообщение
Head=Tail=0;
Цитата Сообщение от Gepar Посмотреть сообщение
ListItem* new_begin=NULL;
определись с использованием либо 0 либо NULL для указателей

Цитата Сообщение от Gepar Посмотреть сообщение
Exception(string data)
C++
1
Exception(const string& data)
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
02.10.2011, 23:47  [ТС]     Покритикуйте код #4
talis, это сейчас допишу как бонус, хотя итераторы изначально прикручены и через них печать сделана на пробу потому как препод сказал что мол в 5ой лабораторной к этому всему нужно будет прикрутить интерфейс, а там мол никаких потоков cout вам не будет и всё такое так что делайте так чтобы можно было выводить данные вне класса как-то.
Jupiter, константность поправил.

Добавлено через 1 минуту
Цитата Сообщение от Jupiter Посмотреть сообщение
Ну так бы не понадобилось передавать ссылку в функцию, а я хотел оставить этот "старый стиль" с передачей в класс функции, чисто для демонстрации преподавателю )
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
02.10.2011, 23:56     Покритикуйте код #5
C++
1
2
3
4
5
6
7
8
9
10
11
12
13
class iterator
    {
        Students::ListItem* current;
 
        //...
 
        public:
        string first(){correct();return current->fullname;}
        string second(){correct();return current->group;}
        int third(){correct();return current->year;}
        int four(){correct();return current->average;}
     
        //...
Ну кто же так делает... Сделайте класс CStudent:

C++
1
2
3
4
5
6
7
8
9
10
class CStudent
{
public: // вообще не public, но для примера
    string fullname;
    string group;
    int year;
    int average;
 
    Student(); // ....
};
В ListItem:

C++
1
2
3
4
5
6
7
class ListItem
{
public:
    CStudent student;
    ListItem * next;
    // .....
}
В iterator:

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
14
class iterator
{
   ListItem *ptr;
 
public:
   iterator( ListItem *_p = 0 ) : p(_p){};
   iterator( const iterator &it ) : p( it.p ){};
 
   //...
 
   CStudent & operator*(){ return ptr->student; }
 
   //...
}
И дальше доступ:

C++
1
2
3
4
5
6
7
8
9
10
11
Students::iterator it = lst.begin();
 
(*it).fullname = "Ivan Ivanov";
it++;
 
if( it != lst.end() )
{
   (*it).fullname = "Basil Pupkin";
}
 
//...
Добавлено через 7 минут
Итератор - это средство доступа к элементу списка. Элементом списка является CStudent. А всякие там подробности реализации в виде указателя на следующий элемент оставьте внутри класса, пользователю класса их знать не обязательно. Итератор при "разыменовывании" возвращает ссылку на CStudent, с которым пользователь уже и работает. CStudent сам обеспечивает себя функциями доступа к fullname, year и прочим полям. Это не забота итератора. Итератор занимается обходом списка и не должен вникать в реализацию CStudent. Он должен просто возвращать ссылку на него и всё.

А у вас итератор является интерфейсом доступа к студенту, а студент - это некая абстрактная сущность, которая весьма посредственно выделена из общей серой массы содержимого элемента списка. А что, если я захочу часть студентов загнать во внешний массив, или ещё что-то с ними сделать? Да элементарно, почему при изменении структуры студента я должен менять класс итератора списка?

Как говорил умный дядя Страуструп, если вы думаете об этом как об отдельной сущности, создайте для этого класс. Рассматривайте студента как одну, самостоятельную сущность, список - как другую, элемент списка (который содержит студента) - как третью, а итератор списка - как четвёртую. Чётко разграничьте полномочия.
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
03.10.2011, 00:14  [ТС]     Покритикуйте код #6
Цитата Сообщение от talis Посмотреть сообщение
Итератор при "разыменовывании" возвращает ссылку на CStudent
Меня в прошлой теме кто-то поравил что "разименованный" итератор должен возвращать *this, те себя и я чуть изменил оператор*, ну да сейчас попробую реализовать Ваши советы по поводу улучшения класса.

Добавлено через 4 минуты
Хотя насчёт доступа через итераторы на редактирование я вот не знаю должна ли быть такая возможность у меня, ведь так можно будет обходить проверки и редактировать напрямую список ...
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
03.10.2011, 00:16     Покритикуйте код #7
Смотрите. В верхней части - вся реализация, в нижней - доступ:

Покритикуйте код
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
03.10.2011, 00:19     Покритикуйте код #8
Цитата Сообщение от Gepar Посмотреть сообщение
Хотя насчёт доступа через итераторы на редактирование я вот не знаю должна ли быть такая возможность у меня, ведь так можно будет обходить проверки и редактировать напрямую список ...
Ага! Вот вы и попались. Действительно, почему итератор должен заботиться о том, как правильно редактировать объект и делать все проверки? У нас самообслуживание. Пусть CStudent сам о себе заботится, и сам решает, как себя редактировать. А то свалили, понимаешь, всю работу на итераторы - и они горбатятся, бедные

Добавлено через 1 минуту
А чтобы запретить редактирование - сделайте ещё class const_iterator, который при "разыменовывании" возвращает const CStudent & - и никто не сможет редактировать объект по ссылке.
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
03.10.2011, 00:44  [ТС]     Покритикуйте код #9
talis, ну в принципе Вы правы, после редактирования класс сейчас выглядит так:

Students.h
C++
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
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
#ifndef STUDENTS_H
#define STUDENTS_H
 
#include <string>
using std::string;
 
#include <iostream>
using std::cout;
using std::cerr;
 
#include <iomanip>
using std::setw;
using std::left;
using std::right;
 
#include <stdexcept>
using std::out_of_range;
 
struct CStudent
{
    string fullname;
    string group;
    int year;
    int average;
};
 
 
class Students
{
    struct ListItem
    {
        CStudent student;
        ListItem *Next;
        ListItem(string ="",int =0,int =0,string ="",ListItem* =0);
    };
 
    ListItem *Head;
    ListItem *Tail;
    ListItem *Current; //указатель на текущий элемент
    int count; // всего элементов
 
public:
    class iterator
    {
       ListItem* current;
 
        //функция для проверки что current инициализирован
        void correct(){if(!*this) throw Students::Exception("access denied");}
 
        public:
 
        iterator() {current=0;}
 
        iterator(const Students &right){*this=right;}
        iterator(ListItem* right){*this=right;}
        iterator operator=(ListItem* right);
 
        iterator& operator++();
        iterator operator++(int);
        bool operator==(const iterator& right) const;
        bool operator!=(const iterator& right) const;
        CStudent& operator*(){return current->student;}
        operator bool(){return current!=0;}
    };
 
    class Exception
    {
        string str;
        public:
        Exception(string data) :str(data){};
        string what(){return str;}
    };
 
 
 
    Students(): Head(0), Tail(0), Current(0), count(0) {}
    Students(string data, int y, int a, string g);
    ~Students(){this->deleteAll();}
 
 
    void addToTail(string data, int y, int a, string g);
    void addToHead(string data, int y, int a, string g);
 
 
    void deleteFromHead(bool mode=1);//1 - с предупреждением(исключением) если список пуст
    void deleteFromTail();
    void deleteAll();
 
    //сортировка, принимает функцию для сравнения элементов
    void sort(bool cmp(const string&,const string&));
 
    //методы возвращающие итераторы
    iterator begin(){return Head;}
    iterator end(){return iterator(0);}
};
 
 
#endif
Students.cpp (почти не менялся, немного переделан лишь конструктор с расчётом на инициализацию CStudent и всё)
C++
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
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
#include "Students.h"
 
//////////////////////STUDENTS/////////////////
 
Students::ListItem::ListItem(string name,int y,int aver,string gr,ListItem* next)
{
    student.fullname=name;
    student.group=gr;
    student.average=(aver>=0 && aver<=100 ? aver : -1);
    student.year=(y>=1950 && y<=2012 ? y : -1);
 
    if(student.average==-1 || student.year==-1)
     throw Exception("wrong data");
    Next=next;
}
 
Students::Students(string data, int y, int a, string g)
{
    Head=Tail=new ListItem(data,y,a,g);
    Current=0;
    count=1;
}
 
 
void Students::addToTail(string data, int y, int a, string g)
{
    if (Head)
    {
        Tail->Next=new ListItem(data,y,a,g,0);
        Tail=Tail->Next;
        count++;
    }
    else
        Head=Tail=new ListItem(data,y,a,g);
}
 
 
void Students::addToHead(string data, int y, int a, string g)
{
    if (Head)
    {
        ListItem* temp=Head;
        Head=new ListItem(data,y,a,g);
        Head->Next=temp;
        count++;
    }
    else
        Head=Tail=new ListItem(data,y,a,g);
}
 
void Students::deleteFromHead(bool mode)
{
    if(Head->Next)
    {
        ListItem *temp=Head;
        Head=Head->Next;
        count--;
        delete temp;
    }
 
    else if (Head)
    {
        ListItem *temp=Head;
        Head=Tail=Current=0;
        delete temp;
        count=0;
    }
 
    else if(mode)
     throw Exception("list is empty");
}
 
void Students::deleteFromTail()
{
    if(Tail != Head)
    {
        ListItem *temp=Head;
        while(temp->Next!=Tail)
            temp=temp->Next;
 
    ListItem *toDelete=temp->Next;
    Tail=temp;
    temp->Next=0;
 
    delete toDelete;
    count--;
    }
 
    else if(Tail==0)
     throw Exception("list is empty");
 
    else
     Head=Tail=0;
}
 
 
void Students::deleteAll()
{
    while (Head)
     deleteFromHead(0);
 
    count=0;
}
 
void Students:: sort(bool cmp(const string&,const string&))
{
    ListItem* new_begin=NULL;
    ListItem* new_end=NULL;
    ListItem* sprev=NULL;
 
    for(ListItem *scur=this->Head;scur!=NULL;scur=this->Head)
    {
        ListItem *smin=NULL;
        ListItem *sminprev=scur;
        string min_name=scur->student.fullname;
        for(ListItem *gp=scur->Next;gp!=NULL;gp=gp->Next)
        {
            if(cmp(gp->student.fullname,min_name))
            {
                min_name=gp->student.fullname;
                smin=gp;
                sprev=sminprev;
            }
            sminprev=gp;
        }
        if(smin==NULL)
        {
            smin=scur;
        }
        else if(smin==scur->Next)
        {
            scur->Next=scur->Next->Next;
        }
        else
        {
            sprev->Next=smin->Next;
        }
        if(new_begin!=NULL)
        {
            new_end->Next=smin;
            new_end=smin;
        }
        else
        {
            new_begin=smin;
            new_end=smin;
        }
        if(smin==this->Head)
         this->Head=smin->Next;
    }
    this->Head=new_begin;
    this->Tail=new_end;
 
}
 
 
//////////////////////ITERATOR/////////////////
Students::iterator& Students::iterator::operator++()
{
    current=current->Next;
     return *this;
}
 
Students::iterator Students::iterator::operator++(int)
{
    iterator temp(*this);
    current=current->Next;
     return temp;
}
 
bool Students::iterator::operator==(const iterator& right) const
{
    return (current==right.current);
}
 
bool Students::iterator::operator!=(const iterator& right) const
{
    return !(*this==right);
}
 
Students::iterator Students::iterator::operator=(Students::ListItem* right)
{
    current=right;
    return *this;
}
main (изменена функция печати и добавлено в конце изменение значения поля fullname через итератор):
C++
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
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
#include <iostream>
using std::cout;
using std::cin;
using std::endl;
using std::string;
#include <windows.h>
#include <string>
#include "Students.h"
#include "algorithm"
using std::for_each;
 
 
bool cmp(const string &str1, const string &str2 )
{
    return strcmp( str1.c_str(), str2.c_str() ) < 0;
};
 
 
void print(CStudent& stud)
{
    cout<<setw(15)<<left<<stud.fullname
     <<setw(7)<<stud.group
     <<setw(5)<<stud.year
     <<setw(7)<<stud.average<<"\n";
 
};
 
Students::iterator findElement(string toFind, Students::iterator begin, Students::iterator end)
{
    for(;begin!=end;++begin)
    {
        if ((*begin).fullname.find(toFind)!=(size_t)-1)
         return begin;
    }
    //если ничего не найдено - вернуть итератор указывающий за конец списка ( на NULL)
    return 0;
};
 
 
int main()
{
    SetConsoleCP(1251);
    SetConsoleOutputCP(1251);
 
 
    Students test("Иванов Иван",1992,100,"КС-09");
    test.addToTail("ЛЛЛ ФФ",1991,100,"КС-09");
    test.addToTail("БББ ВВ",1992,100,"КС-09");
    test.addToHead("Иванов Василий",1993,100,"КС-08");
    test.addToHead("Иванов Петр", 1990,100,"КС-07");
    test.addToHead("ЯЯЯ ММ",1993,100,"КС-08");
    test.addToTail("ЮЮЮ ЛЛЛ",1994,100,"КС-07");
    test.sort(cmp);
 
    cout<<"All list(after sort):\n";
    for_each(test.begin(),test.end(),print);
    test.deleteFromHead();
    test.deleteFromTail();
    cout<<endl<<"After delFromHead + delFromTail:\n";
    for_each(test.begin(),test.end(),print);
 
    cout<<endl<<"All Students with name \"Иванов\" (searching from begin to end of list):\n";
    Students::iterator it=test.begin();
    while(it=findElement("Иванов",it,test.end()))
     print(*it++);
 
    cout<<"\nAfter using (*it) and editing first element (fullname field) of list:\n";
    it=test.begin();
    (*it).fullname="ЛЛЛЛЛЛ";
    for_each(test.begin(),test.end(),print);
}
Добавлено через 1 минуту
CStudent сделана public так как иначе я не смогу её напечатать в main (он ругается что эта структура в классе Students private и мол потому нельзя чтобы функция print в main принимала её в виде параметра)...
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
03.10.2011, 00:50     Покритикуйте код #10
Цитата Сообщение от Gepar Посмотреть сообщение
CStudent сделана public так как иначе я не смогу её напечатать в main (он ругается что эта структура в классе Students мол private)...
Пока ListItem находится в private или protected, то есть не виден снаружи, ничего страшного. Хотя, может, эксперты скажут тут своё гав

Плохим тоном считается наличие конструктора у struct. Лучше бы их сделать class. Параметры конструктора ListItem

C++
1
2
3
4
5
6
        struct ListItem
        {
            CStudent student;
                ListItem *Next;
                ListItem(string ="",int =0,int =0,string ="",ListItem* =0);
        };
производят странное впечатление. Зачем они там?

У класса CStudent всё-таки советую сделать конструктор и

C++
1
friend std::ostream & operator<<( std::ostream &os, const CStudent &st );
Для вывода без print. Чтобы можно было

C++
1
2
3
4
CStudent pupkin( "V. Pupkin", "21TY", 2007, 4 );
//...
cout << "----------------------\n" << pupkin << '\n';
file_out << "----------------------\n" << pupkin << '\n';
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
03.10.2011, 00:57  [ТС]     Покритикуйте код #11
Цитата Сообщение от talis Посмотреть сообщение
производят странное впечатление. Зачем они там?
добавлять элементы удобнее, сразу половина проверок в конструкторе + можно ещё добавить проверки только здесь и больше нигде ,если препод придерётся (а он чувствую придерётся). Не будь там конструктора -при каждом добавлении через любой метод класса куда-либо в список элемента нужно все данные проверять, те проверок бы добавилось много.
Цитата Сообщение от talis Посмотреть сообщение
Пока ListItem находится в private или protected, то есть не виден снаружи, ничего страшного. Хотя, может, эксперты скажут тут своё гав
Но тогда не будет видно в print, хотя если я сейчас перегружу вывод в поток то print и не понадобится, но тем не менее то что не будет возможности вывести данные в main какраз и будет означать что в сл. лабораторных это мне помешает так как мне точно придётся данные через main выводить.
Цитата Сообщение от talis Посмотреть сообщение
Плохим тоном считается наличие конструктора у struct.
Ну я могу сделать его классом и отметить всё public, наличие конструктора у класса плохим тоном не считается ведь
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
03.10.2011, 01:00     Покритикуйте код #12
C++
1
2
3
4
5
Students::iterator& Students::iterator::operator++()
{
    current=current->Next;
     return *this;
}
а если я в конце, и current == 0? segfault?

C++
1
2
3
4
5
6
7
Students::iterator& Students::iterator::operator++()
{
     if( current ) 
         current = current->next;
 
     return *this;
}
Добавлено через 2 минуты
Цитата Сообщение от Gepar Посмотреть сообщение
добавлять элементы удобнее, сразу половина проверок в конструкторе + можно ещё добавить проверки только здесь и больше нигде ,если препод придерётся (а он чувствую придерётся). Не будь там конструктора -при каждом добавлении через любой метод класса куда-либо в список элемента нужно все данные проверять, те проверок бы добавилось много.
Параметры студента должны быть в конструкторе студента, а не в конструкторе элемента списка.

Цитата Сообщение от Gepar Посмотреть сообщение
Но тогда не будет видно в print, хотя если я сейчас перегружу вывод в поток то print и не понадобится, но тем не менее то что не будет возможности вывести данные в main какраз и будет означать что в сл. лабораторных это мне помешает так как мне точно придётся данные через main выводить.
Почему?

C++
1
2
3
for( lst::iterator it = lst.begin(); it != lst.end(); it++ )
   cout << (*it)  /* то есть CStudent &, то есть вызов 
   ostream & operator<<( ostream &os = cout, const CStudent &st = (*it) )*/ << '\n';
Цитата Сообщение от Gepar Посмотреть сообщение
Ну я могу сделать его классом и отметить всё public, наличие конструктора у класса плохим тоном не считается ведь
Ну вот.
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
03.10.2011, 01:06  [ТС]     Покритикуйте код #13
Цитата Сообщение от talis Посмотреть сообщение
а если я в конце, и current == 0? segfault?
Ну тоже вариант ) Добавил проверку что current до этого не был уже ==0.
В постфиксном тоже самое.

Цитата Сообщение от talis Посмотреть сообщение
Параметры студента должны быть в конструкторе студента, а не в конструкторе элемента списка.
Так это же при создании, а при добавлении (addToHead, addToTail).

Цитата Сообщение от talis Посмотреть сообщение
Почему?
Ну потому что всё отправляется в ostream поток таким образом, а прямого доступа к элементам из main не остаётся, а он вроде будет надо насколько я понял (я под win приложения с интерфейсом не писал так что не знаю как оно на самом деле) из слов преподавателя.
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
03.10.2011, 01:13     Покритикуйте код #14
Цитата Сообщение от Gepar Посмотреть сообщение
Ну потому что всё отправляется в ostream поток таким образом, а прямого доступа к элементам из main не остаётся
Gepar, вы немного не поняли. (*it) вернёт вам ссылку на CStudent, и по этой ссылке вы будете работать с объектом класса CStudent. Если вы в классе CStudent сделаете интерфейс доступа к элементам, то сможете работать через него.

Цитата Сообщение от Gepar Посмотреть сообщение
а он вроде будет надо насколько я понял (я под win приложения с интерфейсом не писал так что не знаю как оно на самом деле) из слов преподавателя.
Ахххахаа!! А я-то думаю, в чём дело Под "интерфейсом" препод имел ввиду не графический интерфейс пользователя, а программный интерфейс доступа к объекту. Функции доступа

Добавлено через 2 минуты
C++
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
class CStudent
{
private:
   string name;
   int year;
   //...
 
public:
   CStudent( string _name = "", int _year = 1812 );
   ~CStudent(); // вообще не нужен в данном частном случае, но для примера
 
   // ------- пошёл интерфейс --------
   string getName(){ return name; };
   int getYear(){ return year; };
 
   void setName( string _name ){ name = _name; };
   void setYear( int _year ){ name = _year; };
};
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
03.10.2011, 01:13  [ТС]     Покритикуйте код #15
talis, не-не, под интерфейсом там именно интерфейс, препод своих примеров понапихивал написанных так вот там есть у него лаба с "мордой", не знаю какая по счёту она будет.
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
03.10.2011, 01:17     Покритикуйте код #16
Ну и дальше обращение через интерфейс

C++
1
2
3
4
5
6
StudentList lst;
 
//...
 
for( StudentList::iterator it = lst.begin(); it != lst.end(); it++ )
   (*it).setName( "nameless" );
Кстати, итераторы тоже являются частью интерфейса. Они обеспечивают пользователю списка взаимодействие со студентами в этом списке, беря на себя все заботы по обходу списка, проверки на переход к нулевому элементу и так далее.


Цитата Сообщение от Gepar Посмотреть сообщение
talis, не-не, под интерфейсом там именно интерфейс, препод своих примеров понапихивал написанных так вот там есть у него лаба с "мордой", не знаю какая по счёту она будет.
Н-да?.. Ну это сути не меняет. Красивый графический интерфейс не отменяет хороший программный
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
03.10.2011, 01:23  [ТС]     Покритикуйте код #17
Странно, что-то не срабатывает моя перегрузка оператора...
итератор:
C++
1
 CStudent& operator*(){return current->student;}
в классе Students.h:
C++
1
friend std::ostream &operator<<(std::ostream &os, const CStudent &st);
в Students.cpp:
C++
1
2
3
4
5
6
7
8
std::ostream &operator<<(std::ostream &os, const CStudent &st)
{
    os<<setw(15)<<left<<st.fullname
     <<setw(7)<<st.group
     <<setw(5)<<st.year
     <<setw(7)<<st.average<<"\n";
 
}
в main:
C++
1
2
3
    cout<<"\nPrint list with ostream:\n";
    for(it=test.begin();it!=test.end();it++)
     cout<<(*it);
И не хочет, пишет что не нашло:
C++
1
C:\c++\Projects\StudentsProject\main.cpp|74|error: no match for 'operator<<' in 'std::cout << it.Students::iterator::operator*()'|
Странно.

Добавлено через 2 минуты
Хотя я вспомнил что перегрузка же должна быть отдельно от объявления, и всё же почему не подходит cpp файл, это разве не отдельно (из main компилируется если в main запихнуть код перегрузки) ?
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
03.10.2011, 01:25     Покритикуйте код #18
Ну пока бросилось в глаза, что std::ostream &operator<<(std::ostream &os, const CStudent &st) должна ещё вернуть os. Вы return забыли. А так - надо помедитировать... it какого типа?
Gepar
 Аватар для Gepar
1173 / 529 / 20
Регистрация: 01.07.2009
Сообщений: 3,512
03.10.2011, 01:33  [ТС]     Покритикуйте код #19
talis, об этом компилятор мне напомнил и это я добавил уже когда смог скомпилировать проэкт.
it - Students::iterator конечно, я сейчас попробую ещё в vs скомпилировать когда перегрузка находится в cpp файле, а не в main.

Добавлено через 2 минуты
А VS компилирует нормально, это только minigw чего-то обижается ... ну да ладно, так как уже поздно то я буду спать, думаю класс уже не столь страшен после Вашей критики так что можно и на глаза преподавателю его показать ) Спасибо за помощь в поиске bug'ов и недочётов класса
MoreAnswers
Эксперт
37091 / 29110 / 5898
Регистрация: 17.06.2006
Сообщений: 43,301
03.10.2011, 01:33     Покритикуйте код
Еще ссылки по теме:

C++ Покритикуйте код
Покритикуйте мою игру C++
C++ Графы. Покритикуйте код

Искать еще темы с ответами

Или воспользуйтесь поиском по форуму:
talis
 Аватар для talis
789 / 541 / 37
Регистрация: 11.05.2010
Сообщений: 1,298
Записей в блоге: 1
03.10.2011, 01:33     Покритикуйте код #20
Цитата Сообщение от Gepar Посмотреть сообщение
CStudent& operator*(){return current->student;}
а должно быть

C++
1
CStudent & Students::iterator::operator*(){ return current->student; }
Если, конечно, это не в объявлении класса итератора было.
Yandex
Объявления
03.10.2011, 01:33     Покритикуйте код
Ответ Создать тему
Опции темы

Текущее время: 14:12. Часовой пояс GMT +3.
КиберФорум - форум программистов, компьютерный форум, программирование
Powered by vBulletin® Version 3.8.9
Copyright ©2000 - 2016, vBulletin Solutions, Inc.
Рейтинг@Mail.ru