4
\$\begingroup\$

I was recently told I need to change my coding style. An Interviewer told me he would like to see "more formalized class structure and organization". I have attached few of my cpp files.

if you can you review it and give me your feedback on the things I can improve that will be extremely helpful.

Challenge:

Given n robots (n different paths), m nodes, how much time all the robots requires to complete the circuit. If 2 robots arrive at a node, the 2nd robot should wait till the 1st robot completes its task.

File structure:

├── Solution
│   ├── data
│   │   ├── ***.csv
│   │   ├── ***.csv
│   │   ├── ***.csv
│   ├── include
│   │   ├── robot.h
│   │   ├── node.h
│   │   ├── simulation.h
│   ├── output
│   │   ├── ***.csv
│   │   ├── ***.csv
│   ├── src
│   │   ├── main.cpp
│   │   ├── planner.cpp
│   │   ├── read_write.cpp
│   │   ├── simulation.cpp
│   ├── test
│   │   ├── simulationTest.cpp
│   ├── CMakeLists.txt
│   ├── README.md

simulation.h

#pragma once

...//all headers

#include "robot.h"
#include "node.h"

class simulation{
private:
  std::string robots_ip_string;  
  std::string paths_ip_string;  
  std::string nodes_ip_string; 
  std::string time_op_string;
  std::string visits_op_string;
  std::vector<robot*> robots;  //stores the robot info
  std::unordered_map<int, node*> nodes;
  std::unordered_map<int, std::vector<int>> paths;

public:

  simulation();
  ~simulation();

  void read_robots_input();
  void read_paths_input();
  void read_nodes_input();

  void print_robots();
  void print_nodes();
  void print_paths();
  void run_simulation();
  void simulation_time();
  void visited_node_info();
  std::vector<robot*> get_robots();
};

robot.h

#pragma once

struct robot{
  int robot_id;
  std::string robot_type;
  int robot_speed;
  std::vector<int> path_to_follow;
  int path_length;
  int run_time;  
};

planner.cpp

#include "simulation.h"

struct Path_Queue_Comparator{
  bool operator() (std::pair<robot*,int> const& r1, std::pair<robot*,int> const& r2){
    if (r1.first->run_time == r2.first->run_time)
      return (r2.first->robot_type == "organizer") ? true : false;
    return r1.first->run_time > r2.first->run_time;
  }
};

void simulation::run_simulation(){
  std::priority_queue< std::pair<robot*,int>, std::vector<std::pair<robot*,int>>, Path_Queue_Comparator> path_q;

  for(auto r: robots)
    path_q.push({r, 0});

  while(!path_q.empty()){
    std::pair<robot*,int> temp = path_q.top();   

    robot* current_state = temp.first;  
    int current_index = temp.second;    
    path_q.pop();

    int next_index = current_index+1;

    if(current_index < current_state->path_length){                 
      int node_idx = current_state->path_to_follow[current_index]; 
      int wait_time;

      int task_time = (current_state->robot_type == "organizer") ?
          nodes[current_state->path_to_follow[current_index]]->task_time_organizer_robot
              : nodes[current_state->path_to_follow[current_index]]->task_time_mover_robot;

      if (current_state->run_time >= nodes[node_idx]->leave_time){  
          wait_time = 0;
          nodes[node_idx]->entry_time = current_state->run_time;
          nodes[node_idx]->leave_time = current_state->run_time + task_time;
      }
      else{                                                         
        wait_time = nodes[node_idx]->leave_time - current_state->run_time;
        nodes[node_idx]->entry_time = current_state->run_time + wait_time;
        nodes[node_idx]->leave_time = current_state->run_time + wait_time + task_time;
      }

      current_state->run_time += wait_time;
      current_state->run_time += task_time;

      if(next_index <  current_state->path_to_follow.size())
        current_state->run_time +=  current_state->robot_speed; 

      nodes[node_idx]->visited_robots.insert(current_state);

      path_q.push({current_state, next_index});
    }
  }
}

This is how I usually code (other files are similar). Please let me know the things I need to change in my coding style! It will be extremely helpful.

Thanks

\$\endgroup\$
1
  • 1
    \$\begingroup\$ @Emma challenge: Given n robots (n different paths), m nodes, how much time all the robots requires to complete the circuit. If 2 robots arrive at a node, the 2nd robot should wait till the 1st robot completes its task. \$\endgroup\$
    – Miracle
    Commented Oct 6, 2020 at 21:03

