Prosty kalkulator, opinia kodu

Witam!

Jestem początkujący i chciałbym by ktoś zerknął na owy kod i podpowiedział czy jest on w miarę dobrze napisany, optymalny a może da się go zrobić krócej? Doradził co do ewentualnych zmian.

#include <stdio.h>

void wpisz(int *a, int *b){
    printf("Podaj a = ");
    scanf("%d", a);
    printf("Podaj b = ");
    scanf("%d", b);
}

void text(int *n){
    printf("Jakie działanie chcesz wykonać? \n1 - dodawanie\n2 - odejmowanie\n3 - mnozenie\n4 - dzielenie\n5 - reszta z dzielenia\n6 - koniunkcja bitowa(AND)\n7 - alternatywa bitowa(OR)\n8 - alternatywa rozłączna(XOR)\n9 - negacja bitowa\nPodaj numer operacji: ");
    scanf("%d", n);
}

void dod(int a, int b){
    int suma;
    suma = a + b;
    printf("%d + %d = %d \n", a,b, suma);
}

void ode(int a, int b){
    int roznica;
    roznica = a - b;
    printf("%d - %d = %d \n", a, b, roznica);
}

void mnoz(int a, int b){
    int iloczyn;
    iloczyn = a * b;
    printf("%d * %d = %d \n", a, b, iloczyn);
}

void dziel(int a, int b){
    float iloraz;
    iloraz = a / b;
    printf("%d // %d = %.2f\n", a, b, iloraz);
}

void mod(int a, int b){
    int modulo;
    modulo = a % b;
    printf("%d %% %d = %d\n", a, b, modulo);
}

void kon(int a, int b){
    int koniunkcja;
    koniunkcja = a & b;
    printf("%d & %d = %d\n", a, b, koniunkcja);
}
void albit(int a, int b){
    int alternatywab;
    alternatywab = a | b;
    printf("%d | %d = %d\n", a, b, alternatywab);
}

void altroz(int a, int b){
    int rozlaczna;
    rozlaczna = a ^ b;
    printf("%d ^ %d = %d", a, b, rozlaczna);
}

void neg(int a, int b){
    int neg1, neg2;
    neg1 = ~a;
    neg2 = ~b;
    printf("~%d = %d\n~%d = %d\n", a, neg1, b, neg2);
}


int main(){
    
    int a;
    int b;
    int n;
    wpisz(&a,&b);
    text(&n);
    
    switch (n){
        case 1:
            dod(a,b);
            break;
        case 2:
            ode(a,b);
            break;
        case 3:
            mnoz(a,b);
            break;
        case 4:
            dziel(a,b);
            break;
        case 5:
            mod(a,b);
            break;
        case 6:
            kon(a,b);
            break;
        case 7:
            albit(a,b);
            break;
        case 8:
            altroz(a,b);
            break;
        case 9:
            neg(a,b);
            break;
        default:
            printf("Zły wybór!");
        }
    
    return 0;
}

1. Nazewnictwo. ode(), dod()... Po ludzku można to nazwać.
2. Funkcję text() można nazwać menu() i switch przenieść do tej funkcji.
3. Wszystkie funkcje możesz sobie napisać krócej. Nie trzeba tworzyć dodatkowych zmiennych.
   Np.

   void ode(int a, int b){
       printf("%d - %d = %d \n", a, b, a - b);
   }
   

4. menu() możesz wykonywać w pętli while. Żeby można było więcej liczyć w czasie pojedynczego uruchomienia programu. Wyjście z programu byłoby przypisane do kolejnej pozycji menu.
5. Cały kalkulator możesz usprawnić tak, żeby potrafił obliczać wartości wyrażeń matematycznych. Poczytaj o ONP.

@Spine Fajnie zastosowałem się do pierwszych trzech punktów ale nie wiem jak ugryźć czwarty...

while(??){
  menu(a,b); 
}

Od czego uzależnić warunek?

@Sponczuu:

do
{
    wpisz(&a,&b);
}
while (menu(a, b));

Menu może zwracać 0, jeśli wybrano opcję wyjścia z programu. W przeciwnym razie zwracać 1.

Albo w ogóle do menu dodać wpisz.
Zależy czy chcesz, żeby liczby podawać przed każdym działaniem, czy robić różne działania dla jednokrotnie podanych liczb.
Ja bym zrobił menu bez argumentów.

@Spine:
Kurcze nie ograniam jak zwrócić wartość mojej funkcji menu(a,b) skoro ona jest funkcją void

@Sponczuu: Zmień jej typ na int.

