-
Notifications
You must be signed in to change notification settings - Fork 24
Hamraev_Ivan_Dilshodovich #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: Hamraev_Ivan_Dilshodovich
Are you sure you want to change the base?
Hamraev_Ivan_Dilshodovich #11
Conversation
CourseApp/Class/Hydroplane.cs
Outdated
| private string model; | ||
| private int maxSpeed; | ||
|
|
||
| public Hydroplane(int power, int length, int weight, int landingSpeed, int takeoffSpeed, int lengthFloat, int numberWings, string model, int maxSpeed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А вам точно нужно такое число параметров при инициализации?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Для работы методов не обязательно, могу уменьшить, не вопрос
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
можно завести еще конструктор с меньшим числом параметров
CourseApp/Class/Hydroplane.cs
Outdated
| this.model = model; | ||
| this.maxSpeed = maxSpeed; | ||
| direction = string.Empty; | ||
| speed = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
это не обязательно - оно и так будет выставлено в значение по умолчанию
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Понял, спасибо
CourseApp/Class/Hydroplane.cs
Outdated
| } | ||
|
|
||
| public override void GetInfo() | ||
| => Console.WriteLine($"Model: {model} \nPower: {power}horsepower \nLength: {length}m \nWeight: {weight}kg \nLanding speed: {landingSpeed}km/h \nTakeoff speed: {takeoffSpeed}km/h \nMax speed: {maxSpeed}km/h"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не очень хорошая практика использовать \n - поскольку это сильно зависит от ОС
Посмотрите на многострочные строки в c# (начинаются с @)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, хотел их изначально использовать, но потом в сторону \n склонился, всё поправлю
CourseApp/Class/Hydroplane.cs
Outdated
| public override void GetInfo() | ||
| => Console.WriteLine($"Model: {model} \nPower: {power}horsepower \nLength: {length}m \nWeight: {weight}kg \nLanding speed: {landingSpeed}km/h \nTakeoff speed: {takeoffSpeed}km/h \nMax speed: {maxSpeed}km/h"); | ||
|
|
||
| public override void Movement() => Console.WriteLine($"Самолёт движется со скоростью {speed} км/ч, в направлении: {direction}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Протестировать метод - который просто выводит в консоль будет сложно - проще - возвращать значение (пусть даже и строковое)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Сделаем
CourseApp/Class/Hydroplane.cs
Outdated
| public override void TakeSpeed() => Console.WriteLine($"Speed right now: {speed}km/h"); | ||
|
|
||
| public int GetSpeed() => speed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это лучше реализовать через свойства в C#
CourseApp/Class/Hydroplane.cs
Outdated
| Console.Write("Введите направление, курс полёта: "); | ||
| direction = Console.ReadLine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вот так не надо делать - просто передайте параметры извне
SetDirection(int direction))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Решил тоже свойством оформить и поколдовать с Movement)
|
а куда делся каталог с .github ? почему actions не запустились? |
CourseApp/Class/Hydroplane.cs
Outdated
| this.landingSpeed = landingSpeed; | ||
| this.takeoffSpeed = takeoffSpeed; | ||
| this.lengthFloat = lengthFloat; | ||
| this.numberWings = numberWings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
вот это можно реализовать через вызов родительского конструктора
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Но я не могу получить доступ к переменным родительского конструктора, так как он у меня абстрактный, и переменные соответственно приватные, голову сломал ещё когда писал, как их передавать, в итоге решил в дочерний класс всё запихнуть, а в абстрактном только методы
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
стоп - конструктор то у вас есть у родительского с такими параметрами?
CourseApp/Class/Hydroplane.cs
Outdated
| this.power = power; | ||
| this.length = length; | ||
| this.weight = weight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не характерны ли эти свойства для всех самолетов в целом? эти поля стоит унести в базовый класс
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В этом классе стоит оставить только те поля - которые характерны только для гидросамолетов
| hydroplane.SpeedUp(); | ||
| var result = hydroplane.SpeedUp(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а зачем 2 раза,
- вы можете в рамках первого теста проверить
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тесты необходимо добавить для всех методов - ну как минимум для getInfo точно
| hydroplane.SpeedUp(); | ||
| hydroplane.Braking(); | ||
|
|
||
| Assert.Equal(0, hydroplane.GetSpeed()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
необходимы тесты на конструктор
CourseApp/Class/Plane.cs
Outdated
| direction = "some direction"; | ||
| } | ||
|
|
||
| public string Exc { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а это что означает?
CourseApp/Class/Plane.cs
Outdated
| catch (Exception e) | ||
| { | ||
| Exc = $"Error: {e.Message}"; | ||
| Console.WriteLine(Exc); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не - если уж перешли на эксепшены - выбрасывайте просто дальше - иначе тут ваш выброшенный перехватывается и все - а вы ж как раз хотите не дать пользователю установить значение скорости
CourseApp/Class/Plane.cs
Outdated
| public string Direction | ||
| { | ||
| get | ||
| { | ||
| return direction; | ||
| } | ||
|
|
||
| set | ||
| { | ||
| direction = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
если нет внутри логики - то просто {get; set;}
| /// <summary> | ||
| /// Types of actions. | ||
| /// </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зачем ?
CourseApp/Game.cs
Outdated
| Console.Write("Введите кол-во героев: "); | ||
| int numberOfPlayers = Convert.ToInt16(Console.ReadLine()); | ||
| Console.WriteLine(); | ||
| if (numberOfPlayers % 2.0 != 0) | ||
| { | ||
| Console.WriteLine("Количество героев нечетное!"); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ввод и непосредственную работу следует разделить
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Сделаем
CourseApp/Game.cs
Outdated
| for (int i = 0; i < numberOfPlayers; i++) | ||
| { | ||
| var randomName = _names[_random.Next(0, 20)]; | ||
| switch (CreatePlayers.GetHeroType()) | ||
| { | ||
| case PlayerTypes.NotType: | ||
| new Exception("Player has not type"); | ||
| break; | ||
| case PlayerTypes.Knight: | ||
| players.Add(new Knight(randomName)); | ||
| break; | ||
| case PlayerTypes.Archer: | ||
| players.Add(new Archer(randomName)); | ||
| break; | ||
| case PlayerTypes.Wizard: | ||
| players.Add(new Wizard(randomName)); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
можно это в фабричный метод вынести?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно конечно
| } | ||
| else | ||
| { | ||
| battle[0].GetDamage(battle[1].GiveDamage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а если у меня иммунитет?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вы получаете 0 урона, там дальше в логике это прописано, однако в логере всё же будет сообщение, что вам нанесли урон, но по факту он будет отражён иммунитетом
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это в Wizard идёт проверка при получении урона. Соответственно Invisible становится True, когда используется ульт
| return $"(Лучник) {Name}"; | ||
| } | ||
|
|
||
| public override void Ultimate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
какой паттерн тут бы подошел?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Фабрика?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не очень понимаю на какие конкретно изменения вы намекаете ...

No description provided.