Witam! Postanowiłem napisać prosty program konsolowy w ,którym będę mógł poćwiczyć programowanie obiektowe. Mój wybór padł na kalkulator. Chciałem się spytać Was bardziej doświadczonych programistów o opinię na temat mojego kodu. Wszelka krytyka mile widziana! Z góry dziękuje za odpowiedź.
0
0
W ifach i switchach powtarzasz 90% kodu.
Polecam -> https://pl.wikipedia.org/wiki/DRY
Reszty nie sprawdzałem.
1
Wiele tu do okomentowania, ale nie wystarczy mi czasu na wszystko. Pierwsze z brzegu:
using System;
using System.Collections.Generic;
using System.Collections;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace DragonGame
{
class Program
{
static void Main(string[] args)
{
User person = new User(); //możesz użyć var person = new User();
Calculations calculator = new Calculations(); //jw.
ArrayList counts = new ArrayList(); //nie używaj ArrayList - lepsze jest List<T>
double[] number;
int choice = 0;
do // złe wcięcie - to jest ten sam poziom co choice
{
person.menuList(); //metody w C# nazywamy z wielkiej litery - zawsze
choice = person.getOptionMenuList();
switch (choice)
{
case 1: // "magic numbers" - użyj tutaj enuma
{
counts = person.getNumbers(true);
number = person.changeListToArray(counts);
Console.WriteLine("The result of adding: {0}",calculator.Addition(number));
break;
}
case 2:
{
counts = person.getNumbers(true);
number = person.changeListToArray(counts);
Console.WriteLine("The result of substraction: {0}", calculator.Substraction(number));
break;
}
case 3:
{
counts = person.getNumbers(true);
number = person.changeListToArray(counts);
Console.WriteLine("The result of multiplication: {0}", calculator.Multiplation(number));
break;
}
case 4:
{
counts = person.getNumbers(true);
number = person.changeListToArray(counts);
Console.WriteLine("The result of division: {0}", calculator.Division(number));
break;
}
case 5:
{
counts = person.getNumbers(false);
number = person.changeListToArray(counts);
Console.WriteLine("The result of root: {0}", calculator.Root(number));
break;
}
case 6:
{
counts = person.getNumbers(false);
number = person.changeListToArray(counts);
Console.WriteLine("The result of exponentiation: {0}", calculator.Pow(number));
break;
}
case 7:
{
person.goodBye();
choice = 7;
break;
}
default: { break; }
}
//zbędna pusta linia
//zbędna pusta linia
Console.ReadKey();
} while ( choice != 7 );
//zbędna pusta linia
}
}
}
cdn...
0
Dzięki za odpowiedź, ale dręczy mnie jeden temat. Nie mogę sobie poradzić z zastąpieniem bloków w elemencie case. Mam świadomość, że muszę stworzyć nową metodę i przekazać odpowiednie parametry, lecz nie wiem od jakiego argumentu metody uzależnić wybór konkretnego działania (dodawanie, odejmowanie, dzielenie).
0
Przy tak krótkim kodzie upchanie obliczeń w main() to Twój najmniejszy problem, na początek wywal po prostu cały powtarzalnych kod poza switcha.
0
Zlikwiduj tego switcha, jest niepotrzebny i brzydki.
0
No dobra to wyrzucam switcha? I co dalej ? Maciej Cąderek mam usunąć instancje obiektów i deklaracje tablicy i zmiennej ?
0
mam tyle zastrzeżeń że nie chce mi się nawet wypisywać
moc copy-pasta w Tobie silna
ucz się dalej, ale wyrzuć książki z których się uczysz bo wygląda na to że są bardzo stare - używasz konstrukcji chyba tylko z C# 1.0 a już mamy wersję 6.0
0
A znasz jakieś konkretne kursy internetowe, książki (może być język angielski). Bo najwidoczniej sam natrafiałem na nieodpowiednie literatury. A nie chcę znów uczyć się z nieodpowiednich źródeł.
0
C# 6 in Nutshell, może być dla 5 nawet. Na początek ciekawą pozycję może być też Head First C#.
I polecam obie w wersji angielskiej, mimo że 2gą znajdziesz po polsku też.
0
//możesz użyć var person = new User();
A co mu to da ze zamieni na var ? Bo raczej nic. Bez sensu takie zmienianie na sile
2
Nic nie zmienia bo to tylko User a nie Dictionary<string, MyClass>, jedynie jedna litera mniej do pisania, ale osobiście też dałbym var.
U mnie to raczej takie przyzwyczajenie bo Resharper się czepia, to i sam się zacząłem tak pisać.
1
Mały Szczur napisał(a):
A co mu to da ze zamieni na var ? Bo raczej nic. Bez sensu takie zmienianie na sile
A po co myć dzieci? Przecież zawsze można zrobić nowe.
0
Czyli panowie głównie chodzi o powtarzający się kod w klasie Calculation, metodach używając if i case oraz nie poprawne odstępy. I co jeszcze ? Bo bardzo bym chciał wiedzieć.
2
Może wkleje tu cały kod bo nie ma tego jakoś dużo, a pewnie nie wszystkim chce się ściągać.
class User
{
public void menuList()
{
Console.WriteLine("\t\t\t\t KALKULATOR");
Console.WriteLine("\t\t\t 1.Dodawanie");
Console.WriteLine("\t\t\t 2.Odejmowanie");
Console.WriteLine("\t\t\t 3.Mnożenie");
Console.WriteLine("\t\t\t 4.Dzielenie");
Console.WriteLine("\t\t\t 5.Pierwiastkowanie (drugiego stopnia)");
Console.WriteLine("\t\t\t 6.Potęgowanie");
Console.WriteLine("\t\t\t 7.Exit");
}
public void goodBye()
{
Console.WriteLine();
Console.WriteLine();
Console.WriteLine("\t\t\t Exit the program.");
Console.WriteLine();
Console.WriteLine();
}
public int getOptionMenuList(){
int choice = getValue("Choice: ");
while (!checkChoice(choice))
{
choice = getValue("Choice: ");
if (checkChoice(choice)) break;
}
return choice;
}
private bool checkChoice(int choice)
{
if (choice > 0 && choice <= 7) { return true; }
else return false;
}
private int getValue(string inscription)
{
do
{
int choice;
Console.Write(inscription);
string tmp = Console.ReadLine();
int.TryParse(tmp, out choice); // sprawdza czy można zamienić teks na liczbę. 0 oznacza, że nie można zamienić na liczbę.
if (choice != 0) return choice;
} while(true);
}
private void AskCount(bool value)
{
if (value) {
Console.WriteLine();
Console.WriteLine("Unsubscribe all count.");
Console.WriteLine();
}
else
{
Console.WriteLine();
Console.WriteLine("You mast enter two numbers.");
Console.WriteLine();
}
}
public ArrayList getNumbers(bool value)
{
AskCount(value);
ArrayList lista = new ArrayList();
do
{
double tmp = getValue("Count: ");
lista.Add(tmp);
if (!value && lista.Count == 2)
{
Console.Clear();
return lista;
}
if (value && lista.Count >= 2 )
{
Console.WriteLine("Would you like to interrupt inscribing count, click key 't' ");
ConsoleKeyInfo interruptAddCount;
interruptAddCount = Console.ReadKey();
Console.WriteLine();
if (interruptAddCount.Key == ConsoleKey.T)
{
Console.Clear();
return lista;
}
}
} while (true);
}
public double[] changeListToArray(ArrayList value)
{
double[] array = new double[value.Count];
for (int x = 0; x < value.Count; x++)
{
array[x] = (double)value[x];
}
return array;
}
}
class Calculations
{
private void DisplayError()
{
Console.WriteLine();
Console.WriteLine("/t/t/t There were too few data to execute the operation");
Console.WriteLine();
}
public double Addition(double[] value)
{
if (value.Length > 0) {
double addition = 0;
foreach (double elementArray in value)
addition += elementArray;
return addition;
}
DisplayError();
return 0;
}
public double Substraction(double[] value)
{
if (value.Length > 0)
{
double sub = 0;
foreach (double elementArray in value)
sub -= elementArray;
return sub;
}
DisplayError();
return 0;
}
public double Multiplation(double[] value)
{
if (value.Length > 1)
{
double multi = value[0];
for (int elementArray = 0; elementArray < value.Length; elementArray++) {
multi *= value[elementArray];
}
return multi;
}
DisplayError();
return 0;
}
public double Division(double[] value)
{
if (value.Length > 1)
{
double div = value[0];
for (int elementArray = 1; elementArray < value.Length; elementArray++)
{
div /= elementArray;
}
return div;
}
DisplayError();
return 0;
}
public double Root(double[] value)
{
if (value.Length >= 0) return Math.Sqrt(value[0]);
DisplayError();
return 0;
}
public double Pow(double[] value)
{
return Math.Pow(value[0],value[1]);
}
}
Klasa User wypisuje menu w konsoli, takie coś to powinno być w klasie Menu jeśli już.
Metoda getOptionmenuList, dwa razy wpisujesz z palca w parametrze ten sam string, on powinien być jako pole klasy. Nie musisz nawet go podawać dwa razy bo tą pętla można zamienić na do while która się tu lepiej nadaje.
Metoda checkChoice, a co jeśli dodasz do kalkulatora jeszcze jedną operacje? Będziesz musiał pamiętać aby tu zmienić.
getValue, tu o dziwo potrafiłeś zastosować do while
AskCount, nie ogarniam co to robi, mało mówiąca nazwa parametru i g**no mówiąca nazwa metody.
changeListToArray to do wylotu, jak byś używał List<double> to aby zamienić na tablice wystarczy .ToArray(), albo nie trzeba w ogóle tego zamieniać.
W klasie Calculations metody powinny się nazywać jakoś w stylu ZróbCoś a twoje się nazywają Coś, powinno być np Divide, Multiply
Pewnie jeszcze coś by się znalazło.
Aha, coś mnie oświeciło, wydaje mi się że tłumaczysz sobie słówko Count jako liczba, tymczasem to oznacza liczyć.
0
Dzięki za konkretną odpowiedź.
1
tryParse zwaraca wartośc bool na końcu nie musisz przyrównywać wyniku parsowania do zera. Poza tym co jeśli ktoś faktycznie w prowadzi zero? jak się tego po zbędziesz będziesz miał universalną metodę czytania intów. Jesli myslałeś o tym że zera tam przecież być nie moze i dlatego tak, to lepiej by było jak byś to całe pobieranie robił w jednej metodzie i ew. wydzielił pod metody gdy bedą Ci kiedyś potrzebne
1
Witam,
Pomyśl trochę nad czytelnością i spójnością kodu, np nazwy funkcji powinny być z wielkiej litery. Zastanów się nad maksymalnym uproszczaniem kodu funkcji
private bool CheckChoice(int choice)
{
return choice > 0 && choice < 8;
}
albo lepiej zamień to na enum z parsowaniem danych wejściowych.
Pozdrawiam,
mr-owl
0
Poprawiłem mój kod na tyle ile umiałem o to klasa User. Czy przynajmniej kod tej jednej klasy wygląda już lepiej ? Mam jeszcze jedno pytanie, gdyż w jednym komentarzu zostało mi napisane, aby wyrzucić instrukcję switch czy jest ona niepożądana? Dlaczego ?
class User
{
public enum TheMenuValues
{
addition = 1, // first
substraction,
multiply,
division,
root,
intensification,
exit // last
}
public enum AmountDowlandedData
{
one = 1,
two,
ten = 10
}
public string view { private get; set; }
public int GetOptionMenuList()
{
int choice;
do
{
choice = GetValue(view);
} while (!checkChoice(choice));
return choice;
}
private bool checkChoice(int choice)
{
return choice >= (int)TheMenuValues.addition && choice <= (int)TheMenuValues.exit;
}
private int GetValue(string inscription)
{
bool value;
int choice;
do
{
Console.Write(inscription);
string tmp = Console.ReadLine();
value = int.TryParse(tmp, out choice);
} while (!value);
return choice;
}
public List<double> GetNumbers(AmountDowlandedData value)
{
List<double> lista = new List<double>();
do
{
double tmp = GetValue(view);
lista.Add(tmp);
if ((int)value == lista.Count())
{
Console.Clear();
return lista;
}
} while (true);
}
}