Pierwszy program w portfolio

Odpowiedz Nowy wątek
2019-01-17 08:15
0

Witajcie
Po wielkich trudach, stworzyłem swój pierwszy poważniejszy program i dlatego tym chętniej wysłucham waszych opinii.
Pozdrawiam


Nieoszlifowany diament
edytowany 1x, ostatnio: Adept123, 2019-01-17 08:21

Pozostało 580 znaków

2019-01-17 09:08
4

Powiem szczerze - po przeczytaniu drugiej linii mi się odechciało.

uses Crt, Dos; { doĄczenie moduu Crt oraz Dos }

LITOŚCI...
Nie chcę dawać teraz wykładu dot. sensu oraz sposobu wstawiania komentarzy, ale powiem tylko tyle, że to jest jakaś totalna parodia. Nie mam pojęcia, czemu on ma tutaj służyć, bo każdy kto ma minimalne pojęcie o programowaniu doskonale wie, co ten uses robi, a jak ktoś się nie zna, to i komentarza nie zrozumie.


That game of life is hard to play
I'm gonna lose it anyway
The losing card I'll someday lay
So this is all I have to say
edytowany 1x, ostatnio: cerrato, 2019-01-17 09:09

Pozostało 580 znaków

2019-01-17 09:50

Dobra, po chwili stwierdziłem, że jednak napiszę coś więcej. Rozumiem, że to Twoje pierwsze "większe" osiągnięcie, więc nie chcę, żebyś się zraził przez jakiegoś buraka z forum ;)

Odnośnie komentarzy - podtrzymuję to, co pisałem przed chwilą. Do tego widzę, że ta moja uwaga dotyczy praktycznie wszystkich - są wstawiane w miejscach, które raczej nie wymagają komentowania, a nawet jeśli, to nie w takiej postaci.

Widzę, że w wielu miejscach w deklaracjach typów ograniczasz długość stringow - nie ma w tym niczego złego (aczkolwiek w trochę bardziej "nowoczesnych" dialektach stringi mają dynamiczną długość, w związku z czym nie ma ryzyka marnowania miejsca w pamięci, więc nie trzeba ich na siłę skracać), ale musisz się zastanowić, czy podane długości są odpowiednie. Swoją drogą - czy nie lepiej, żebyś (skoro piszesz w Pascalu - wbrew pozorom to bardzo fajny i wcale nieprzestarzały język) zainteresował się jego jakąś nową odmianą? Można pobrać darmową wersję Delphi, można także (polecam) skorzystać z Lazarusa.

poziomprac ma pole wykształcenie, które jest typu string - ja bym dał raczej jakiś typ wyliczeniowy - https://pl.wikibooks.org/wiki[...]l/Typy_zmiennych#Wyliczeniowy.

readln(liczba); - zmienna liczba jest typu integer. Czy przewidziałeś sytuację, w której ktoś zamiast liczny wpisze "wew42fsd"?

Zawsze staraj się unikać długich funkcji i procedur, staraj się je upraszczać aż do granic przyzwoitości, a jeśli gdzieś widzisz fragment, który się powtarza - staraj się dać go do osobnej procedury. Przykładowo:

writeln('Podaj pierwsze imi©:');
readln(imiepier);
imiepier[1] := UpCase(imiepier[1]);

jest wykonywane kilka razy pod rząd, dla poszczególnych danych dot. pracownika. Aż prosi się, żeby fragment wczytujący wydzielić do osobnej procedury, a potem ją wywoływać. Coś w stylu ImiePier := WczytajOdUsera ('Podaj pierwsze imie');.

Odnośnie pytania o języki obce - bez sensu jest pytanie o wszystkie 4. Jeśli jako pierwszy podam English, a drugi zostawię pusty, to oznacza, iż więcej nie znam. Po co więc pytać mnie o 3 i 4?

Procedura przewijanie oraz inne miejsca, które zajmują się dostępem do danych zawartych w rekordach - aż prosi się, żeby zrobić z tego obiekty. Zapis w postaci (imiedrug:16-(length(imiepier)+1)+length(imiedrug)) jest średnio czytelny. A jak masz 5 takich linii jedna pod drugą, to moja jedyna myśl, kiedy na to patrzę - mam nadzieję, że to działa i nie trzeba będzie niczego zmieniać, bo nie chcę tego nawet dotykać ;)

write(s:10+abs(length('Nazwisko')-length(nazwisko))+length(s)) - ponownie, aż prosi się, żeby zamiast zamieszczać takie paskudztwa w kodzie, zrobić z tego osobną procedurę, która przyjmie jako parametr to, co ma wyświetlić, a całe formatowanie będzie ukryte. Bo w tej chwili masz pomieszaną logikę (czyli decyzję CO trzeba wyświetlić) z jej implementacją - czyli sposobem, JAK to ma się dziać. Po pierwsze - takie podejście utrudnia dalszą rozbudowę czy modernizację programu. Po drugie - jak ktoś analizuje LOGIKĘ, czyli mechanizm działania, to ma głęboko gdzieś, w jaki sposób coś zostanie wyświetlone. Dla niego istotne jest to, ze COŚ ma być wyświetlone. Analogicznie - zmieniając sposób wyświetlania, nie interesuje Cię kiedy dane informacja ma się pojawić, skupiasz się jedynie na tym, żeby odpowiednio ją sformatować. Po trzecie (to takie trochę rozszerzenie poprzedniej uwagi) - zastanów się, co w sytuacji, jakbyś zamiast wystawiać te dane na monitor, chciałbyś je wystawić na drukarkę. Obecnie musisz kombinować, a mając to w osobnej funkcji, sposób wyprowadzania danych zmieniasz tylko w niej, a dla reszty programu nic się nie zmienia, w sytuacji, w której mają coś "wypluć" korzystają z tej samej funkcji, niezależnie od tego, gdzie i jak ma ona te dane wypisać. I ostatnia sprawa na rzecz wydzielania takich rzeczy do osobnych procedur - w sytuacji, jeśli będziesz chciał coś zmienić (np. kolor napisów, albo szerokość wydruku) to mając to wpisane na sztywno do kodu przy każdym writeln();, musisz poprawić w każdym miejscu. Mając to w osobnej funkcji - poprawiasz tylko raz. Łatwiej i mniejsze ryzyko popełnienia błędu.

Ta sama uwaga (dot. wydzielania powtarzalnych operacji do osobnej funkcji) dotyczy procedury Napisy_Do_Jezykow. Masz tam 4 razy powtórzone praktycznie to samo, takie kopiuj-wklej z drobnymi zmianami.

Ogólnie nazwy funkcji i zmiennych są OK (można się doczepić do tego, że bardzie profesjonalnie jednak by było po angielski). Ale czasami o tym zapominasz i dajesz a,j,l,lp,numer,c: integer;. Doprowadza to do zapisów w stylu j := c + 1; c := c + (lp mod c) albo if pi_pl then if 1 < j then znaleziono := WB(t,1,j-1,nazw_pracownika,pi_pl,dr_pl) - OSCAR dla osoby, która bez analizy reszty programu będzie mogła od razu ustalić, co ten fragment robi, za co odpowiada i czemu jest akurat w tym miejscu wstawiony :P Swoją drogą - jak pisałem na początku, dajesz komentarze oczywiste w treści i w miejscach, które tego nie wymagają, a tutaj mamy jakąś zagadkę, przy której parę słów wyjaśnienia by się przydało, ale nikt nic nie napisał. Aczkolwiek i tak lepszą opcją jest nie dawać komentarzy, ale za to pisać w taki sposób, żeby kod się sam dokumentował.

if dane[j]^.poziom.obce[1] <> '' then - i tak 4 razy. Czy nie lepiej, zamiast wpisywać [1], następnie [2] itp. zrobić to w pętli for i zapisać w postaci poziom.obce[zmienna_z_for]?

