Discussion :

[kheironn]

Bonjour, je ne suis pas un pro du vb.net ni du linq et je trouve que mon code est assez pourri et me parait assez peu performant. Si un expert pouvait m'aider à l'améliorer cela me rendrait un grand service. Le bon côté, c'est que mon code fonctionne parfaitement et obéit à toutes mes règles de gestion.
voici l'extrait qui me gène :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
Partial Public Class CheckForm
    Inherits IdentificationPage
 
    Dim reponse As String = String.Empty
    Dim currantGoals As DataTable = Nothing
 
    Private Sub YieldMinErrors(ByVal listSaisieObj As List(Of SaisieObj), ByVal collaborators As List(Of CollaboratorLight))
        'Le contrôle se fait en 3 étapes : sur la saisie, sur le nombre d'agences puis sur les données déjà enregistrées.
        Dim listErrorMin = listSaisieObj.Where(Function(p) p.Valeur < p.Min And p.Valeur >= 0 And p.Codeassoc = String.Empty)
        For Each goalInError In listErrorMin
            'On a trouvé des objectifs saisis où la valeur ne correspond pas au minimum attendu donc on va contrôler que cette valeur est bonne en tenant compte du nombre d'agences de chaque collaborateur. Chaque collaborateur en erreur lèvera un dernier contrôle.
            Dim collaboratorsInError As String = String.Empty
            For Each collaborator In collaborators
                Dim nbAgencies = If(collaborator.AgenciesCode Is Nothing, 0, collaborator.AgenciesCode.Count)
                Dim difference = goalInError.Min - (goalInError.Valeur * nbAgencies)
                If (difference > 0) Then
                    'On a des collaborateurs pour lesquels le nombre d'agences ne compense pas l'écart entre la valeur saisie et le minimum. On va devoir contrôler ce qui a déjà été saisi.
                    Dim total As Decimal = 0
                    total = GetMinTotalByCollaboratorAndGoal(collaborator, goalInError.Id.ToString())
                    If (difference - total > 0) Then
                        collaboratorsInError = String.Concat(collaboratorsInError, "<dd>", common.VbSysMethodes.GetNomComplet(collaborator.IdentifiantSI), "</dd>")
                    End If
                End If
            Next
 
            If Not String.IsNullOrEmpty(collaboratorsInError) Then
                Me.reponse = String.Concat(Me.reponse, String.Format("<p>L'objectif '{0}' doit être supérieur ou égal à {1} sur l'ensemble des agences pour :{2}</p>", goalInError.Libelle, goalInError.Min, collaboratorsInError))
            End If
        Next
    End Sub
 
    Private Function GetMinTotalByCollaboratorAndGoal(ByVal coll As CollaboratorLight, ByVal goalId As String) As Decimal
        Dim records = (From rec In currantGoals Where rec("IDENTIFIANTSI") = coll.IdentifiantSI And rec("CODEOBJECTIF") = goalId Select rec).ToList()
        Dim total As Decimal
        Dim minimalTotal As Decimal = Decimal.MaxValue
 
        ' on doit exclure les enregistrements à remplacer, c'est à dire ceux des agences selectionnées puis faire un total PAR semaine.
        If (records.Count > 0) Then
            For Each agency In coll.AgenciesCode
                Dim soc = coll.CodeSociety
                Dim ag = agency
                Dim rowsToExclude = records.Where(Function(row) row("codesociete") = soc And row("codeservice") = ag).ToList()
                For Each row In rowsToExclude
                    records.Remove(row)
                Next
            Next
 
            Dim periods = (From p In records Select p("ANNEE") * 100 + p("SEMAINE")).Distinct().ToList()
            For Each period In periods
                total = 0
                Dim year As Integer = Decimal.Floor(period / 100)
                Dim week As Integer = period - year * 100
                Dim rows = (From r In records Where r("ANNEE") = year And r("SEMAINE") = week Select r).ToList()
                For Each row In rows
                    total += row("VALEUR")
                Next
                minimalTotal = If(total < minimalTotal, total, minimalTotal)
            Next
        End If
 
        Return If(minimalTotal = Decimal.MaxValue, 0, minimalTotal)
    End Function

