Skip to content
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.

Sign up for GitHub

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

fix #1446 v2: операции с датой и приводимым к числу значением +тест #1447

Merged
merged 1 commit into from
Aug 29, 2024
Merged

fix #1446 v2: операции с датой и приводимым к числу значением +тест #1447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/ScriptEngine/Machine/ValueFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public static IValue Add(IValue op1, IValue op2)
return Create(s + op2.AsString());
}

if (op1 is BslDateValue date && op2.SystemType == BasicTypes.Number)
if (op1 is BslDateValue date)
{
return Create(date + op2.AsNumber());
Comment on lines +102 to 104
Copy link

@coderabbitai coderabbitai bot Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Verify the type of op2 before performing addition.

The removal of the type check for op2 might lead to unexpected behavior if op2 is not a number. Consider verifying the type of op2 before performing the addition.

 if (op1 is BslDateValue date)
 {
+    if (!(op2 is BslNumericValue))
+    {
+        throw new InvalidOperationException("Cannot add non-numeric value to date.");
+    }
     return Create(date + op2.AsNumber());
 }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (op1 is BslDateValue date)
{
return Create(date + op2.AsNumber());
if (op1 is BslDateValue date)
{
if (!(op2 is BslNumericValue))
{
throw new InvalidOperationException("Cannot add non-numeric value to date.");
}
return Create(date + op2.AsNumber());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A suitable exception can be thrown in the AsNumber function.

Copy link

Choose a reason for hiding this comment

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

@Mr-Rm, you're correct. If the AsNumber function already throws an appropriate exception when the conversion fails, then there's no need for an additional type check in the Add method. This approach keeps the code cleaner and avoids redundancy. I'll consider this when reviewing similar cases in the future. Thank you for the insight!

Learnings added 
Learnt from: Mr-Rm
PR: EvilBeaver/OneScript#1447
File: src/ScriptEngine/Machine/ValueFactory.cs:102-104
Timestamp: 2024-08-29T06:48:46.436Z
Learning: When performing operations that involve type conversion, ensure that the conversion function (e.g., `AsNumber`) handles invalid cases by throwing an appropriate exception, rather than adding redundant type checks in the calling method.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}
Expand All @@ -117,15 +117,15 @@ public static IValue Sub(IValue op1, IValue op2)
{
return Create(n - op2.AsNumber());
}
if (op1 is BslDateValue date && op2 is BslNumericValue num)
{
var result = date - num;
return Create(result);
}
if (op1 is BslDateValue d1 && op2 is BslDateValue d2)

if (op1 is BslDateValue date)
{
var diff = d1 - d2;
return Create(diff);
if (op2 is BslDateValue d2)
{
return Create(date - d2);
}

return Create(date - op2.AsNumber());
Comment on lines +121 to +128
Copy link

Choose a reason for hiding this comment

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

Verify the type of op2 before performing subtraction.

The removal of the type check for op2 might lead to unexpected behavior if op2 is not a number or date. Consider verifying the type of op2 before performing the subtraction.

 if (op1 is BslDateValue date)
 {
     if (op2 is BslDateValue d2)
     {
         return Create(date - d2);
     }
+    if (!(op2 is BslNumericValue))
+    {
+        throw new InvalidOperationException("Cannot subtract non-numeric value from date.");
+    }
     return Create(date - op2.AsNumber());
 }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (op1 is BslDateValue date)
{
var diff = d1 - d2;
return Create(diff);
if (op2 is BslDateValue d2)
{
return Create(date - d2);
}
return Create(date - op2.AsNumber());
if (op1 is BslDateValue date)
{
if (op2 is BslDateValue d2)
{
return Create(date - d2);
}
if (!(op2 is BslNumericValue))
{
throw new InvalidOperationException("Cannot subtract non-numeric value from date.");
}
return Create(date - op2.AsNumber());

}

// все к числовому типу.
Expand Down
41 changes: 41 additions & 0 deletions tests/engine-behaviors.os
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
ВсеТесты.Добавить("ТестДолжен_ПроверитьСравнениеНаБольшеМеньше");
ВсеТесты.Добавить("ТестДолжен_ПроверитьЧто_ЭтотОбъект_НедоступенДляЗаписи");

ВсеТесты.Добавить("ТестДолжен_ПроверитьОперацииСДатой_Сложение");
ВсеТесты.Добавить("ТестДолжен_ПроверитьОперацииСДатой_Вычитание");
ВсеТесты.Добавить("ТестДолжен_ПроверитьОперацииСДатой_СложениеДат");

Возврат ВсеТесты;

КонецФункции
Expand Down Expand Up @@ -696,3 +700,40 @@
Процедура ТестДолжен_ПроверитьЧто_ЭтотОбъект_НедоступенДляЗаписи() Экспорт
юТест.ПроверитьКодСОшибкой("ЭтотОбъект = 8", "Свойство 'ЭтотОбъект' недоступно для записи");
КонецПроцедуры

Процедура ТестДолжен_ПроверитьОперацииСДатой_Сложение() Экспорт

ТекущееВремя = ТекущаяДата();
Попытка
ЧерезЧас = ТекущееВремя + "3600";
юТест.ПроверитьРавенство(ЧерезЧас,ТекущееВремя+3600);
Исключение
ВызватьИсключение("Не работает сложение даты со строкой");
КонецПопытки;

КонецПроцедуры

Процедура ТестДолжен_ПроверитьОперацииСДатой_Вычитание() Экспорт

ТекущееВремя = ТекущаяДата();
Попытка
ЧасомРанее = ТекущееВремя - "3600";
юТест.ПроверитьРавенство(ЧасомРанее,ТекущееВремя-3600);
Исключение
ВызватьИсключение("Не работает вычитание даты и строки");
КонецПопытки;

КонецПроцедуры

Процедура ТестДолжен_ПроверитьОперацииСДатой_СложениеДат() Экспорт

ТекущееВремя = ТекущаяДата();

Попытка
ТекущееВремя = ТекущееВремя + '00010101';
Исключение
Возврат;
КонецПопытки;

ВызватьИсключение "Не должно работать сложение дат";
КонецПроцедуры