C++ Program do oceny

Question

Chciałbym prosić o ocenę programu, głównie pod kątem czystego kodu. 

EDIT: Program ma za zadanie skrócenie napisu podanego przez użytkownika. Przykład AAABBCCCC zamienia na A3BBC4.

#include <iostream>

using namespace std;

void getSubtitles(char against[][201], int dimension);

void reduceSubtitles(char toChange[][201], int sizeOfSubtitles);

void makeSmaller(char toChange[]);

void cutRepeatsInSubtitle(char toChange[], char afterChange[], int i, int &j, int &sameLetters);

void moreThenThreeLettersInRow(char subtitle[], int &actualLetter, const int amountOfSameLettersInRow);

int changeDigitToAscii(int DigitToSwap);

void showSubtitles(char caption[][201], int amount);

int main()
{
    int amount;
    cin>>amount;

    char inscription[amount][201];

    getSubtitles(inscription,amount);

    reduceSubtitles(inscription,amount);

    showSubtitles(inscription,amount);

    return 0;
}

void getSubtitles(char caption[][201], int amount)
{
    for(int i=0;i<amount;i++)
        cin>>caption[i];
}

void reduceSubtitles(char toChange[][201], int amount)
{
    for(int i=0;i<amount;i++)
        makeSmaller(toChange[i]);
}

void makeSmaller(char toChange[])
{    
    char afterChange[201];
    int sameLetters=1, actualLetterAfterChange=0;
    
    for(int i=0;toChange[i];i++)
        cutRepeatsInSubtitle(toChange, afterChange, i, actualLetterAfterChange, sameLetters);
    
    afterChange[actualLetterAfterChange]='\0';

    int j;
    for(j=0;afterChange[j];j++)
        toChange[j]=afterChange[j];
    while(j<201)
        toChange[j++]='\0';
}

void cutRepeatsInSubtitle(char toChange[], char afterChange[], int i, int &j, int &sameLetters)
{
    if(toChange[i]==toChange[i+1])
        sameLetters++;
    else if(sameLetters==1)
    {
        afterChange[j++]=toChange[i];
        sameLetters=1;
    }
    else if (sameLetters==2)
    {
        afterChange[j++]=toChange[i-1];
        afterChange[j++]=toChange[i];
        sameLetters=1;
    }
    else
    {
        afterChange[j++]=toChange[i];
        moreThenThreeLettersInRow(afterChange,sameLetters,j);
        sameLetters=1;
    }
}

void moreThenThreeLettersInRow(char subtitle[], int &actualLetter, const int amountOfSameLettersInRow)
{
    if(amountOfSameLettersInRow>99)
        subtitle[actualLetter++]=(char)changeDigitToAscii(amountOfSameLettersInRow/10/10);

    if(amountOfSameLettersInRow>9)
        subtitle[actualLetter++]=(char)changeDigitToAscii(amountOfSameLettersInRow/10);

    subtitle[actualLetter++]=(char)changeDigitToAscii(amountOfSameLettersInRow%10);
}

int changeDigitToAscii(int DigitToSwap)
{
    switch(DigitToSwap)
    {
        case 0: return 48; break;
        case 1: return 49; break;
        case 2: return 50; break;
        case 3: return 51; break;
        case 4: return 52; break;
        case 5: return 53; break;
        case 6: return 54; break;
        case 7: return 55; break;
        case 8: return 56; break;
        case 9: return 57; break;
    }
}

void showSubtitles(char caption[][201], int amount)
{
    for(int i=0;i<amount;i++)
        cout<<caption[i]<<endl;
}

 

Answer 1

Napisałbyś chociaż, czego dotyczy ten kod, co on w ogóle robi. Tak każdy oceniający musi niepotrzebnie tracić czas na sprawdzanie tego.

Nie będę wnikał bardzo w treść kodu, ale widzę parę błędów:

• Używanie using namespace std nie jest dobrym pomysłem. Główny problem, jaki to generuje, to brak podkreślenia, które elementy należą do biblioteki standardowej. Kolizja oznaczeń jest pewnie mało prawdopodobna, no ale może się też zdarzyć.
• Upchanie definicji funkcji do jednego kodu z mainem. Czytelność by się znacznie zwiększyła, gdyby funkcja główna była jakoś wyszczególniona.
• Ten kod jest małym nieporozumieniem.

  switch(DigitToSwap)
  {
      case 0: return 48; break;
      case 1: return 49; break;
      case 2: return 50; break;
      case 3: return 51; break;
      case 4: return 52; break;
      case 5: return 53; break;
      case 6: return 54; break;
      case 7: return 55; break;
      case 8: return 56; break;
      case 9: return 57; break;
  }

  Po pierwsze, po co wywoływać instrukcję break po każdym casie, jeśli return i tak kończy wykonywanie funkcji. Po drugie, wystarczyłoby napisać coś takiego:

  return DigitToSwap + 48;

  Po trzecie, funkcja nie zwraca wartości przy wartościach innych niż wyróżnionych w switchu. Nawet nie wiem, jak ci to kompilator przepuścił, no ale cóż.

Comments

Na prawdę w podstawowym toolkicie nie ma narzędzia do konwersji kodowania tylko trzeba ręcznie klepać? : P

---

Ale niby dlaczego miałoby być potrzebne, jeśli algorytm do napisania tego jest trywialny?

Podobnie sprawa wygląda z funkcją std::rand z biblioteki cstdlib. Ona zwraca jakieś ogromne pseudolosowe wartości, a żeby dostać liczbę z danego przedziału, to wystarczy wykorzystać prosty trick z operacją modulo i dodawaniem.

Wniosek jest jeden. Nie ma potrzeby robić narzędzi do wszystkiego i przez to komplikować samą dokumentację.

---

I masz więcej swojego kodu, więcej do testowania, mniej klarowny kod.
Nie ma co wynajdować koła na nowo i lepiej mieć do tego gotowe, otestowane narzędzie. Nawet przy takich pierdołach. Tak moim zdaniem.

I za każdym razem piszesz te swoje trywialne rozwiązania w każdym projekcie? Niezła zaradność xD

---

Zależy od założenia. Tak w ogóle można pisać '0' zamiast 0 i nagle problem znika.

---

Gotowe narzędzie wyraża więcej niż jakieś krzaki w kodzie - tak czy siak trzeba to wydelegować do funkcji.

---

I za każdym razem piszesz te swoje trywialne rozwiązania w każdym projekcie? Niezła zaradność xD

A kto ci zabrania stworzyć oddzielne pliki źródłowe do tego? Pliki obiektowe ze skompilowanymi funkcjami lub nawet bibliotekę linkowaną statycznie lub dynamicznie? Najwyraźniej masz małe wyobrażenie, jak się tworzy uniwersalny kod  

---

Ale niby dlaczego miałoby być potrzebne, jeśli algorytm do napisania tego jest trywialny?

 

---

To jest oczywiście moje zdanie i nie każę ci tak samo uważać. A mój ostatni komentarz jedynie odnosił się do tego fragmentu, abstrahując od moich przekonań.