bien évidement il y a pleins d'autre méthodes qui appellent ce code.

Merci d'avance

[tomlev]

Voilà ce que j'ai noté, dans l'ensemble je vois pas de problèmes très graves

• Implémenter des règles métier dans le code d'une page, c'est pas terrible... Il vaudrait mieux séparer le domaine métier du code de présentation.
• listSaisieObj pourrait être déclaré de type IEnumerable(Of SaisieObj), puisque tu n'utilises aucun membre spécifique à List(Of SaisieObj). D'une manière générale il vaut mieux déclarer le type le moins spécifique possible, pour limiter les contraintes sur le code qui l'utilise
• idem pour collaborators
• p.Codeassoc = String.Empty est correct mais ne gère pas le cas où ça vaut Nothing ; il vaut mieux utiliser String.IsNullOrEmpty(p.Codeassoc)
• les concaténations de chaines dans une boucle sont généralement à éviter pour des raisons de performances. Utilise plutôt un StringBuilder pour collaboratorsInError
• Tu peux simplifier ce bout de code :
  

  Code :

  Sélectionner tout - Visualiser dans une fenêtre à part

  1
  2
  3
  4
                  Dim rowsToExclude = records.Where(Function(row) row("codesociete") = soc And row("codeservice") = ag).ToList()
                  For Each row In rowsToExclude
                      records.Remove(row)
                  Next

  En ceci :
  
  

  Code :

  Sélectionner tout - Visualiser dans une fenêtre à part

      records.RemoveAll(Function(row) row("codesociete") = soc And row("codeservice") = ag)

A la rigueur, pour réduire un peu la complexité, tu pourrais essayer de subdiviser ces méthodes en méthodes plus simples.

Pour les perfs, je vois pas trop ce que tu pourrais améliorer. Quand tu dis que ça te "parait" assez peu performant, tu as observé que c'était lent, ou tu supposes que c'est lent en voyant le code ?

[kheironn]

