Nietypowe deklaracje zmiennych

[Gość]

8 godzin temu, jand napisał:

Tak jak ludzie masowo robiący telefonami zdjęcia obiecują sobie, że potem je przejrzą, posegregują - a ilu to naprawdę robi?

Swojego czasu kiedy pisałem proste artykuły z wykonywanych prac serwisowych, to mało tego, że fotki miałem posegregowane, to i względnie opisane aby przypomnieć sobie szczegóły odnośnie tego co było w danym momencie zrobione. Natomiast co do kodu, to chciałbym wypróbować standard w którym kod programu będę miał w miarę prosto opisany, ale same funkcje czy też poboczne linijki możliwie pozawijane, tak aby to się nie plątało po ekranie. Będzie po prostu zaznaczone gdzie się zaczyna i kończy, tak aby tego fragmentu kodu nie analizować, tylko uwzględniać to co on realizuje. Zresztą nawet w tym poradniku jest napisane, aby kod poboczny typu funkcje zbytnio nie rozpisywać oraz robić możliwie krótko z krótkimi jasnymi opisami.

[spook]

Dnia 13.05.2024 o 20:44, rafal220 napisał:

Nie wiem co masz na myśli, skoro funkcja działa prawidłowo.  Zresztą poprawiłem jeszcze jedną rzecz, i jest całkiem ok.

/* HiB  */               
byte oneshot_hib(byte hib) { 
  
static byte d1 = 0;
byte ret = 0;

if (hib & 1) {ret = hib >> d1; d1=1;}
else {d1 = 0;}  
return ret;
}
/* HiB  */

Nazwa funkcji jest intuicyjna, a kod jest możliwie ściśnięty.  

To może ja się wypowiem, co jest nie tak. Kryterium, że kod jest dobry, jeśli tylko działa prawidłowo, było dobre jakieś dwie, a może nawet trzy dekady temu. Obecnie kod jest dobry, jeśli jest przede wszystkim czytelny, utrzymywalny i rozwijalny - to, że działa, to jest bezwzględny wymóg, a nie zaleta lub wada. Jest takie powiedzenie: "Każdy głupi umie napisać kod, który rozumie komputer. Dobrzy programiści piszą kod, który rozumieją ludzie".

Co więc dokładnie jest nie tak z Twoim kodem:

• Nazwa funkcji nie oddaje intencji - czyli tego, co funkcja ma robić.
• Nazwy zmiennych są skrótowe lub składają się ze sklejenia liter i cyfr ("d1"). Nie wiadomo, co reprezentują.
• Kod jest źle sformatowany, zawartość funkcji powinna zostać wcięta o jedną tabulację lub 4 spacje. Blok po if i else powinien być też w osobnej linii. Zawartości bloków po if i else również wcięte.
• Statyczna zmienna upchnięta w funkcji utrudnia analizę jej działania - lepiej już ją wypchnąć do wyższego zakresu (np. jako zmienną globalną)
• byte nie jest standardowym typem C++ - powinno być unsigned char albo ewentualnie uint8_t. Redefiniowanie istniejących typów bez wyraźnej przyczyny wpływa negatywnie na czytelność kodu.
• Za więcej niż jedno polecenie w jednej linijce cofam każdy code review, który dostaję do zrobienia.
• Kod nie ma być pisany tak, żeby zajmował mało miejsca, tylko żeby był czytelny. Od właściwej prezentacji jest przyzwoity edytor (np. Visual Studio Code). Tam funkcje zwija się jednym kliknięciem i nie zajmują miejsca.

Jeżeli dobrze zrozumiałem ten kawałek kodu, to takie rzeczy implementuje się wzorcem maszyny stanu. Bardzo prosta implementacja może wyglądać tak:

 

bool outputting = false;
const int SEQUENCE_LENGTH = 3;
const unsigned char * sequence = new unsigned char[SEQUENCE_LENGTH] { 0, 1, 0 };
int index = 0;

unsigned char output_sequence_on_low_bit_lit(unsigned char & input) {
	
  if (!outputting && (input & 1 == 1)) {
    outputting = true;
    index = 0;
  }
  
  if (outputting) {
    unsigned char result = sequence[index];
    index++;
    
    if (index >= SEQUENCE_LENGTH) {
    	input >>= 1;
      	outputting = false;
    }
    
    return result;
  } else {
    return 0;
  }
}

Tak naprawdę to te zmienne globalne też rażą w oczy. Aż się prosi, żeby zamknąć cały ten kod w osobnej klasie.

class SequenceOutputter {
private: 
    const int SEQUENCE_LENGTH = 3;
    const unsigned char * sequence = new unsigned char[SEQUENCE_LENGTH] { 0, 1, 0 };

