Local network graph is ready #8
Conversation
…bou the process of infection
| module LocalNetwork | ||
|
|
||
| open System | ||
| open System.Collections.Generic |
There was a problem hiding this comment.
System.Collections.Generic в F# нетипичен, обычно встроенных структур данных достаточно (только они все немутабельные, так что System.Collections.Generic может быть полезен в плане производительности на некоторых задачах)
| let numberOfComputers () = List.length computersCommunication | ||
|
|
||
| /// Random numbers sensors for computers. On every step check if the computer is infected. | ||
| let rndSensorValues = new List<Random>() |
There was a problem hiding this comment.
Мм, а зачем Вам целый список генераторов случайных чисел? Обычно одного на всю программу вполне достаточно, но целый список генераторов --- это очень загадочно. Это же просто генератор случайных чисел, он просто возвращает следующее случайное число. Если сделать их два и у каждого вызвать Next, Вы получите два случайных числа --- точно так же, как если сделать один и у него дважды вызвать Next.
| initRec (acc + 1) | ||
| initRec 0 | ||
|
|
||
| do init |> ignore |
There was a problem hiding this comment.
А зачем тогда init что-то возвращает?
|
|
||
| /// Checks if the current step infects computer. | ||
| let isInfectedThisStep vertex = | ||
| (probabilityOfInfection vertex).CompareTo(double(rndSensorValues.[vertex].Next(0, 100))) >= 0 |
There was a problem hiding this comment.
Тут бы и просто оператор сравнения подошёл, кажется
| |> List.iter (fun x -> if not (infected.Contains x) && isInfectedThisStep x then newInfected.Add x) | ||
|
|
||
| /// Infects first computer. | ||
| let infectFirst = |
There was a problem hiding this comment.
Мм, это не функция, а значение, вычисляющееся в момент создания объекта и далее не меняющееся. А называется как функция и используется, кажется, как функция
| elif (infected.Count = numberOfComputers() || infected.TrueForAll(fun x -> not <| neighboursCanBeInfected x)) then infected | ||
| else | ||
| infected.ForEach(fun x -> tryInfectNeighbours x) | ||
| infected.AddRange(newInfected); |
There was a problem hiding this comment.
Точка с запятой не нужна. Ну и правда, это какой-то уж очень императивный код, куча мутабельного состояния и неочевидные зависимости по данным. Я бы newInfected просто возвращал и не делал полем.
| elif (infected.Count = numberOfComputers () || infected.TrueForAll(fun x -> not <| neighboursCanBeInfected x)) then infected | ||
| else | ||
| infected.ForEach(fun x -> tryInfectNeighbours x) | ||
| infected.AddRange(newInfected) |
There was a problem hiding this comment.
Тут концептуально ничего не поменялось, это всё так же жутковато даже для кода на C#, а уж на F# так писать вообще нельзя без веских причин :)
| let mutable infected = new List<int>() | ||
|
|
||
| /// Infected this step. | ||
| let mutable newInfected = new List<int>() |
There was a problem hiding this comment.
Они mutable, но в них буквально нет ни одного присванивания. Зачем они mutable? Но я выше намекал, что их лучше вообще заменить на 'a list. Не настаиваю, но код не должен вызывать ощущения "что происходит?". Пока вызывает.
| let mutable infected = [] | ||
|
|
||
| /// Infected this step. | ||
| let mutable newInfected = [] |
There was a problem hiding this comment.
Так нет, функциональный стиль --- это не когда у нас поле немутабельного типа, котрое по сути представляет собой глобальную переменную, его кто-то где-то внутри меняет, потом его надо не забыть сбросить в пустой список и т.д., функциональный стиль --- это когда у нас практически нет состояния и мы всё, что надо, просто возвращаем из функций или передаём как параметр, так что поток данных в программе в точности соответствует потоку исполнения. Тут можно было сделать так, чтобы tryInfectNeighbours выдавал список заражённых, а вместо List.iter(fun x -> tryInfectNeighbours x) infected (что само по себе не очень, правильно List.iter tryInfectNeighbours infected) сделать List.map и List.concat
There was a problem hiding this comment.
И мне кажется, как раз забыли сбросить newInfected :)
| let tryInfectNeighbours vertex = | ||
| let tryInfectNeighboursRec vertex = | ||
| computersCommunication.[vertex] | ||
| |> List.iter (fun x -> if not (List.contains x infected) && isInfectedThisStep x then newInfected <- newInfected @ [x]) |
There was a problem hiding this comment.
newInfected @ [x] или x :: newInfected?
| type LocalNetwork(computersCommunication: int list list, OSOfComputers: string list, probabilityInfectionForOS: float list) = | ||
|
|
||
| /// Number of computers in local network. | ||
| let numberOfComputers () = List.length computersCommunication |
There was a problem hiding this comment.
Это можно было сделать полем, ведь computersCommunication немутабельный, его длину можно сосчитать один раз и она не сможет измениться
| infectAllRec [] | ||
|
|
||
| /// Infects all the computers with virus. Returns infected computers. | ||
| member public this.Infected = infectAll |
There was a problem hiding this comment.
Тогда оно должно называться как что-то вроде "Infect"
| if List.length infected = 0 then | ||
| printInf infected | ||
| infectAllRec (infectFirst () :: infected) | ||
| elif (List.length infected = numberOfComputers () || List.forall (fun x -> not (neighboursCanBeInfected x infected)) infected) then infected |
There was a problem hiding this comment.
Я бы сказал, что граф сети может быть несвязным или разбитым на изолированные области "незаражаемыми" компьютерами, а это условие тут вроде как не проверяется, так что зависнуть мы таки можем. Но ладно :)
| /// Tries to infect all the vertexes that are connected with infected. | ||
| let tryInfectNeighboursOfInfected infected = | ||
| let rec tryInfectNeighboursOfInfectedRec infected leftVrtxs = | ||
| match leftVrtxs with |
There was a problem hiding this comment.
Пхнглуи вагх`нагл фхагн. Ну и имена у Вас в программе :)
No description provided.