Merci pour ces réponses.
Il s'agit d'un code que je reprends d'un autre développeur et certains détails comme p.Codeassoc = String.Empty m'ont échappés lors de mon refactoring. (c'est la cas pour les 5 1ers points de ta réponse )

Pour les perfs, je supposais une lenteur du fait de l'enchainement de boucles, mais j'ai fait des tests entre temps et dans les cas d'utilisation habituels, c'est bon.

Le problème, par rapport à mes habitudes en C#, c'est que je trouve ce code un peu lourd / usine à gaz. Je trouvais qu'il y avait beaucoup de boucles ^pour filtrer mes lignes de données à utiliser au final et je me demandais si je ne pouvais pas via linq faire plus clair.

Tu as surement raison, en redécoupant ce code cela devrait l'éclaircir d'autant plus qu'il y a 2 autres vérifications qui vont se baser sur le même principe, donc factorisable. Pour le moment, je cherchais plutôt à faire un truc qui marche pour ma méthode GetMinTotalByCollaboratorAndGoal et améliorer ma YieldMinErrors

Pour le découpage, je dirais que je suis sur une archi qui m'étonne. L'application utilise du javascript qui appelle des webservices codés sous forme de page aspx. je ne connaissais pas cette façon de faire les ws. Mais effectivement, je suis bien dans le projet "g.u.i." de l'application. et je me disais aussi que ce code devrait être dans la BL.

[kheironn]

Il s'agit d'un service web de contrôle de valeur avant de lancer un enregistrement. Ce service retourne les erreurs trouvées et bloque l'enregistrement.

[Pol63]

pour la lisibilité il est aussi utile de diminuer le nombre d'imbrications

dans GetMinTotalByCollaboratorAndGoal à la place de

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
if records > 0 then
  'gros traitement
end if
 
return qqch

tu peux faire

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
if records <= 0 then return qqch
 
'gros traitement
 
return qqch

[kheironn]

En cours de (re)factorisation j'en arrive à ça :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
#Region "Check minimal"
        Private Sub YieldMinErrors(ByVal listSaisieObj As IEnumerable(Of SaisieMasse.SaisieObj), ByVal collaborators As List(Of CollaboratorLight))
        Dim listErrorMin = listSaisieObj.Where(Function(p) p.Valeur < p.Min And p.Valeur >= 0 And String.IsNullOrEmpty(p.Codeassoc))
        For Each goalInError In listErrorMin
            'On a trouvé des objectifs saisis où la valeur ne correspond pas au minimum attendu donc on va contrôler que cette valeur est bonne en tenant compte du nombre d'agences de chaque collaborateur. Chaque collaborateur en erreur lèvera un dernier contrôle.
 
            Dim collaboratorsInError As New StringBuilder()
            For Each collaborator In collaborators
                Dim nbAgencies = If(collaborator.AgenciesCode Is Nothing, 0, collaborator.AgenciesCode.Count)
                Dim difference = goalInError.Min - (goalInError.Valeur * nbAgencies)
                If (difference > 0) Then
                    'On a des collaborateurs pour lesquels le nombre d'agences ne compense pas l'écart entre la valeur saisie et le minimum. On va devoir contrôler ce qui a déjà été saisi.
 
                    Dim total As Decimal = 0
                    total = GetMinTotalByCollaboratorAndGoal(collaborator, goalInError.Id.ToString())
                    If (difference - total > 0) Then
                        collaboratorsInError.Append(String.Concat("<dd>", common.VbSysMethodes.GetNomComplet(collaborator.IdentifiantSI), "</dd>"))
                    End If
                End If
            Next
 
            If Not String.IsNullOrEmpty(collaboratorsInError.ToString()) Then
                Me.reponse = String.Concat(Me.reponse, String.Format("<p>L'objectif '{0}' doit être supérieur ou égal à {1} sur l'ensemble des agences pour :{2}</p>", goalInError.Libelle, goalInError.Min, collaboratorsInError))
            End If
        Next
    End Sub
 
    Private Function GetMinTotalByCollaboratorAndGoal(ByVal coll As CollaboratorLight, ByVal goalId As String) As Decimal
        ' on doit exclure les enregistrements à remplacer, c'est à dire ceux des agences selectionnées puis faire un total PAR semaine.
        Dim minimalTotal As Decimal = Decimal.MaxValue
        Dim records = (From rec In currantGoals Where rec("IDENTIFIANTSI") = coll.IdentifiantSI And rec("CODEOBJECTIF") = goalId Select rec).ToList()
 
        If (records.Count > 0) Then
            records = Me.FilterRecordsByServices(records, coll)
 
            If (records.Count > 0) Then
                Dim periods = (From p In records Select p("ANNEE") * 100 + p("SEMAINE")).Distinct().ToList()
                For Each period In periods
                    Dim total As Decimal = Me.GetTotalFromRecordsByPeriod(records, period)
                    minimalTotal = If(total < minimalTotal, total, minimalTotal)
                Next
            End If
        End If
 
        Return If(minimalTotal = Decimal.MaxValue, 0, minimalTotal)
    End Function
#End Region
 
    Private Function FilterRecordsByServices(ByRef records As List(Of DataRow), ByVal coll As CollaboratorLight) As List(Of DataRow)
        For Each agency In coll.AgenciesCode
            Dim soc = coll.CodeSociety
            Dim ag = agency
            records.RemoveAll(Function(row) row("codesociete") = soc And row("codeservice") = ag)
        Next
        Return records
    End Function
 
    Private Function GetTotalFromRecordsByPeriod(ByVal records As IEnumerable(Of DataRow), ByVal period As Integer) As Decimal
        Dim total As Decimal = 0
        Dim year As Integer = Decimal.Floor(period / 100)
        Dim week As Integer = period - year * 100
        Dim rows = (From r In records Where r("ANNEE") = year And r("SEMAINE") = week Select r).ToList()
        For Each row In rows
            total += row("VALEUR")
        Next
        Return total
    End Function

Maintenant il me faut revoir YieldMinErrors, et voir pour les noms qui ne sont pas forcément bien choisis.