Reviewer 1: 7 The names of some functions and variables are not very descriptive, and you have not consistently named channels with ‘channel’.#BBFABBA6;"> Examples of poorly descriptive names are the functions ‘e2p’, ‘p2e’, #FF5582A6;">and the variable ‘done’. Additionally, they do not always correspond to which files they are located in. Examples of #FFF3A3A6;">inconsistent naming of channels are ‘buttonchannel’ and ‘florsensor’. It is difficult to see from the main function which modules are used and how they are interconnected. It is not clear which modules control what. #BBFABBA6;">It is not intuitive that the entire program runs in ‘control.go’. Not having a global package called ‘elevator’ means you had to run ‘control.run’. You have no unnecessary comments, and the ones you have are descriptive. It is difficult to understand the relationship between actions and different functions. This makes it difficult to find which files the different functions are located in. #FF5582A6;">One example is that ‘controlPanelRun’ calls ‘addOrder’, so you have to go to ‘orders.go’, and this function uses a struct called ‘ElevatorRequest’ which is found in ‘elevator.go’. This could be more organized. It is not intuitive how the different parts of the system are designed. It is difficult to see how ‘orderassigner’ and ‘ack-signal’ work. We believe the code is divided into modules that only solve one problem each, but this is not clear because it is difficult to see how it works. The code is somewhat messy. It is difficult to understand how the elevator actually operates, and especially how. #FF5582A6;">We spent a long time finding ‘Run’, which was not in ‘elevator’, but in ‘control’. Therefore, some restructuring of the code is needed to make it more readable.”
Reviewer 2: 9
- Well structured entry point; easy to see what modules communicate with each other
- Relatively easy to recognize working principles; p2p, acknowledgement
- Modules are to a high degree coherent and complete, and still not too long
- #BBFABBA6;">Code is overall understandable; functions are mostly pure and well named, except OptimalRequest and UniqueOrders, which we found confusing
- Information flows mostly in the same direction, and is easy to follow
- Comments for the most part only used where necessary, but sometimes a little excessively
- elevator.Run() could have been split into Initialize() and Run() to improve purity
Reviewer 3: 6 The modularization into only two modules, ‘network’ and ‘elevator’, seems superficial. A more granular approach would without doubt enhance both code organization and maintainability. Most files in the root directory could have been organized into folders, improving clarity and navigability within the project structure. #FF5582A6;">Found it confusing to understand some of the semi-generated file ‘protocol.pb.go’ within the network-module. The same file also contains enumeration value maps for elevator directions and states, which does not belong in a network-module. Except for the comments above about the network-module, it seems well-functioning and easy to understand and trace - both udp.go and network.go. Excessive comments on trivial or self-explanatory aspects of the code cluttered the readability without adding any significant value. Such comments are found in i.e. #BBFABBA6;">doors.go Some depreciated code was still included in the final code, such as in protocol.pb.go. The code has only minimal use of shared variables.
Reviewer 4: 8
- Short and nice main.go file. #FF5582A6;">However there seems to be an init()-function that is not in use.
- The function run() in elevator.go was rather heavy to read with that many cases and no spaces. Seems more complicated that it needed to be.
- #BBFABBA6;">Some of the if-conditions had a tendency to be a bit long aswell. Could defined have been shortened and concatenated as bool variables above to make it more readable
- #FF5582A6;">enums like direction and state was defined in both elevator.go and protocol.pb.go. Could have been defined at one place.
- #FF5582A6;">Even though the protocol.pb.go file was rather large it was easy to read with short and concise functions.
- Good solution with start.bash and stop.bash to keep the system alive. As well as the server structure seems like a robust and cool idea!
- Good idea to init the script with ID and Port with the go run command.
Reviewer 5: 6 The entry points give little insight to how the system works, given the function calls InitNetworkServer and Run. Upon looking at those functions, the architecture is still not revealed. Only by following the function calls several times, the case AssignGlobalOrders (in elevator.go) reveals peer to peer. There are several instances where methods are used over functions in the network module. From a code quality perspective, functions with inputs and outputs are preferable so that core and shell can be distinguished. #BBFABBA6;">The variable names have room for improvement to enhance readability. Names like es, ess, o, e2p, el and the index variables i and j in the for-loops could have been more describing. #FF5582A6;">The variables are used in different places, which makes it confusing with these names. There is strong coupling between the modules control, network and cost function.#FF5582A6;"> Sending to the network is done directly in the modules, instead of sending it out of the module. The elevator module assigns orders which are not intuitive. Smaller modules, such as UDP, doors and orders are coherent. The code is not very easy to understand due to non-describing names and the#FF5582A6;"> lack of a main function. #FF5582A6;">The flow between the modules is hard to follow, due to that a lot of the functionality is packed into other functions, methods and structs and that.