if obecna_liczba >= 1 then
       while ((obecna_liczba>=1) and
         ((dane[obecna_liczba]^.nazw.nazwisko > nowyprac^.nazw.nazwisko) or
         ((dane[obecna_liczba]^.nazw.imiepier > nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)) or
         ((dane[obecna_liczba]^.urodz.rok > nowyprac^.urodz.rok) and
         (dane[obecna_liczba]^.nazw.imiepier = nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)) or
         ((dane[obecna_liczba]^.urodz.miesiac > nowyprac^.urodz.miesiac) and
         (dane[obecna_liczba]^.urodz.rok = nowyprac^.urodz.rok) and
         (dane[obecna_liczba]^.nazw.imiepier = nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)) or
         ((dane[obecna_liczba]^.urodz.dzien > nowyprac^.urodz.dzien)
         and (dane[obecna_liczba]^.urodz.miesiac = nowyprac^.urodz.miesiac) and
         (dane[obecna_liczba]^.urodz.rok = nowyprac^.urodz.rok) and
         (dane[obecna_liczba]^.nazw.imiepier = nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)) or
         ((dane[obecna_liczba]^.urodz.miejsce > nowyprac^.urodz.miejsce)
         and (dane[obecna_liczba]^.urodz.dzien = nowyprac^.urodz.dzien)
         and (dane[obecna_liczba]^.urodz.miesiac = nowyprac^.urodz.miesiac) and
         (dane[obecna_liczba]^.urodz.rok = nowyprac^.urodz.rok) and
         (dane[obecna_liczba]^.nazw.imiepier = nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)))) do
            obecna_liczba := obecna_liczba - 1;

Po przeczytaniu tego zacząłem bardzo mocno rozważać samobójstwo jako najlepsze rozwiązanie ;) Może teraz Ty, jako twórca, wiesz o co chodzi w tym fragmencie, ale podejrzewam, że za miesiąc będziesz miał taki sam odruch wymiotny co ja, kiedy na to spojrzysz...

To wszystko co napisałem dotyczy jedynie sposobu napisania kodu, nie mam ani czasu, ani sił, żeby sprawdzać, czy wszystkie operacje działające na danych są poprawne, czy dobrze operujesz wskaźnikami, czy nie ma wycieków pamięci itp. Może ktoś inny będzie miał na to ochotę ;)

W każdym razie - jak poprawisz ten kod zgodnie z tym, co napisałem - śmiało wrzucaj, ocenimy postęp.


That game of life is hard to play
I'm gonna lose it anyway
The losing card I'll someday lay
So this is all I have to say
edytowany 3x, ostatnio: cerrato, 2019-01-17 09:54

Pozostało 580 znaków

2019-01-17 10:12
3
  1. uses Crt, Dos; { dołączenie modułu Crt oraz Dos } - komentarze powinny wnosić rzeczy, których nie da się domyślić w ciągu kilkuset milisekund od patrzenia na kod (np. jeśli wykorzystujesz jakiś skomplikowany algorytm / sztuczkę itd).

  2. ident, imiepier, imiedrug - w erze terabajtowych dysków nie warto oszczędzać kilka bajtów na identyfikatorze. ImiePierwsze, ImieDrugie, JezykiObce [...] są znacznie czytelniejsze, a nie tracisz przy tym praktycznie nic.

  3. data = record - pascalową konwencją byłoby TData, a najlepiej po angielsku: TDate.

  4. Dlaczego rekord o nazwie data zawiera wewnątrz informację o miejscu? Co ma jedno do drugiego?

  5. telefon: string[11] - dlaczego nie po prostu telefon: string? Podobnie jak w przypadku wszystkich innych string[cośtam] - dlaczego nie po prostu string?

  6. Wynik_Fun - nazwa tej funkcji nie mówi absolutnie nic o tym, co ta funkcja robi. Czym ona się tak właściwie zajmuje?

  7. Procedura Czytaj_liczbe zwraca tylko jedną wartość, zatem powinna być funkcją.

  8. Gdybyś zmienił nazwę Czytaj na WczytajPracownika (i analogicznie z resztą) nie musiałbyś wcale dorzucać do tej procedury komentarza.

  9. Nie mam pojęcia co robi procedura Przewijanie.

  10. Zamiast zakładać na sztywno, że można wprowadzić tylko cztery języki obce, zrób tak, aby dało się ich wpisać nieskończenie wiele. Zwróć uwagę jak wpłynie to na projekt Twojej aplikacji (np. na procedurę Napisy_Do_Jezykow).

  11. Procedura Znajomosc_Jezykow to czysta magia - nie mam najmniejszego pojęcia czemu służy przykładowo j := c + 1; c := c + (lp mod c), krótkie nazwy zmiennych j i c tylko utrudniają rozumienie całego kodu.

  12. Nie mam pojęcia co robi funkcja WB i dlaczego akurat tak zdecydowałeś się ją nazwać.

  13. W jakim celu utworzyłeś zmienną zgodnosc w funkcji Spraw_daty_urodzenia?

  14. Dlaczego funkcja Spraw_daty_urodzenia przyjmuje tak dużo argumentów? Spodziewałbym się, że będzie ona przyjmować tylko rok, miesiąc i dzień.

  15. Ten cały ogromny if w procedurze Wstaw jest... ogromny.

  16. Procedura Wyszukaj_Po_Nazwisku robi za dużo / jest za długa.

  17. W procedurze Odczytaj brakuje zabezpieczenia przed wyjściem poza rozmiar tablicy dane.

  18. Tablica dane powinna się nazywać np. ludzie (konkretnie zamiast ogólnie).


