README quelques fautes Logins et pass en début : très bien note sur les connexions multiples : très bien. En effet, il est concevable que l'on puisse se connecter plusieurs fois (pas forcement plusieurs tablettes, mais par exemple un ordi+tablette). Testé jusqu'à 5 connexions "Généralement, il est préférable par souci d'optimisation d'effectuer le moins de connexions à la base de données que possible, et donc de limiter les connexions à deux situations : une unique connexion pour la récupération des données depuis la base de données au lancement du serveur, et des connexions dans le seul cas où le serveur devrait modifier/ajouter une valeur de la base de données." Et si la base fait plusieurs centaines de Go de données, on charge tout en mémoire ? Mots clés non expliqués : opcode, hook Port 45354 et IP : modifiable ? lignes 111-122 : envoi de la liste de tous les patients ? traitement javascript ? Problème en cas de gros volume ligne 129 : la traitement de la liste des patients ne fait pas parti de la logique plutôt que l'interface ? mise en cache des images : utilité ? purge du cache ? Choix de la résolution ? lignes 172-175 : plusieurs connections au même patient = plusieurs ECG différent... Le patient à plusieurs coeurs ? Fonctionnalités préchargement des données ? Garder la main durant le chargement ? Compilation win: aucun warning ni bugs pour serveur+client linux : quelques warnings Exécution serveur: comment on quitte ? c'est quitté proprement si on ferme simplement la fenètre ? en mode débug, quitter de cette façon provoque l'apparition d'un message d'erreur "Le processus de l'application ne peut être arrêté : The program is not being run." client: démarrage sans serveur : plusieurs tentatives. Message d'avertissement à partir de la 3ème tentative "Le serveur est-il bien lancé ?". Pas de possibilité de stop les tentatives de connexion. Warning d'avertissement sur la console "QAbstractSocket::ConnectionRefusedError" Qt 4.8: erreur du débogueur qml "file:///C:/.../qml/Delegates/HomeSlideDelegate.qml:30: Unable to assign [undefined] to double angle" à 4 reprise. en mode débug et release. Idem avec 4.7.4 Serveur Génératlités inclusion des modules complets (, , etc.) au lieu d'inclure uniquement la classe problème de granularité et de respect de principes de POO définir clairement les invariants de classe et les pré et post conditions pas de tests unitaires pas de gestion à distance du serveur fichier qrc : toutes les images = binaire créé très lourd (> 4 Mo) fichier pro : pas d'organisation des fichiers par "thème" (network, database, module, etc.) Granularité ? les fichiers sources dans un répertoire "include" main.cpp logfile : fonction libre et singleton pas de choix de la sortie de debug (on aurait peut aimer voir les logs directement sur un client) plus généralement, gestion du serveur par un module admin sur client ? objet master détruit quand et comment ? -> pas de possibilité de quitter proprement le serveur tous est fait dans le master = non respect du SRP master utilité de master ? tous aurait pu être fait dans le main.cpp db et simulator ? macro pas forcement visible au premier coup d'oeil. Destruction ? #include inutile tcpserveur pourquoi le include ? clientusername et password en dur ? toutes les messages sont parsé par une fonction du tcpserveur : non respect du OCP. Un design pattern chaine de commande aurait été préférable opcodes[] : tous les messages en dur. Utilisation de pointeurs de fonction bof. référence circulaire opcode<->tcpserveur inutile Non respect du SRP : gérer les connexions, dispatcher les messages, traiter les messages, gérer les clients, gérer le json... ce qui explique pourquoi le fichier .cpp fait > 1200 lignes initialize() : faire le connect avant le listen() ? fillStrings() : pourquoi fonction variadic handleLogout() : ligne 149 customer = NULL; sert à rien json transmit par réseau : plus lourd que d'envoyer des données binaires formatées preparePatient() : toutes les données envoyée : et si le dossier est lourd ? et si le patient est connu depuis 20 ans ? unhandled() : utilité d'une fonction qui ne fait rien ? répétition importante du code, a factoriser QByteArray packet; QDataStream out(&packet, QIODevice::WriteOnly); out << (quint64)0; out << int(MSG_PONG); out.device()->seek(0); out << (quint64)(packet.size() - sizeof(quint64)); customer->send(packet); Ne permet que la consultation/modification, pas la création ? non respect du ISP (ségrégation des interfaces) : TcpServer hérite de QTcpServer mais l'interface de QTcpServer n'est pas (ou peu) utilisée -> utiliser la composition plutôt que l'héritage opcodes liste en dur, pointeur de fonction simulator pourquoi include ? singleton inutile : pourquoi la créé dans master.cpp alors que utilisé uniquement dans tcpserveur ? ligne 50 : explicit inutile (private + pas de paramètre) destructeur privé : comment les données sont détruites ? ok, par freeInstance() cohérence : utilisation de O et de NULL progress : un par client plutôt que par patient = non synchro sur un même patient gestion des clients connecté au simulateur = nouvelle responsabilité sendData() : creation des données à envoyer = nouvelle responsabilité recréé une nouvelle liste à chaque update = lourd customer pas de test systématique de _socket database structure codée en dur chargement de toutes les données structure c++ en image de la structure sql dbstore load() ligne 96 : pourquoi forever (+ break interne) alors que la taille est calculé juste avant ? pourquoi utiliser query.size() pour connaitre la taille avant chargement au lieu de _data.size() après chargement ? (problème de post condition : normalement, la post condition est "_rowCount correspond au nombre de ligne chargée" ; là, en cas d'erreur de lecture, la post condition n'est pas respectée) pourquoi utiliser _data.append(data) dans load() au lieu de addData() ? Pourquoi QList au lieu de QVector ? en utilisant QList, tu utilises forcement un conteneur contigue en mémoire identique à QVector il faut utiliser QLinkedList pour utiliser une liste chainée ici sémantique d'une collection (sauf la fonction load) : pourquoi ne pas avoir utilisé un vecteur avec 1 fonction libre ? dbstore templaté = ok, très bien ; par contre dbstructure = 1 classe parente + 1 classe par structure = bof bof du coup, 1 DBStore par type de base, perte de l'intérêt du template (à part duplication du code) intérêt du CRTP ici ? dbstructure structure en dur dans le code. Pourquoi ne pas avoir utilisé des QVariants ? problème de granularité. Pourquoi ne pas avoir créé une classe par structure (prescription, symptom, urgence, etc.) gui fenetre pas déplacable, pas redimenssionable menu principal agréable. aspect différent sous linux et windows Pas d'utilisation des previews pour éviter les attentes inutiles (par exemple, si on se trompe en cliquant ou si on veut faire un zoom toute de suite sans attendre que tout s'affiche) Pour un patient donnés, avoir la liste de tous les examens sous forme de preview (pour trouver rapidement la radio de la jambe ou la radio de la face postérieur du cubitus avec extension du rétrograde inversé...) liste des patients fenêtre trop petite par rapport à la quantité d'information recherche : prend en compte la casse, permet pas des recherches avancées (patients passé cette semaine patients qui prennent un certain médicament, patients en fonction de l'age ou sexe, etc.) Non évolutif sans modifier le code : par exemple si on veut ajouter un 4ème onglet ou ajouter le n° de sécu dans la feuille du patient Comment on fait sortir un patient ? Est ce que l'on a un suivi sur les anciens patients ? Si un patient revient ? Pas vu tout de suite le bouton permettant de revenir au menu principal Salon pourquoi un menu intermédiaire "entrer dans le salon" ? la même personne plusieurs fois connecté = plusieurs connexion dans le salon le serveur gère plusieurs fois la même personne connecté comme étant plusieurs personnes différentes pas logique dans certain cas (salon). Idem pour les données générées pour les patients Police pas forcement agréable à long terme (mais donne un style particulier à l'application) pas de possibilité de changer de police Utilité d'avoir un bouton d'information sur la connexion visible en permanence ? J'aurais préféré un menu settings permettant de modifier les paramètres de l'application et d'avoir les infos Menu urgences : fluide. Pas de grille pour connaître l'échelle et défilement continue (difficile de calcul la fréquence précisiement en mesurant le temps entre de complexe QRS) Pas utile d'avoir le menu de préférence en permanence animation pour affichier les menus sympas (mais la rotation trop lente : pas paramétrable) Résultats d'analyse pourquoi avoir mis un ECG comme image dans le menu alors que les ECG ne sont pas là ? par type d'analyse et pas par patient. Pas de recherche possible (au bout de 6 mois, 2000 patients et 5000 analyses, comment retrouve-t-on les infos) Scan : application en attente de chargement le temps que les images soient envoyées affichage de toutes les images sur la gauche très partique et fluide. déplacement entre les images intuitif Pas de possibilité de zoom mode animation : pas de controle fin (avancer, reculer, avoir un curseur pour se déplacer rapidement entre devant et derrière le thorax) mais les contrôles présents sont utiles et pratiques Radios : idem, application bloqué pendant le chargement zoom trop important à chaque clic, pas de possibilité de zoom adapaté (à la souris par exemple ou avec un slider). lors du zoom, perte du centre (si le point d'intérêt est au centre et que l'on zoom, il faut redéplacer l'image) Déplacement sur les bords génants et de permet pas de centrer sur un bord de l'image Pas de possibilité de traitement d'images (luminosité, contraste) Client respect de la "règle" 1 fichier = 1 classe main.cpp win.initialize() -> nom ne permet pas de savoir que fait également show() c'est quoi "qmlRegisterType" et pourquoi là ? -> pas de doc MainWindow #include -> module complet au lieu de la classe seule. De plus, aucune classe Qt utilisée dans l'en-tête = inclusion inutile MainWindow qui n'est pas une QMainWindow ? possibilité de confusion entre 2 classes de nom proche mais qui ne sont pas similaires Pourquoi héritage de QObject ? Pourquoi faire une classe qui initialise tout alors que l'on peut tout initialiser dans le main ? encapsulation abisive ? Initialisation de QmlApplicationViewer dans le constructeur : pour initialiser les paramètre du viewer dans le constructeur de mainwindow et pas dans le contructeur du viewer directement ? Et si l'initialisation du viewer est faite dans le constructeur du viewer, il n'est peut être pas utile d'avoir autant de fonctions disponible en public ? C'est mieux quand on commente la ligne "_viewer->setWindowFlags(Qt::FramelessWindowHint);" pour avoir une fenêtre déplacable et dimenssionable :) Fonction initialize() -> en fait, cette fonction n'initialize pas grand chose, non ? beaucoup de choses sont initialisé dans le constructeur Fonction hookMgr : on voit pas non plus tout de suite que c'est une macro avec singleton ça simplifie l'écriture mais la compréhension ? TcpSocket liste de tous les handle et send en dur. Utilisation d'une chaîne de responsabilité ? Interval entre 2 pings : ok, pour la reconnexion au serveur. gestion de perte du connexion au serveur efficace (mais dommage que l'on ne peut plus travaillé en cas de déco...) interval, IP, port, login, pass : en dur, non paramétrable handleMessage : pourquoi pointeur et pas référence ? utilisation d'un tableau de fonction : approche pas top (préfère utilisation de pattern) unhandled : pourquoi faire une fonction qui fait rien ? fichier beaucoup trop long -> problème du à l'utilisation d'une fonction par message OpCode : ok, idem serveur HookMgr : même problème que pour TcpSocket, 1 fonction par message rend illisble le code pourquoi singleton ? pourquoi héritage de QObject ? Est-ce vraiement utile d'avoir 3 conteneurs de hooks différents ? ECGViewer création de la vue en C++ pour être intégré dans QML : très bien. Utilisation des signaux/slots découpage des fichiers pas claire (tous dans include) liste de points : QList, pas le plus efficace pour cela (mais pas sur que ce soit une liste chaînée... mais dans tous les cas, un tableau de données contigues aurait été mieux) addPoints() : suppression de points en début et ajout en fin : risque allocation et copie inutile. L'idéal : un circular buffer Ne traite pas les problèmes de perte de données : seul les intensités sont envoyé. Si perte de données (par exemple 10 % des donneés), distance entre 2 QRS est modifiée et le calcul de fréquence est faussé. Nécessité d'envoyer l'intensité + le temps : perte de données = juste un "trou" dans l'ECG mais les calculs restent correctent QmlApplicationViewer pour l'inclure dans le projet avec "include(qmlapplicationviewer/qmlapplicationviewer.pri)" et pas comme des fichiers classiques ? Utilité de la fonction createApplication() ? ... ok, classe créée automatiquement par Qt Creator :) QML logic.js : traitement des json par javascript. et si la taille des messages est conséquente ? risque de saturation. A mon avis : traitement potentiellement lourd en C++ main.qml : beaucoup de chose. Peut être trop. Ca fait lourd le QML... Components : très bien découpé, code très clair ColorPicker : fluide, à comparer avec le code de l'exo (la version QML que j'avais fait me semblait moins fluide, peut être un problème de taille des la zone de selection) pas génant d'avoir une fenêtre de selection petite, ici on n'a pas besoin de plus Utilisation intelligente des liste et délégate Excellente maitrise du QML