@Spine: No niby działa jak powinno ale czy o ten kod jest git?

     case 9:
         negacja(a,b);
         return 1;
     case 10: // 10 - koniec programu(w menu)
         return 0;
     default:
         printf("Zły wybór!\n");
         return 1;
     }
 return 0;

Przede wszystkim zapewnij ze program dziala poprawnie. W szczegolnosci piszac znaczek / lub % zawsze (a co najmniej dla int) musisz sie zastanowic czy mozesz to zrobic.

@eleventeen: No teraz wiem, że nie ;/ Dlaczego mi teraz zaczęło zapętlać program?

 printf("Jakie działanie chcesz wykonać? \n1 - dodawanie\n2 - odejmowanie\n3 (...)

ta linia to jakiś potworek. Weź to jakoś podziel.

void ode(int a, int b){

tworzenie skrótów od polskich nazw to słabiutki pomysł.
Pomyśl o czytelności, że twój kod czyta ktoś, kto widzi to pierwszy raz na oczy. Jak stosujesz skróty, to należy to robić w oczywisty sposób.

Zamiast ode - (jak już idziesz w polski) lepsze byłoby odejmij
ale lepiej napisać po angielsku subtract. Ew. nawet skrótowo sub, ponieważ mul, div czy sub są to utarte skróty i każdy je zrozumie.

natomiast ode to mi się kojarzy prędzej z tym: https://en.wikipedia.org/wiki/Ordinary_differential_equation

void mnoz(int a, int b){
    int iloczyn;
    iloczyn = a * b;
    printf("%d * %d = %d \n", a, b, iloczyn);
}

Mieszasz logikę (operator mnożący dwie liczby) z ze sposobem prezentacji danych (wyświetlanie tekstu na standardowym wyjściu za pomocą printf). A co jeśli będziesz chciał policzyć coś cicho bez wyświetlania tego użytkownikowi? Żeby to osiągnąć, ta funkcja musiałaby wyglądać tak:

int mnoz(int a, int b){
    return a * b;
}

A wywołania printf powinny znaleźć się w innym miejscu programu.

W ten sposób byś mógł oddzielić część programu, gdzie faktycznie coś jest liczone, od kodu, który prezentuje te dane np. wyświetlając je na wejściu.

Wyniesienie drukowania do subfunkcji (choć nie dziwi u początkujących) neguje (prawie) wszystkie zalety podziału na funkcje. Właściwie stają się zabetonowane jako tylko drukujące (i nie zwracające wyniku)

Popieram drugą część wypowiedzi @LukeJL - choć jest błędny typ funkcji voidint

int  mnoz(int a, int b){
    return a * b;
}

Funkcje które przetwarzają dane (tutaj: obliczają) i zwracają wyniki dadzą się wykorzystać w innym zagadnieniu.

Mówiąc o taki kodzie mówimy o częściach kodu "klienckich", w sensie "klient poprosił o obliczenie" a wydrukuje sobie sam (albo nie wydrukuje, albo wydrukuje inaczej, na inny plik, na HTML itd )

@ZrobieDobrze: "Wyniesienie drukowania do subfunkcji (choć nie dziwi u początkujących) neguje (prawie) wszystkie zalety podziału na funkcje. Właściwie stają się zabetonowane jako tylko drukujące (i nie zwracające wyniku)" przez to mam rozumieć, że tworze osobnej funkcji gdzie jest praktycznie tylko drukowanie jakiegoś teksu jest bezsensowne?

Jeśli stworzysz osobną funkcję, która jedynie drukuje jakiś tekst, np. w ładniejszy sposób i jest reużywalna to taka funkcja ma jak najbardziej sens (np. drukuj(), printNumber()). Dobra rada ode mnie: Jeśli Twoja funkcja mnoz mnożyła dwie liczby oraz je drukowała to robiła więcej niż wynikało by to z jej nazwy. Powinna robić jedynie mnożenie i zwracać wynik z operacji. W ten sposób wykonuje czynność zawartą w nazwie, robi to dobrze i robi tylko tyle. Notabene zmień tą nazwę, bo jest paskudna (na chociaż pomnoz, ale lepiej multiply ) .

Teraz coś lepiej to wygląda?

Funkcja dzielenie() może być tak zapisana (return (float)a / b)?
Jak mogę zmienić funkcję negacja?
Czy w switch'u te return'y są git?
I jeszcze czy która opcja jest lepsza switch czy if'y?

#include <stdio.h>
#include <stdlib.h>

void wpisz(int *a, int *b){
    printf("Podaj a = ");
    scanf("%d", a);
    printf("Podaj b = ");
    scanf("%d", b);
}

int dodawanie(int a, int b){
    return a + b;
}

int odejmowanie(int a, int b){
    return a - b;
}

int mnozenie(int a, int b){
    return a * b;
}

float dzielenie(int a, int b){
    return (float)a / b;
}

int modulo(int a, int b){
    return a % b;
}

int koniunkcja(int a, int b){
    return a & b;
}
int alternatywa(int a, int b){
    return a | b;
}

int alternatywaroz(int a, int b){
    return a ^ b;
}

void negacja(int a, int b){
    int neg1, neg2;
    neg1 = ~a;
    neg2 = ~b;
    printf("~%d = %d\n~%d = %d\n", a, neg1, b, neg2);
}

int menu(int a, int b){
    int n;
    scanf("%d", &n);
    switch (n){
        case 1:
            printf("%d + %d = %d\n", a, b, dodawanie(a,b));
            return 1;
        case 2:
            printf("%d - %d = %d\n", a, b, odejmowanie(a,b));
            return 1;
        case 3:
            printf("%d * %d = %d\n", a, b, mnozenie(a,b));
            return 1;
        case 4:
            printf("%d / %d = %.2f\n", a, b,dzielenie(a,b));
            return 1;
        case 5:
            printf("%d %% %d = %d\n", a, b, modulo(a,b));
            return 1;
        case 6:
            printf("%d & %d = %d\n", a, b, koniunkcja(a,b));
            return 1;
        case 7:
            printf("%d | %d = %d\n", a, b, alternatywa(a,b));
            return 1;
        case 8:
            printf("%d ^ %d = %d\n", a, b,alternatywaroz(a,b));
            break;
        case 9:
            negacja(a,b);
            return 1;
        case 10:
            return 0;
        default:
            printf("Zły wybór!\n");
            return 1;
        }
    return 0;
}

int main(){
    int a;
    int b;
    do
    {
        wpisz(&a,&b);
        printf("Jakie działanie chcesz wykonać? \n1 - dodawanie\n2 - odejmowanie\n3 - mnozenie\n4 - dzielenie\n5 - reszta z dzielenia\n6 - koniunkcja bitowa(AND)\n7 - alternatywa bitowa(OR)\n8 - alternatywa rozłączna(XOR)\n9 - negacja bitowa\n10 - Koniec programu\nPodaj numer operacji: ");
    }
    while (menu(a, b));
    return 0;
}

case 1:
     printf("%d + %d = %d\n", a, b, dodawanie(a,b));
     return 1;
 case 2:
     printf("%d - %d = %d\n", a, b, odejmowanie(a,b));
     return 1;
 case 3:
     printf("%d * %d = %d\n", a, b, mnozenie(a,b));
     return 1;

Zobacz, że tutaj masz duplikację kodu. Wszystkie te printfy odbywają się wg tego samego schematu a operator b = c . Da się to zapisać jako (poza dzieleniem, bo tam masz wynik we floatach*):

printf("%d %s %d = %d\n", a, operator, b, c);

więc możesz ten printf wyciągnąć poza switcha, jedynie przypisując operator i wynik odpowiednio.

void negacja(int a, int b){
    int neg1, neg2;
    neg1 = ~a;
    neg2 = ~b;
    printf("~%d = %d\n~%d = %d\n", a, neg1, b, neg2);
}

tutaj na siłę robisz z negacji operator dwuargumentowy, czyli twoja funkcja negacja liczy negację dwóch liczb naraz. trochę bez sensu

*swoją drogą czy nie lepiej przejść również na floaty w dodawaniu/mnożeniu/odejmowaniu? Tylko wtedy i tak by ci zostały inty w operacjach bitowych koniunkcja, alternatywa itp.

Ładnie wykorzystujesz wskaźniki unikając zmiennych globalnych, tylko nazwy słabe jak inni już pisali no i nie masz zabezpieczenia przed dzielenie przez zero. A przy wyświetlaniu menu mógłbyś podzielić ten tekst, o ile oczywiście twój kompilator taką składnię obsługuje:

printf ("Jakie działanie chcesz wykonać? \n"
        "1 - dodawanie\n2 - odejmowanie\n3 - mnozenie\n4 - dzielenie\n"
        "5 - reszta z dzielenia\n6 - koniunkcja bitowa(AND)\n7 - alternatywa bitowa(OR)\n"
        "8 - alternatywa rozłączna(XOR)\n9 - negacja bitowa\n10 - Koniec programu\n"
        "Podaj numer operacji: ");

@LukeJL:

printf("%d %s %d = %d\n", a, operator, b, c);

Tutaj operator mógłby być znakiem, bo wśród zestawu operacji nie ma dwuznakowych operatorów - więc %c a nie %s.