Reasumując: jest całkiem nieźle jak na początkującego :-)
IMO powinieneś popracować nad nazewnictwem (aby było bardziej opisowe) oraz komentarzami (aby nie opisywały rzeczy oczywistych).


edytowany 3x, ostatnio: Patryk27, 2019-01-17 10:14
Wynik_Fun - nazwa tej funkcji nie mówi absolutnie nic - jak to nic? Przecież to jest wynik funkcji :P - cerrato 2019-01-17 10:15
hmm ciekawe czy by przeszło jak bym nazwał funkcję result - babubabu 2019-01-17 19:25

Pozostało 580 znaków

2019-01-19 19:25
0

Myślę, że jak na pierwszy program to jest dobrze, ale w tym miejscu trochę nie wiadomo o co chodzi.

       while ((obecna_liczba>=1) and
         ((dane[obecna_liczba]^.nazw.nazwisko > nowyprac^.nazw.nazwisko) or
         ((dane[obecna_liczba]^.nazw.imiepier > nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)) or
         ((dane[obecna_liczba]^.urodz.rok > nowyprac^.urodz.rok) and
         (dane[obecna_liczba]^.nazw.imiepier = nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)) or
         ((dane[obecna_liczba]^.urodz.miesiac > nowyprac^.urodz.miesiac) and
         (dane[obecna_liczba]^.urodz.rok = nowyprac^.urodz.rok) and
         (dane[obecna_liczba]^.nazw.imiepier = nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)) or
         ((dane[obecna_liczba]^.urodz.dzien > nowyprac^.urodz.dzien)
         and (dane[obecna_liczba]^.urodz.miesiac = nowyprac^.urodz.miesiac) and
         (dane[obecna_liczba]^.urodz.rok = nowyprac^.urodz.rok) and
         (dane[obecna_liczba]^.nazw.imiepier = nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)) or
         ((dane[obecna_liczba]^.urodz.miejsce > nowyprac^.urodz.miejsce)
         and (dane[obecna_liczba]^.urodz.dzien = nowyprac^.urodz.dzien)
         and (dane[obecna_liczba]^.urodz.miesiac = nowyprac^.urodz.miesiac) and
         (dane[obecna_liczba]^.urodz.rok = nowyprac^.urodz.rok) and
         (dane[obecna_liczba]^.nazw.imiepier = nowyprac^.nazw.imiepier)
         and (dane[obecna_liczba]^.nazw.nazwisko = nowyprac^.nazw.nazwisko)))) do

Pozostało 580 znaków

2019-01-19 23:01
4

Co to za emotka ^. i czemu jej tak często używacie?


"HUMAN BEINGS MAKE LIFE SO INTERESTING. DO YOU KNOW, THAT IN A UNIVERSE SO FULL OF WONDERS, THEY HAVE MANAGED TO INVENT BOREDOM."

Pozostało 580 znaków

2019-01-19 23:04
4

Co to za emotka ^. i czemu jej tak często używacie?

To emotka jednookiego pirata. Powinna być wstawiana w miejscach, gdzie kod nie jest Twój, tylko został pozyskany z netu i umieszczony w projekcie metodą kopiuj-wklej .


That game of life is hard to play
I'm gonna lose it anyway
The losing card I'll someday lay
So this is all I have to say

Pozostało 580 znaków

2019-01-20 12:07
0

Witajcie
Pol90 i dla wszystkich innych zainteresowanych wyjaśniam :).
Ta warunek while wyznacza miejsce wstawienia nowego pracownika.
Np. jeśli wstawię Jan Kowalski ur. 01 01 1980 miejsce urodzenia: Warszawa i Jan Kowalski ur. 31 12 1979 miejsce urodzenia: Warszawa, to wpis drugi będzie pierwszy i odwrotnie. Jeśli na końcu dodam Jan Kowalski ur 01 01 1980 miejsce urodzenia: Białystok to ten będzie po środku :).
Pozdrawiam