1 Answer 1

2
\$\begingroup\$

Avoid storing redundant data in classes

I see variables like these in class simulation:

std::string robots_ip_string;
std::string paths_ip_string;
std::string nodes_ip_string;
...

Usually, repeating the name of the type in the name of a variable is unnecessary, so at first sight I would say: why not just call them robots_ip, paths_ip, and so on? However, a closer look shows that there is also:

std::vector<robot*> robots;
std::unordered_map<int, node*> nodes;
std::unordered_map<int, std::vector<int>> paths;

This looks to me like you read the input into a string, and them later convert the textual input into more structured data. But is it really necessary to keep the input strings around? I would remove the *_string member variables, and ensure that any function that reads input stores it in a local variable in that function, and them immediately converts it into the vectors and maps.

Another potentially case of redundant information is in struct robot:

struct robot{
  ...
  std::vector<int> path_to_follow;
  int path_length;
  ...
};

A std::vector knows how long it is. Instead of storing path_length, maybe you can get the same information from calling path_to_follow.size()? If so, I would remove path_length.

Separate input/output as much as possible from logic

In class simulation, there are read_*_input() functions, print_*() functions, run_simulation() and a few other functions, most of them take no arguments and return void. If I see a function like:

void read_robots_input();

Then I cannot tell where it is reading the input from. Is it from a file, standard input, the network? If it's from a file, which filename? Also, why are there three different read_*_input() functions? Would you ever use one but not the others? Also, what if I create an instance of class simulation but never call read_*_inputs()? Here are a few suggested changes:

  • Have the constructor read all required inputs. This way, after creating an instance of class simulation, you know that it is in a valid state and ready to run the simulation.
  • Have a way to tell the constructor which inputs to read. This could be three filenames, or even better three references to std::istream objects, so that the constructor no longer cares if the input comes from a file, a stringstream or anything else with the std::istream interface.

Note that this doesn't mean you have to put everything into the constructor itself, you can still have private helper functions that parse the individual files and call those from the constructor.

For writing the output, use (a) function(s) that takes references to std::ostream objects. Again, this allows the caller to decide whether to print to a file or to std::cout for example.

What do simulation_time() and visited_node_info() do? Do they print this information somewhere? If so, make it clear from the name, and have them match the other print_*() functions.

Use an enum class for robot_type

Using an enum class for robot_type will avoid expensive string comparisons such as in:

return (r2.first->robot_type == "organizer") ? true : false;

Instead, make it so you can write it like:

return r2.first->robot_type == RobotType::ORGANIZER;

Avoid repeating the name of a class in its member variables

Again, the goal is to avoid being redundant and repeating yourself. struct robot can be written as:

struct robot{
  int id;
  RobotType type;
  int speed;
  std::vector<int> path_to_follow;
  int path_length;
  int run_time;  
};

Call continue/break/return early to reduce indentation levels

In run_simulation(), there is a big if-statement that contains most of the code. However, you can just do with a very small if-statement that goes to the next iteration of the loop:

while(!path_q.empty()){
  ...

  if(current_index == current_state->path_length)
    continue;

  int node_idx = ...
  ...
}

Use more auto, structured bindings

Avoid repeating types by using auto. For example:

std::pair<robot*,int> temp = path_q.top();

Can be rewritten as:

auto temp = path_q.top();

If you can use C++17, then it becomes even easier to unpack the pair:

auto [current_state, current_index] = path_q.top();

Use references as aliases

I see the following piece of code repeated more than once:

nodes[current_state->path_to_follow[current_index]]->...

That's quite long, and it makes it hard to see what is going on. First, you could have used node_idx to shorten this to:

nodes[node_idx]->...

But it would be better to give a clear name to this expression. You can do this by creating a reference, like so:

auto &current_node = nodes[node_idx];

Then the rest of the code becomes much clearer:

int task_time = current_state->type == RobotType::ORGANIZER ?
                current_node->task_time_organizer_robot :
                current_node->task_time_mover_robot;

if (current_state->run_time >= current_node->leave_time) {
    wait_time = 0;
    current_node->entry_time = current_state->run_time;
    ...

Note that current_state should probably be named current_robot, to be consistent.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ By the way, there's just been a nice talk at CppCon about the issue with those void() member functions: youtube.com/watch?v=Wg1f9Sufyic \$\endgroup\$
    – G. Sliepen
    Commented Oct 6, 2020 at 22:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.