Skip to content

Create constraint system#56

Open
MrsSima wants to merge 2 commits intoyurii-litvinov:masterfrom
MrsSima:darya-merge2
Open

Create constraint system#56
MrsSima wants to merge 2 commits intoyurii-litvinov:masterfrom
MrsSima:darya-merge2

Conversation

@MrsSima
Copy link
Contributor

@MrsSima MrsSima commented Apr 8, 2018

Created view and check methods for amount and
attribute values constraints change.

/// </summary>
public class ConstraintItem
{
public int Flag { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Флаг чего? :) И почему он int?

{
public int Flag { get; set; }

public (string, string) Elements { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тоже, тип возвращаемого значения ничего не говорит о его содержании. Может, лучше структуру возвращать?


set
{
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это точно должно быть свойство, а не метод?


if (this.Flag == 1)
{
var value = (ValueTuple<int, int>)this.Value;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут лучше наследование, а не такое нетипизированное решение. Особенно с ненужным boxing-ом

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наследование я делала, но в таком случае возникают другие проблемы с переопределением свойства, помучавшись с этим вопросом, я в итоге вернулась к этому варианту. Вполне вероятно, я делала кривое наследование. Спрошу об этом завтра.

return "Type: " + this.ObjectType + Environment.NewLine
+ "Element: " + this.Elements.Item1 + Environment.NewLine
+ "Attribute: " + this.AttributeName + Environment.NewLine
+ "Value: " + this.Value.ToString();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String interpolation не хотите тут попользовать?


this.valueLabel.Text = "RegExp:";
this.stringBox.Visibility = Visibility.Visible;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это тоже обычно binging-ами делается


if ((valueFrom < 0) || (valueTo < 0))
{
throw new Exception("The number can not be less than zero.");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Просто исключения бросать нельзя, надо бросать исключение какого-то конкретного класса

return true;
}
catch
catch (Exception ex)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А ловить Exception нельзя и подавно. От него же вообще все исключения в дотнете наследуются, Вы так поймаете даже ThreadAbortException, FileLoadException и прочие штуки, которые ловить было бы неправильно

{
this.nodesList = value;
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Может, это сделать автоматическим свойством, раз тут всё равно ничего содержательного не делается?

{
NewValue = value
};
this.OnAttributeChange?.Invoke(this, args);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут тоже с отступом что-то не так

@MrsSima MrsSima force-pushed the darya-merge2 branch 2 times, most recently from 3b4d20a to 54f3460 Compare April 10, 2018 07:56
Created view and check methods for amount and
attribute values constraints change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants