7
\$\begingroup\$

I have written code that can compile, execute, generate output and match that output with the actual output of the given code, kind of like this site. Currently, this supports only Java and C++ code. I need some suggestions about design, OO, architecture, packaging or any other suggestion that I could improve on.

This is main method from where the application start:

@Component
public class App {

  @Autowired
  QueueService<Submission> queue;

  @Autowired
  Compiler compiler;

  @Autowired
  VerdictService verdictService;

  public static void main( String[] args ) {
      AbstractApplicationContext context = new AnnotationConfigApplicationContext(AppConfig.class);
      App app = context.getBean(App.class);
      app.task();
}

  private void task() {
     while (true) {
          if (!queue.isEmpty()) {
              Submission submission = queue.get();
              Verdict verdict = compiler.submit(submission); 
              verdictService.post(verdict);
          }
      }
  }
}

The compiler object takes a submission object that is a hibernate entity class. It contains the source code and has a relation with problem entity that contains the input file and the actual output of that problem. The latter generated output from the given code will match with that actual output whether the submitted code is right or wrong.

I have used lombok for the getter and setter:

@Entity
public class Submission {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
@Column(name = "submission_id")
private long id;

@Column(name = "username")
private String userName;

@Column(name = "status")
private boolean status;

@ManyToOne(optional = false)
@JoinColumn(name = "problem_id", referencedColumnName = "problem_id")
private Problem problem;

@Column(name = "language")
private String lang;

@Column(name = "src_file", length = 100000)
private byte[] srcFile;

@Column(name = "time")
@Temporal(TemporalType.TIMESTAMP)
private Date time;
}

Problem entity:

@Entity
public class Problem {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
@Column(name = "problem_id")
private long id;

@Column(name = "name")
private String name;

@Column(name = "time_limit", nullable = false)
private Long timeLimit;

@Column(name = "memory_limit", nullable = false)
private Long memoryLimit;

@Column(name = "in_file", length = 100000)
private byte[] inputFile;

@Column(name = "res_file", length = 100000)
private byte[] resultFile;

@Column(name = "time")
@Temporal(TemporalType.TIMESTAMP)
private Date time;
}

The compiler class, from where code will be compiled, executed and match the output:

@Service
public class CompilerImpl implements Compiler {

@Value("${working.dir}")
private String WORKING_DIR;
@Value("${file.name.cpp}")
private String FILE_NAME_CPP;
@Value("${file.name.java}")
private String FILE_NAME_JAVA;
@Value("${file.name.input}")
private String INPUT_FILE;
@Value("${file.name.output}")
private String OUTPUT_FILE;
@Value("${file.name.result}")
private String RESULT_FILE;

@Autowired
Engine engine;

@Autowired
DiffChecker diffChecker;

private Submission submission;

private DTO dto;

@Override
public Verdict submit(Submission submission) {
    this.submission = submission;
    init();
    boolean flow = true;
    CompileStatus status;
    ProcessBuilder compile = ProcessBuilderFactory.getProcessBuilder(dto);
    ProcessBuilder execute = ProcessBuilderFactory.getExecutionProcessBuilder(dto);

    status = engine.compile(compile);
    if (status != CompileStatus.COMPILE_SUCCESS) //CompileStatus is enum class
        flow = false;

    if (flow) {
        status = engine.execute(execute, dto);
        if (status != CompileStatus.EXECUTION_SUCCESS)
            flow = false;
    }

    if (flow) {
        status = diffCheck();
        logger.info("Diff checker, {}", status);
    }

    Verdict verdict = new Verdict(); //Verdict is also entity class that has one to one relation with submission entity.
    verdict.setCompileStatus(status);
    verdict.setSubmission(submission);
    return verdict;
}

private void init() {
    makeDTO();
    FileUtil.batchCreate(submission, dto);
}

private void makeDTO() {
    String fileName = Language.langCheck(submission.getLang()).equals(Language.CPP) ? FILE_NAME_CPP : FILE_NAME_JAVA;
    this.dto = new DTO(
            submission.getLang().toLowerCase(),
            fileName,
            INPUT_FILE,
            OUTPUT_FILE,
            RESULT_FILE,
            WORKING_DIR + submission.getId(),
            submission.getProblem().getTimeLimit(),
            submission.getProblem().getMemoryLimit()
    );
}

private CompileStatus diffCheck() {
    String codeResult = FileUtil.fileToString(dto.getResultFilePath());
    String judgeOutput = FileUtil.fileToString(dto.getOutputFilePath());
    return diffChecker.check(codeResult, judgeOutput);
}

}