    bool outputting = false;
    int index = 0;

public:
    unsigned char output_sequence_on_low_bit_lit(unsigned char & input) {
	
    if (!outputting && (input & 1 == 1)) {
      outputting = true;
      index = 0;
    }
  
    if (outputting) {
      unsigned char result = sequence[index];
      index++;
    
      if (index >= SEQUENCE_LENGTH) {
    	input >>= 1;
      	outputting = false;
      }
    
      return result;
    } else {
      return 0;
    }
  }
}

// (...)

SequenceOutputter outputter;
auto result = outputter.output_sequence_on_low_bit_lit(some_input);

Kod pisałem na kolanie, więc nie gwarantuję, że działa - poza tym trochę zgaduję przeznaczenie pierwotnej funkcji, bo szczerze mówiąc nie chcę mi się jej debuggować na kartce.

Zdementujmy też może od razu kilka mitów.

Mnie krótkie nazwy nie przeszkadzają, wiem, co ten kod robi.

Owszem - teraz. Za trzy miesiące nie będziesz miał zielonego pojęcia i wtedy skrótowe nazwy na pewno nie pomogą w analizie opisanej funkcji (wiem z doświadczenia). Poza tym zwróć uwagę, że chwalisz się funkcją na forum. W takim przypadku Twoim priorytetem powinno być napisanie jej w taki sposób, żeby ludzie na forum ją jak najszybciej zrozumieli, bo wtedy też Ci najszybciej pomogą. Co oznacza, że to przede wszystkim oni mają ją rozumieć, a nie Ty. Współcześnie podczas pisania poważnych projektów programistycznych zakłada się a priori, że pisany przez kogoś kod będą czytali PRZEDE WSZYSTKIM inni programiści. Jednoliterowe lub skrótowe identyfikatory to też już przeszłość, identyfikator ma mówić, czym obiekt jest, a nie - być rekordowo krótki. Na przykład w OpenMacroKeyboard, o którym pisałem w innym wątku mam interfejs o nazwie IMacroKeyboardPreviewWindowAccess - a to nie jest wcale najdłuższa nazwa, jaką napisałem. Nazwy metod testów jednostkowych to wręcz całe zdania opisujące działanie testu (i to są współczesne normy programowania, a nie moje wymysły, żeby było jasne).

Dzięki krótkim nazwom i sklejeniu kilku poleceń w tym samym wierszu kod jest zwięzły.

Nie jest zwięzły, tylko sztucznie skrócony, to po pierwsze. Żaden znany mi standard formatowania kodu - zarówno C++, jak i każdego innego języka, który znam, nie dopuszcza pisania kilku poleceń w jednym wierszu. Po drugie, od prezentowania kodu jest dobry edytor, a nie kod jako taki. Współczesne IDE jak np. wspomniany wcześniej Visual Studio Code potrafią zrobić cuda w zakresie czytelnego prezentowania plików z kodem źródłowym. Twój kod traci na czytelności po to, żeby osiągnąć coś, co większość IDE załatwia od ręki. Ba, nawet mój Dev.Editor umie składać bloki w C-podobnych językach.

Mój kod jest wydajniejszy niż twój, bo jest krótszy.

Dwie albo trzy dekady temu mogłoby tak być. Współcześnie optymalizatory wbudowane w kompilatory robią takie cuda, że ręczne optymalizowanie kodu często przynosi więcej szkody niż pożytku i zazwyczaj jest pozbawione sensu. "Ręczna" optymalizacja zazwyczaj utrudnia pracę optymalizatora, co powoduje, że kod - paradoksalnie - może być wolniejszy.

Teraz jest już jasne, co mamy na myśli?

Pozdrawiam -- Spook.

[Gość]

16 godzin temu, spook napisał:

Teraz jest już jasne, co mamy na myśli?

Niby jasne. Problem w tym, że analiza Twojego kodu (nawet gdybym go rozumiał) zajęłaby mi 10 razy więcej czasu. Takie pytanko. W jaki sposób sprawdziłeś, że Twój kod jest wydajniejszy? Masz do tego jakiś symulator? Czy może potrafisz takie rzeczy robić na oko?  Zresztą żeby pisać taki kod jaki przedstawiłeś, to najpierw trzeba nauczyć się szybkiego czytania podobnej składni. Pisanie kodu w taki sposób na chwilę obecną nie jest na moje siły. 

 

[Gość]

21 godzin temu, spook napisał:

Mój kod jest wydajniejszy niż twój, bo jest krótszy.