Nieoszlifowany diament
edytowany 1x, ostatnio: Adept123, 2019-01-20 12:12
"Ta warunek while wyznacza miejsce wstawienia nowego pracownika.", ale w "prawdziwym" programowania zwykle takie sprawy załatwia się w bazie danych - cw 2019-01-20 12:53
To nie chodzi o to, zebyś nam wyjaśnił co ten fragment robi - podejrzewam, że każdy zainteresowany, jakby przysiadł na chwilkę i to rozebrał na czynniki pierwsze to by doszedł do sensu tego rozwiązania. Chodziło nam o to, że jest to napisane strasznie. Rozumiem, że się uczysz i nie można od Ciebie oczekiwać jakichś zaawansowanych technik, ale musisz przyznać, że wygląda to w sposób odstraszający. Zastanów się, czy masz pomysł, jak można by było to uprościć. - cerrato 2019-01-20 17:38

Pozostało 580 znaków

2019-01-20 12:44
cw
0

Też nie chcę Ciebie zniechęcać bo od czegoś trzeba zacząć, ale:

  • trudno ten program nazwać dużym lub większym. Ja osobiście określiłbym to jako "wprawkę"
  • nie wiem czy najlepszym pomysłem jest pisanie w pascalu. Radziałbym przejść np. na Pythona
  • to już nie te czasy, aby dane pobierać od użytkownika z linii komend
edytowany 1x, ostatnio: cw, 2019-01-20 12:46
Sam "goły" Pascal to rzeczywiście lekki zabytek, ale bazujace na nim Delphi czy Lazarus wcale złe nie są :P - cerrato 2019-01-20 17:36
to już nie te czasy, aby dane pobierać od użytkownika z linii komend - nie mogę się zgodzić. Wiele programów nadal jest wywoływanych z linii komend z różnymi opcjami. I to jest dużo lepsze niż okienka, bo można choćby zastosować pipeline. - Pyxis 2019-01-20 17:46
" I to jest dużo lepsze niż okienka" owszem zgodzę się np. w kontekście skryptów w pythonie, których sam tworzę i wykorzystuję. Niemniej, taki program jak powyżej to pisało się 30 lat temu, a przez trzy dekady technologia poszała jednak trochę do przodu - cw 2019-01-20 18:05

Pozostało 580 znaków

2019-01-20 17:59
4
Adept123 napisał(a):

Witajcie
Pol90 i dla wszystkich innych zainteresowanych wyjaśniam :).
Ta warunek while wyznacza miejsce wstawienia nowego pracownika.
Np. jeśli wstawię Jan Kowalski ur. 01 01 1980 miejsce urodzenia: Warszawa i Jan Kowalski ur. 31 12 1979 miejsce urodzenia: Warszawa, to wpis drugi będzie pierwszy i odwrotnie. Jeśli na końcu dodam Jan Kowalski ur 01 01 1980 miejsce urodzenia: Białystok to ten będzie po środku :).
Pozdrawiam

Abstrahując już od tego, że ten kod jest strasznie przekombinowany i zupełnie nieczytelny, to wypadałoby chociaż przenieść to do jakiejś funkcji i nazwać ją SortujPracownikow albo jakoś tak, żeby mieć mniejszy WTF w przyszłości.

cw napisał(a):
  • to już nie te czasy, aby dane pobierać od użytkownika z linii komend

Dla początkujących te czasy są i będą zawsze. Rzucanie się na jakieś zaawansowane technologie frontendu na początku jest źródłem wielu frustracji i jeszcze większej liczby złych nawyków.


"HUMAN BEINGS MAKE LIFE SO INTERESTING. DO YOU KNOW, THAT IN A UNIVERSE SO FULL OF WONDERS, THEY HAVE MANAGED TO INVENT BOREDOM."
edytowany 1x, ostatnio: somekind, 2019-01-20 18:01

Pozostało 580 znaków

2019-01-21 11:14
1

Jak na pierwszy poważniejszy program to spoko. Staraj się tylko nazywać zmienne w języku angielskim oraz bardziej skomplikowany kod rozbijaj na mniejsze części, refaktoryzuj i upraszczaj. Ten kod w pętli while jest niedopuszczalnie złożony.


Wiedza to potęga

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

1 użytkowników online, w tym zalogowanych: 0, gości: 0, botów: 1

Robot: CCBot