I am looking for feedback to improve code readability and improve the algorithm's efficiency. I have already posted a question on code review about this. The feedback was helpful, and I did rethink the data structures.

Link to the previous question: Job Scheduling Algorithm

Info about classes:

Item is the item the machines work on. Machine is a single machine that can be used from start time to end time. A single task is a single item on a machine. Job is a complete job, and can have more than one task.

The algorithm schedules the tasks, re-orders them for the specific machine, and adds to total profit is a job is finished within the schedule.

class ItemModel:
    def __init__(self, name:str, starts:int, ends:int)->None:
        self.name = name
        self.start = start 
        self.ends = ends 
class JobModel:
    def __init__(self, tasks_needed:int, tasks_done:int, profit:int)->None:
        self.tasks_needed = tasks_needed
        self.tasks_done = tasks_done 
        self.profit = profit 
class MachineModel:
    def __init__(self, starts:int, ends:int)->None:
        self.starts = starts 
        self.ends = ends 

class View:
    def output_profit(profit:int)->None:
        print("profit: " + str(profit), end = "\n")
    def output_sublist(sublist:[int])->None:
        for item in sublist:
            print(str(item), end = " ")

def algorithm(items:[ItemModel], jobs:[JobModel], machines:[MachineModel])->(int, [ItemModel]):
    total_profit = 0 
    for start in range(len(items)):
        for end in range(len(items)):
            for job_row in range(len(jobs)):
                if (time >= jobs[job_row].start_time) and (time <= jobs[job_row].end_time):
                    jobs[job_row].task_done += 1 
            profit = 0 
            for job_row in range(len(jobs)):
                if jobs[row].tasks_needed == jobs[row].tasks_done:
                    profit += jobs[row].profit
            yield (profit, sublist)
def main():
    items = [ItemModel("a", 0, 2), ItemModel("b", 3, 4)]
    jobs = [JobModel(0, 0, 12), JobModel(0, 0, 10)]
    machines = [MachineModel(0, 1), MachineModel(0, 7)]
    View view = View() 
    for pair in algorithm(items, jobs, machines):

if __name__ == "__main__":
  • 'algorithm' is not correct at all and will fail at runtime. please make sure your code works before posting it. start and end are never read. time is never defined. 'row' is never defined--it is typo-d as 'job_row' the second time. use 'for job in robs', not 'for job_row in range(len(jobs)) together with 'jobs[job_row]'. I would generally recommend a linter, which should catch all these errors.

  • Add a docstring to "algorithm", describing what its inputs are, and what it returns. Make sure to describe what it returns in terms of MEANING, and not what the algorithm does. ("returns the best way to schedule..." and not "loops through...") Change the name of 'algorithm' to reflect what it does (ex. 'schedule').

  • I personally think it's fine to put the contents of 'main' directly in the if __name__ == "__main__" block, but I've seen it both ways.

  • For the output_ methods, consider returning a string, and printing the string, as two steps. " ".join() may help.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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