Tutaj, to raczej kompilator i tak wszystko optymalizuje. Czy krótko napisane, czy też długo. Tylko postaw się na moim miejscu. Nie zaprzeczam temu co napisałeś, ale jedną linijkę rozpisałeś na całą stronę zaznaczając iż to jest tylko fragment kodu funkcji której interesuje Ciebie działanie, a nie dokładnie to, co realizuje zawarty w niej kod. 

Ja na chwilę obecną rozpisałem sobie podstawowe funkcje jakimi posługuję się w PLC.

// 
/* HiB  */               
uint8_t oneshot_hib(uint8_t cl_hib) { 
  
static uint8_t d1 = 0;
uint8_t ret = 0;

if (cl_hib & 1) {ret = cl_hib >> d1; d1=1;} // HiB (HighBit)
else {d1 = 0;}  
return ret;
}
/* HiB  */

/* LoB  */               
uint8_t oneshot_lob(uint8_t cl_lob) { 
  
static uint8_t d1 = 0;
uint8_t ret = 0;

if (!cl_lob & 1) {ret = !cl_lob >> d1; d1=1;} // LoB (LowBit)
else {d1 = 0;}  
return ret;
}
/* LoB  */

/* LHB  */
uint8_t oneshot_lhb(uint8_t cl_lhb) {

static uint8_t d1 = 0;
uint8_t ret = 0;

if (cl_lhb ^ d1) {ret=1; d1=!d1;} // LHB (LowHighBit)
return ret;  
}
/* LHB */

/* flip-flop RS */
uint8_t flipflop_RS(uint8_t cl_r, uint8_t cl_s) {

static uint8_t ret = 0;

if (cl_r & 1){ret = 0;} 
else if (cl_s & 1){ret = 1;} // przezutnik RS 
return ret;  
}
/* flip-flop RS */

/* flip-flop T */
uint8_t flipflop_T(uint8_t cl_T){

static uint8_t d1 = 0;
static uint8_t ret = 0; 
 
if (cl_T & d1){ret = !ret;}  // przerzutnik T wyzwalanie H
if (cl_T & 1){d1 = 0;}
else {d1 = 1;}
return ret;  
}
/* flip-flop T */

/* delayed switching ON */
uint8_t timer_on(uint8_t cl_switchingTime, int switchingTime){ // dobrać typ zmiennej do wielkości zegara
  
// cl_switchingTime 0 = false, 1 = true; switchingTime value (ms)

static unsigned long t0 = 0;   
static unsigned long t1 = 0;
static uint8_t d1 = 0;
static uint8_t ret = 0;
 
if (cl_switchingTime & 1) {t0 = millis() - t1; 
if (t0 >= switchingTime) {ret = 1;}}              // opóźnione załączenie
else {t1 = millis(); ret = 0;}
return ret;
}
/* delayed switching ON */

/* delayed shutdown */
uint8_t timer_of(uint8_t cl_shutdownTime, int shutdownTime){   // dobrać typ zmiennej do wielkości zegara
  
// cl_shutdownTime 0 = false, 1 = true; shutdownTime value (ms)

static unsigned long t0 = 0;   
static unsigned long t1 = 0;
static uint8_t d1 = 0;
static uint8_t ret = 0;
 
if (cl_shutdownTime & 1) {t1 = millis(); ret = 1;}       
else if (t0 >= shutdownTime) {ret = 0;} 
t0 = millis() - t1;                    // opóźnione wyłączenie
return ret;
}
/* delayed shutdown */

/* standard timer */
uint8_t timer(uint8_t cl_time, int timeON, int timeOF){   // dobrać typ zmiennej do wielkości zegara
  
// cl_time 0 = false, 1 = true; timeof & timeon value (ms)

static unsigned long t0 = 0;   
static unsigned long t1 = 0;
static uint8_t d1 = 0;                   // timer klasyczny
static uint8_t ret = 0;
 
if (cl_time & 1 && t0 >= timeON + timeOFF) {ret = 0; t1 = millis();}       
else if (cl_time & 1 && t0 >= timeON) {ret = 1;}
if (cl_time == 0) {t1 = millis(); ret = 0;}
t0 = millis() - t1;                    
return ret;
}
/* standard timer */