I am a creating a working directory based on the submission id. It's like the main directory /home/seal/test/, and if the submission is like 123, the working directory will be /home/seal/test/123. In that directory I create three file: src file, input file, and output file.

I take the necessary thing from the submission object to place in DTO object:

public class DTO {
private final String lang;
private final String srcFileName;
private final String inputFileName;
private final String outputFileName;
private final String resultFile;
private final String workingDir;
private final long timeLimit;
private final long memoryLimit;

public String getLang() {
    return lang.toLowerCase();
}

public String getSrcPath() {
    return workingDir + "/" + srcFileName;
}

public String getInputFilePath() {
    return workingDir + "/" + inputFileName;
}

public String getOutputFilePath() {
    return workingDir + "/" + outputFileName;
}

public String getWorkingDir() {
    return workingDir;
}

public String getResultFilePath() {
    return workingDir + "/" + resultFile;
}

// if var ==  true then return file name with extension A.cpp otherwise without extension like A
public String getSrcName(boolean var) {
    String f = null;
    if (var) {
        f = srcFileName;
    } else {
        String[] strings = srcFileName.split("\\.");
        f = strings[0];
    }
    return f;
}
}

This is the ProcessBuilderFactory that will return the ProcessBuilder for compilation or execution:

public class ProcessBuilderFactory {

public static ProcessBuilder getProcessBuilder(DTO dto) {
    ProcessBuilder processBuilder = null;
    switch (dto.getLang()) {
        case Language.CPP :
            processBuilder = new ProcessBuilder("g++", dto.getSrcName(true), "-o", dto.getSrcName(false));
            break;
        case Language.JAVA :
            processBuilder = new ProcessBuilder("javac", dto.getSrcName(true));
            break;
    }
    processBuilder.directory(new File(dto.getWorkingDir()));
    return processBuilder;
}

public static ProcessBuilder getExecutionProcessBuilder(DTO dto) {
    ProcessBuilder processBuilder = null;

    switch (dto.getLang()) {
        case Language.CPP :
            processBuilder = new ProcessBuilder("./" + dto.getSrcName(false));
            break;
        case Language.JAVA :
            processBuilder = new ProcessBuilder("java", "-cp", ".", dto.getSrcName(false));
            break;
    }

    processBuilder.directory(new File(dto.getWorkingDir()));
    processBuilder.redirectInput(new File(dto.getInputFilePath()));
    processBuilder.redirectOutput(new File(dto.getResultFilePath()));

    return processBuilder;
}

}

This Engine class will compile and execute:

@Service
public class Engine {
public CompileStatus compile(ProcessBuilder processBuilder) {
    boolean compiled = true;
    processBuilder.redirectErrorStream(true);

    try {
        Process process = processBuilder.start();
        InputStream inputStream = process.getInputStream();
        String temp;
        try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream))) {
            process.waitFor();
            while ((temp = reader.readLine()) != null) {
                compiled = false;
            }
            if (!compiled) {
                inputStream.close();
                return CompileStatus.COMPILE_ERROR;
            }
            inputStream.close();
            return CompileStatus.COMPILE_SUCCESS;
        }
    } catch (IOException | InterruptedException e) {            
    }

    return CompileStatus.COMPILE_ERROR;
}

public CompileStatus execute(ProcessBuilder processBuilder, DTO dto) {
    processBuilder.redirectErrorStream(true);
    try {
        Process process = processBuilder.start();
        if (!process.waitFor(dto.getTimeLimit(), TimeUnit.MILLISECONDS))
            return CompileStatus.TIME_LIMIT_EXIT;
        int exitCode = process.exitValue();
        if (exitCode != 0) {
            return CompileStatus.RUN_TIME_ERROR;
        }
    } catch (Exception e) {
        return CompileStatus.EXECUTION_ERROR;
    }

    return CompileStatus.EXECUTION_SUCCESS;
}
}

This class will check the original output and the output generated by the submitted code:

@Service
public class DiffCheckerImpl implements DiffChecker {
private static final String REGRX = "(?m)(^\\s+|[\\t\\f ](?=[\\t\\f ])|[\\t\\f ]$|\\s+\\z)";

@Override
public CompileStatus check(String codeOutput, String output) {
    codeOutput = codeOutput.replaceAll(REGRX, "").trim();
    output = output.trim();
    DiffMatchPatch dmp = new DiffMatchPatch();
    CompileStatus status;

    List<DiffMatchPatch.Diff> list = dmp.diffMain(codeOutput, output);
    if (list.size() == 1 && list.get(0).operation == DiffMatchPatch.Operation.EQUAL)
        status = CompileStatus.ACCEPTED;
    else
        status = CompileStatus.WRONG;

    return status;
}
}

