Ocena kodu/algorytmu (uwaga, nawiązanie do spoja)

pytanie zadane 15 marca 2018 w C i C++ przez Jakub 0 Pasjonat (23,120 p.)
zamknięte 17 marca 2018 przez Jakub 0

Witam, nie nadałem temu pytaniu kategorii "SPOJ" bo bardziej chodzi tu o ogólnie ocenę kodu, jego jakości a nie że mam jakieś problemy z sędzią i kompilacją... Ale uprzedziłem w temacie ;)

Raz na jakiś czas daje jakiś kod do oceny (chodź by tak prosty jak ten), bo zazwyczaj też wtedy dowiaduje się o ciekawych algorytmach, składni itd... Tym razem jest to zadanie z serwisu spoj. Polega ono że na wejściu mamy kod html'owy i litery po między '<' a '>' musimy zamienić na duże (czyli zawartość tagów chyba to się nazywa).

Mam taki kod w c++:

#include <iostream>
#include <string>
#include <vector>
#include <cctype>

///----------------------------------------------------------------------------------------

using vecStr = std::vector<std::string>;
vecStr read();
void change(vecStr&);
void show(const vecStr&);

///--------------------------------------------------------------------------------------

int main(){

    auto lines = read(); //wczytanie linii kodu 
    change(lines); //zmieniamy wygląd tagów (litery na duże) 
    show(lines); //pokazujemy zmieniony kod 

    return 0;
}

///-----------------------------------------------------------------------------------------

vecStr read(){ //wczytanie danych 
    vecStr vec;
    std::string temp;
    while(std::getline(std::cin,temp)){
        vec.push_back(temp);
    }

    return vec;
}

///------------------------------------------------------------------------------------------------

void change(vecStr& lines){ //algorytm do zamieniania wielkości liter pomiędzy <>
    for(auto& it : lines){ //pętla dla pojedynczych linii tekstu
        bool d{false}; //czy zmieniać wielkość liter
        for(auto& ch : it){ //pętla dla pojedynczych znaków  w linii 
            if(ch=='<')
               d = true;
            else if(ch=='>')
                d = false;
            else if(d){
                ch = toupper(ch); //zamiana litery na dużą 
            }
        }
    }
}

///------------------------------------------------------------------------------------------------

void show(const vecStr& lines){ //prezentacja zmienionych danych 
    for(const auto& it : lines){
        std::cout<<it<<std::endl;
    }
}

///------------------------------------------------------------------------------------------

Plik tekstowy z kodem:

<html>
<head>
<TITLE>To jest tytul</Title>
</head>
<body>
<b>Cos tam</b>
</body>
</html>

Oraz plik wsadowy dokonujący przekierowania strumienia wejściowego z pliku do programu:

@echo off
"Tagi HTML.exe" < "HTMLfile.txt"
pause > nul

Może kod programu nie jest specjalnie długi, ale może jest jakaś drobnostka do poprawienia? Zastanawiam się nad funkcją:

toupper(char);

z biblioteki cctype. Czy można ją zastąpić czymś nowszym?

Dziękuje za rady i serdecznie pozdrawiam :)

komentarz zamknięcia: Temat wyczerpany

3 odpowiedzi

odpowiedź 15 marca 2018 przez mokrowski Mędrzec (155,460 p.)
wybrane 17 marca 2018 przez Jakub 0

Najlepsza

Jeśli rozwiązanie mieści się w kryteriach sędziego ze SPOJ'a, to jest ok. Jakość kodu także dobra. Stąd też ew. moje uwagi, to już pedanteria granicząca z czepialstwem.

Co do kodu który pokazałeś to linia 21, zbędne return 0. C++ sam to robi w funkcji main(). Samo toupper() z <cctype> także jest ok. Wprawdzie jest także toupper() z <locale>, ale tu raczej wyzwań lokalizacyjnych nie masz.

Co do samego algorytmu, jak mówiłem wcześniej... sędzia akceptuje więc ok. Znając jednak specyfikę SPOJ'a, myślę że możesz wykonać tę pracę bez tworzenia kontenera z danymi wyłącznie na podstawie transformacji wczytywanych literek i wyprowadzaniu ich na strumień wyjścia. Moja podpowiedź: std::transform z <algorithm>, std::istream_iterator i std::ostream_iterator. Nawiązując do Twojego kodu, tak to by wyglądało:

#include <iostream>
#include <algorithm>
#include <iterator>
#include <iomanip>
#include <cctype>
 
int main(){
    using iIt = std::istream_iterator<char>;
    using oIt = std::ostream_iterator<char>;
    iIt iBeg(std::cin), iEnd;
    oIt iOut(std::cout);
    bool upperFlag{false};
    std::cin >> std::noskipws;
    std::transform(iBeg, iEnd, iOut, 
            [&](char c) {
                if(c == '<') {
                    upperFlag = true;
                } else if(c == '>') {
                    upperFlag = false;
                } else if(upperFlag) {
                    c = toupper(c);
                }
                return c;
    });
}

komentarz 15 marca 2018 przez Jakub 0 Pasjonat (23,120 p.)

Dziękuje za ciekawy sposób realizacji programu. Jest tylko taka sprawa, nie wiele z tego kodu rozumiem :/

Rozumiem oczywiście proces zwiększania liter. Za to:

std::transform...

Nie rozumiem, funkcja w funkcji? Czy to jest lambda? Jeśli tak to o nich jeszcze w książce z której się uczę nie miałem. Ale postaram się na ten temat poczytać wcześniej w internecie.

Był bym bardzo wdzięczny za opis plus minus co wszystko oznacza, bo jestem początkujący i o takich zapisach jeszcze nie słyszałem ( upperFlag -> to oczywiście rozumiem ) :

using iIt = std::istream_iterator<char>;
    using oIt = std::ostream_iterator<char>;
    iIt iBeg(std::cin), iEnd;
    oIt iOut(std::cout);
    bool upperFlag{false};
    std::cin >> std::noskipws;

Tak jak mówiłem:

Raz na jakiś czas daje jakiś kod do oceny (chodź by tak prosty jak ten), bo zazwyczaj też wtedy dowiaduje się o ciekawych algorytmach, składni itd...

;)

komentarz 15 marca 2018 przez mokrowski Mędrzec (155,460 p.)

using iIt = std::istream_iterator<char>;
using oIt = std::ostream_iterator<char>;

Definiuję sobie typy iteratorów aby później przy wywołaniu algorytmu nie pisać dużej ilości literek definiujących typ. iIt to Input Iterator a oIt to Output Iterator.

iIt iBeg(std::cin), iEnd;
oIt iOut(std::cout);

Tworzę obiekty iteratorów z których iBeg będzie wprowadzał dane ze strumienia std::cin a iEnd jest postym iteratorem potrzebnym do informacji że dane się już skończyły. Podobnie iOut. To iterator wyjściowy na strumieniu std::cout.

std::cin >> std::noskipws;

Domyślnym separatorem danych na strumieniach jest '\n' lub ' ' (znak nowej linii lub spacja). Ja chcę aby strumień std::cin czytał każdy znak. Stąd wyłączam ignorowanie znaków separatorów.

Algorytm std::transform potrzebuje iteratora początkowego i końcowego danych które pobiera oraz iteratora początkowego danych które zwraca. Jako ostatni argument wpisuje się funkcję która wykona operacje na danych i zwróci wynik. Dane do funkcji trafiają kolejno z danych wejściowych a to co funkcja zwróci, wpisywane jest do danych wyjściowych. Wprost transformuje dane. Np. aby dane w std::vector podnieść do kwadratu, wystarczy (tu napiszę program bez lambdy:

#include <iostream>
#include <vector>
#include <algorithm>

int twoPower(int a) {
    return a * a;
}

int main() {
    std::vector<int> values = { 1, 3, 6, 8, 4, 9 };
    std::vector<int> results(values.size());
    std::transform(values.cbegin(), values.cend(), results.begin(), twoPower);

    for(const auto& val: results) {
        std::cout << val << ' ';
    }
    std::cout << '\n';
}

odpowiedź 15 marca 2018 przez Bondrusiek Maniak (61,370 p.)

Witam,

według mnie powinieneś poprawić nazwy funkcji. Twoje nie mówią za dużo. Powinieneś użyć takiego stylu do pisania funkcji

czasownik + rzeczownik

Więc

auto lines = read(); // na auto lines = readLine();
change(lines); //na changeLine(lines);
show(lines);// na showLine(lines);

Dodatkowo staraj się unikać oczywistych komentarzy. Nic nie wnoszą a zaciemniają kod. Na podstawie kodu powinno się max informacji. Np

auto lines = read(); //wczytanie linii kodu 
change(lines); //zmieniamy wygląd tagów (litery na duże) 
show(lines); //pokazujemy zmieniony kod 

// na np

auto lines = readLine();
changeLine(lines);
showLine(lines);

//Powyższe definicje już dużo mówią wiadowo że readLine() wczytuje linie
// a showLine() ją pokazuje. Komentarze tylko zaciemniają kod

Jeszcze staraj się unikać takich 'magicznych' nazw.

bool d{false};
//powyzej nic nie mówi
//mozna zamienić na 
bool changeLetterSize{false};

komentarz 15 marca 2018 przez Jakub 0 Pasjonat (23,120 p.)

Dziękuje, rzeczywiście bardzo słuszna uwaga. Co do komentarzy to dopisałem je kiedy umieszczałem kod w serwisie (orginalna wersja ich nie zawiera). Ale rzeczywiście... raczej nie są potrzebne bo rolę funkcji są jasne.

odpowiedź 15 marca 2018 przez Beginer Pasjonat (22,110 p.)

Ja oceniam "wrażenie artystyczne" kodu, jest b.wysokie. Można by pokazywać w podręcznikach programowania (a i tam kody nie wyglądają tak ładnie, przejrzyście). Przyjemnie, łatwo się to czyta i analizuje.

Nie widziałem jeszcze tak schludnie, porządnie napisanego kodu. Choć sam staram się to robić dobrze i zwracam na to uwagę.

Mógłbyś jeszcze wyrównać komentarze (od tej samej kolumny). Nie zawsze jest to możliwe i wskazane, ale tutaj akurat tak. Znalazłem też pewien drobiazg w strumieniu cout - operatory lepiej wydzielać spacjami.

std::cout<<it<<std::endl;

Chyba jesteś plastykiem, nie koderem!