/* timer with a continue */
uint8_t contitimer(uint8_t cl_contitime, int contitimeON, int contitimeOFF){   // dobrać typ zmiennej do wielkości zegara
  
// cl_time 0 = false, 1 = true; contitimeOFF & contitimeON value (ms)

static unsigned long t0 = 0;   
static unsigned long t1 = 0;
static uint8_t d1 = 0;                   // timer klasyczny z kontynuacją czasu
static uint8_t ret = 0;

 
if (cl_contitime & 1 && t1 >= contitimeON + contitimeOFF) {ret = 0; t0 = millis();}       
else if (cl_contitime & 1 && t1 >= contitimeON) {ret = 1;} 
if (cl_contitime == 1) {t1 = millis() - t0;}
else {t0 = millis() - t1;}               
return ret;
}
/* timer with a continue */
//

 Są po to gotowe biblioteki, jednak ja pragnąłem samemu to rozpisać zważywszy na to, że są to najprostsze rzeczy. Sam zobacz. Wyszła cała strona dla kilku podstawowych algorytmów jakie stosuje się w PLC których nie ma w bibliotece podstawowej. Gdybym to rozpisał w sposób jaki zaprezentowałeś, to normalnie dostałbym oczopląsu i nie wiem czy nie prościej byłoby jednak skorzystać z gotowej biblioteki niż to rozpisywać w taki sposób.   

W powyższym kodzie mogą być błędy, ponieważ pozamieniałem nazwy zmiennych, albo ich typy z byte na uint8_t, chociaż w opisie Arduino nie wspomnieli, że od byte powoli się odchodzi, a z kolei bool nie chcę stosować, ponieważ zwrócona wartość z funkcji w tej postaci wygeneruje mi błąd dla operacji przesunięć bitowych.  

[Elvis]

@rafal220 Tak z ciekawości zapytam - po co umieszczasz te kody na forum?

Nie chcesz niczyjej rady, bo sam wiesz wszystko najlepiej. Zakładam więc, że nie oczekujesz pomocy. Z drugiej strony kod który prezentujesz jest tak koszmarny, że raczej nikt z niego nie skorzysta, czyli nie robisz tego żeby innym pomóc. Możesz mi wierzyć, że chwalić się nie ma czym - więc pytanie: po co to publikujesz? 

[Gość]

@Elvis Znajdę czas, to dokładnie opiszę wszystkie zmienne i jeszcze raz sprawdzę czy wszystko działa. Same nazwy funkcji są dość powszechnie stosowane w środowisku PLC. Natomiast tutaj mogę to opisać skoro terminologia jest obca.   

P.S.

Można wcisnąć ctrl + T. Kod się ułoży po twojemu.

Edytowano Maj 15, 2024 przez rafal220

[Santiago]

12 godzin temu, rafal220 napisał:

Znajdę czas, to dokładnie opiszę wszystkie zmienne

Każdy z nas ma wiele rzeczy na głowie i raczej nikt nie będzie uczył się Pana nazw zmiennych. Raczej należy dostosować się do większości. 

Ja podobnie uwielbiam krótkie nazwy i zwięzły kod , jednak jak zawodowcy piszą , że tak jest źle to chyba nie ma sensu dyskutować.

Powodzenia !

[Gość]

21 minut temu, Santiago napisał:

Każdy z nas ma wiele rzeczy na głowie i raczej nikt nie będzie uczył się Pana nazw zmiennych.

Co stoi na przeszkodzie aby kolega pozmieniał sobie nazwy zmiennych na bardziej przystępne? Skomplikowanej filozofii w kodzie który przedstawiłem raczej nie ma. 

23 minuty temu, Santiago napisał:

Ja podobnie uwielbiam krótkie nazwy i zwięzły kod , jednak jak zawodowcy piszą , że tak jest źle to chyba nie ma sensu dyskutować.

Tylko że ci sami zawodowcy w podobny sposób zaczynali przygodę. Dlaczego? Bo na początku łatwiej jest zrozumieć sam mechanizm działania C kiedy nie zaśmiecasz umysłu stosem zbędnych informacji. No ale skoro już przeskoczył kolega ten etap oraz jest w stanie ogarniać (podobnie jak zawodowcy) fizykę C wraz z analizą oraz interpretacją długich nazw zmiennych, to nie pozostało mi nic innego, jak życzyć powodzenia w dalszej drodze edukacji.  

[FlyingDutch]

Cześć,

trochę nie rozumiem po co Ci w programie w języku C++ przerzutniki RS lub T (wyzwalane poziomem):

/* flip-flop RS */
uint8_t flipflop_RS(uint8_t cl_r, uint8_t cl_s) {

static uint8_t ret = 0;

if (cl_r & 1){ret = 0;} 
else if (cl_s & 1){ret = 1;} // przezutnik RS 
return ret;  
}
/* flip-flop RS */

/* flip-flop T */
uint8_t flipflop_T(uint8_t cl_T){

static uint8_t d1 = 0;
static uint8_t ret = 0; 
 
if (cl_T & d1){ret = !ret;}  // przerzutnik T wyzwalanie H
if (cl_T & 1){d1 = 0;}
else {d1 = 1;}
return ret;  
}
/* flip-flop T */