This is my utility class:

public class FileUtil {
public static byte[] readFileAsByte(String path) {
    byte[] bytes = null;
    try {
        bytes = Files.readAllBytes(Paths.get(path));
    } catch (IOException e) {
        throw new RuntimeException(e);
    }
    return bytes;
}

public static void writeByteToFile(byte[] bytes, String path) {
    try {
        Files.write(Paths.get(path), bytes);
    } catch (IOException e) {
        throw new RuntimeException(e);
    }
}

public static void createDirectory(String path) {
    try {
        Files.createDirectory(Paths.get(path));
    } catch (IOException e) {
        throw new RuntimeException(e);
    }
}

public static void batchCreate(Submission submission, DTO dto) {
    FileUtil.createDirectory(dto.getWorkingDir());
    FileUtil.writeByteToFile(submission.getSrcFile(), dto.getSrcPath());
    FileUtil.writeByteToFile(submission.getProblem().getInputFile(), dto.getInputFilePath());
    FileUtil.writeByteToFile(submission.getProblem().getResultFile(), dto.getOutputFilePath());
}

Here is my package architecture:

enter image description here

I'll also give my GitHub repo if someone needs it.

\$\endgroup\$
  • 2
    \$\begingroup\$ You certainly put some work into writing a great question, nicely done! \$\endgroup\$ – Phrancis Aug 27 '16 at 16:26
3
\$\begingroup\$

Just a few thoughts, this is by no means a complete review.

Variable naming

  • In CompilerImpl you have variables like

    private String WORKING_DIR;
    private String FILE_NAME_CPP;
    

    and so on. The (albeit rather old) code conventions suggest that this case should be only used for constants, not for regular variables.

  • In public String getSrcName(boolean var); the meaning of var is so unobvious that you felt like commenting it yourself. Why not name it something like boolean withExtension?

Potential Nullpointers

See e.g. ProcessBuilderFactory.getProcessBuilder:

public static ProcessBuilder getProcessBuilder(DTO dto) {
    ProcessBuilder processBuilder = null;
    switch (dto.getLang()) {
        case Language.CPP :
            processBuilder = new ProcessBuilder("g++", dto.getSrcName(true), "-o", dto.getSrcName(false));
            break;
        case Language.JAVA :
            processBuilder = new ProcessBuilder("javac", dto.getSrcName(true));
            break;
    }
    processBuilder.directory(new File(dto.getWorkingDir()));
    return processBuilder;
}

As far as I can see dto.getLang() comes directly from the input submission (not fully shown), so there is no guarantee that it will be either Language.CPP or Language.JAVA, leaving processBuilder potentially null. Since dto.getLang() returns a String I would suggest to replace it with an enum and put some validation before accepting a submission so that valid values are guaranteed during processing.

Superfluous variables

The following struck my eye because flow is actually superfluous:

boolean flow = true;
CompileStatus status;
ProcessBuilder compile = ProcessBuilderFactory.getProcessBuilder(dto);
ProcessBuilder execute = ProcessBuilderFactory.getExecutionProcessBuilder(dto);

status = engine.compile(compile);
if (status != CompileStatus.COMPILE_SUCCESS) //CompileStatus is enum class
    flow = false;

if (flow) {
    status = engine.execute(execute, dto);
    if (status != CompileStatus.EXECUTION_SUCCESS)
        flow = false;
}

if (flow) {
    status = diffCheck();
    logger.info("Diff checker, {}", status);
}

Why not implement it like this?

ProcessBuilder compile = ProcessBuilderFactory.getProcessBuilder(dto);
ProcessBuilder execute = ProcessBuilderFactory.getExecutionProcessBuilder(dto);

CompileStatus status = engine.compile(compile);

if (status == CompileStatus.COMPILE_SUCCESS) {
    status = engine.execute(execute, dto);
}

if (status == CompileStatus.EXECUTION_SUCCESS) {
    status = diffCheck();
    logger.info("Diff checker, {}", status);
}
\$\endgroup\$
1
\$\begingroup\$

If you're going for nice code, take a look at the SonarQube tool. It has many nice plugins and it's good for Java.

So go grab it, follow instructions and run it for your projects - you will see many complaints, suggestions, etc.

I see you store files in database. I don't remember seeing this approach very often. Usually people store files in Amazon S3 for long term use. Or if its temporary than in temporary folder "/tmp" on server. There should be a purpose to store files, may be if its needed for the future it could be processed to be documents in MongoDB.

Solutions seems to be single threaded. You might have thread pool and submitting data to 30-50 running threads.

https://docs.oracle.com/javase/tutorial/essential/concurrency/pools.html

\$\endgroup\$

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.