Już wiele lat programuję w C i nigdy nie były mi potrzebne w programie przerzutniki. Zrozumiałbym gdybyś używał przerzutników w "programie" w językach VHDL lub Verilog dla ukladów FPGA. Dla mikrokontrolerów raczej używa się pinów przerwań zewnętrznych (gdzie mogą one być wyzwalane dowolnym zboczem). Możesz podać przykład programu, gdzie masz zamiar wykorzystać takie przerzutniki?

BTW: podobnie jak @Elvis i inii koledzy sygnalizowali - uważam, że podawany przez Ciebie kod jest bardzo słabej jakości.

Pozdrawiam

[Elvis]

Niska jakość tego kodu nie dotyczy tylko formatowania, czy też nazewnictwa zmiennych. To po prostu bardzo kiepski kod, który można byłoby poprawiać, ale chyba nie to było intencją autora i celem umieszczania kodu na forum.

Tak żeby łatwiej było zrozumieć co mam na myśli, jedno pytanie pomocnicze - jak za pomocą tej "biblioteki" utworzyć projekt, który będzie miał 2 przerzutniki RS?

[Gość]

56 minut temu, Elvis napisał:

jak za pomocą tej "biblioteki" utworzyć projekt, który będzie miał 2 przerzutniki RS?

Wyprzedziłeś mnie, bo sam miałem zadać to pytanie. W PLC jest tak, że raz prosto zbudowany kod możesz używać w prosty sposób w dowolnym miejscu n razy. A w C ?  Czy jeżeli zmienna nr 1 wywoła funkcję, to jak tą funkcje poprawnie wywołać w prosty sposób w działaniu ze zmienną nr2 ? Sam kod jest dobry, bo działa, ale każde wywołanie funkcji będzie się wiązać z przypisaniem kolejnych numerów do zmiennych. 

1 godzinę temu, FlyingDutch napisał:

trochę nie rozumiem po co Ci w programie w języku C++ przerzutniki RS lub T (wyzwalane poziomem):

RS względnie jest przydatny (po prostu sprawdziłem działanie kodu), natomiast T może się przydać. Np. Raz naciskasz switch LED się zapala. Ponownie wciskając switch LED gaśnie. Zatrzaskiwanie zmiennej w przerzutniku T czasem może się przydać. 

[Elvis]

8 minut temu, rafal220 napisał:

Sam kod jest dobry, bo działa

Nie, to zupełnie nieprawda. Lepszy jest kod, który nie działa i wystarczy poprawić błąd niż kod który się do niczego nie nadaje, nawet jeśli wydaje się działać poprawnie.

[Gość]

@Elvis Na chwilę obecną lepszego nie wymyśle. Zależy mi tylko na tym, aby te funkcje w łatwy sposób deklarować w dowolnych miejscach w kodzie tak aby nie trzeba było zmieniać nazw zmiennych w samej funkcji.

[FlyingDutch]

34 minuty temu, rafal220 napisał:

RS względnie jest przydatny (po prostu sprawdziłem działanie kodu), natomiast T może się przydać. Np. Raz naciskasz switch LED się zapala. Ponownie wciskając switch LED gaśnie. Zatrzaskiwanie zmiennej w przerzutniku T czasem może się przydać. 

Podaj konkretny przykład zastosowania RS. Dla przerzutnika typu T - wystarczy wykrywać zmianę stanu switcha i negować za każdym razem zmienną ze stanem diody LED. Naucz się korzystać z możliwości języka C, oraz z API Arduino do wykonywania prostych zadań, a nie tworzysz miliony dziwnych funkcji (to nie jest PLC, tylko mikrokontroler).

Tutaj masz link do "Arduino Reference" (funkcje ułatwiające programowanie w Arduino):

https://www.arduino.cc/reference/en/

BTW: poczytaj sobie o parametrach wejściowych i wyjściowych funkcji w C/C++

Pozdrawiam

Edytowano Maj 16, 2024 przez FlyingDutch
update

[Gość]

56 minut temu, FlyingDutch napisał:

Dla przerzutnika typu T - wystarczy wykrywać zmianę stanu switcha i negować za każdym razem zmienną ze stanem diody LED

To przecież tak jest to zrobione, tylko negujesz w kółko return a nie wyjście LED. 

57 minut temu, FlyingDutch napisał:

Tutaj masz link do "Arduino Reference" (funkcje ułatwiające programowanie w Arduino):

Ten link to podstawa. Dodałem skrót jako pierwszy, przed dyskusją na forum.