Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm seeking a general review, but I'm particularly interested in style.

This program gets 2 lists of peptide to spectrum matches, so every spectrum title is linked to a list of 1 or 10 possible peptides and this program checks for an occurrence on both lists. If they do occur on both lists, on what rank of 10 the peptides occur in the list that contains 10 peptides.

this is an example of the list with 10 peptides generated by deNovoGUI = DNG this is an example of the list with 1 peptides generated by searchGUI = SG

The mfg file is the input for both the SG and DNG algorithms. It contains a lot of stuff and mainly also every spectrum title. But because not every spectrum is processable by the algorithms, they will filter out some spectra that they don't use. To compare the use of this list to see if both algorithms used a certain spectrum X, see ValidForMatch.

package com.compomics.searchguicompare;

import java.io.File;
import java.io.IOException;

public class Main {
    public static void main(String[] args) throws IOException {
        searchGUIcompare main = new searchGUIcompare();
        File SGFile = new File("C:\\Users\\Paulo\\Documents\\NetBeansProjects\\searchGUIcompare\\src\\main\\recourses\\Paulo.txt");
        File DNGFile = new File("C:\\Users\\Paulo\\Documents\\NetBeansProjects\\searchGUIcompare\\src\\main\\recourses\\20080311_CPTAC6_07_6A005.mgf.out");
        File mgfFile = new File("C:\\Users\\Paulo\\Documents\\NetBeansProjects\\searchGUIcompare\\src\\main\\recourses\\20080311_CPTAC6_07_6A005.mgf");
        main.readSequencerOutput (SGFile, "sg");
        main.readSequencerOutput (DNGFile, "dng");
        main.compare(mgfFile);
    }
}

package com.compomics.searchguicompare;

import java.util.ArrayList;
import java.util.Collections;

/**
 *
 * @author Paulo
 */
public class Match {
    private Peptide SGPeptide;
    private Peptide DNGPeptide;
    private static int[][] rankList2 = new int[10][10];

    Match(Peptide SGPeptide, Peptide DNGPeptide){
        this.SGPeptide = SGPeptide;
        this.DNGPeptide = DNGPeptide;
        rankList2[SGPeptide.getIndex()][DNGPeptide.getIndex()]++;

        }


    public Peptide getSGPeptide(){
        return SGPeptide;
    }
    public Peptide getDNGPeptide(){
        return DNGPeptide;
    }
    public int[][] getRankList(){
        return rankList2;
    }
}



package com.compomics.searchguicompare;

import java.util.ArrayList;

/**
 *
 * @author Paulo
 */
class Peptide {

    private String score;
    private String sequence;
    private int index;
    private int modification;
    private ArrayList<String> modlist;

    Peptide(String peptideNG, String index, String score, int modification, ArrayList<String> modlist) {
    sequence = peptideNG;
    this.index = Integer.parseInt(index);
    this.score = score;
    this.modification = modification;
    this.modlist = modlist;
    }

    public String getSequence() {
        return sequence;
    }

    public int getIndex() {
        return index;
    }

    public String getScore(){
        return score;
    }

    public boolean isModified(){
        return modification == 1;
    }

    public ArrayList<String> getModifications(){
        return modlist;
    }



}


package com.compomics.searchguicompare;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.regex.*;

public class searchGUIcompare {

    private String score;
    private String spectrumNG;
    private ArrayList<Match> matchList = new ArrayList();
    private ArrayList<String> onlyInSG = new ArrayList();
    private ArrayList<String> onlyInDNG = new ArrayList();
    private ArrayList<String> noPSM = new ArrayList();
    private ArrayList<Peptide> peptideList = new ArrayList();
    private HashMap<String, ArrayList<Peptide>> peptideSGMap = new HashMap();
    private HashMap<String, ArrayList<Peptide>> peptideDNGMap = new HashMap();

    public void readSequencerOutput(File file, String SA) {
        try (BufferedReader rd = new BufferedReader(new FileReader(file))) {
            while (true) {
                String newLine = rd.readLine();
                if (newLine == null) {
                    break;
                }
                if (SA.toLowerCase().contains("dng") || SA.toLowerCase().contains("denovogui")) {
                    getPeptideDNG(newLine);
                } else if (SA.toLowerCase().contains("sg") || SA.toLowerCase().contains("searchgui")) {
                    getPeptideSG(newLine);
                } else {
                    System.out.println("invalid Sequencing algoritme, the supported algoritms are:");
                    System.out.println("SearchGUI = sg");
                    System.out.println("DeNovoGUI = dng");
                }
            }
        } catch (IOException ex) {
            System.out.println("invalid file");
        }
    }

    private void getPeptideSG(String newSGLine) {
        int i = newSGLine.indexOf(",");
        if (i != -1) {
            String spectrum = newSGLine.substring(newSGLine.lastIndexOf(",", newSGLine.lastIndexOf(",") - 1) + 1, newSGLine.lastIndexOf(","));
            String peptideSG = newSGLine.substring(i + 1, newSGLine.indexOf(",", i + 1));
            //replace I and L becous deNovoGUI will always use L when both are possible
            peptideSG = peptideSG.replace("I", "L");
            ArrayList<String> modlist = new ArrayList();
            Peptide SGPeptide = new Peptide(peptideSG, "0", "", 0, modlist);
            ArrayList<Peptide> oneSGPeptide = new ArrayList();
            oneSGPeptide.add(SGPeptide);
            peptideSGMap.put(spectrum, oneSGPeptide);
        }
    }

    private void getPeptideDNG(String newDNGLine) {

        if (!newDNGLine.isEmpty()) {

            // get the spectrum title
            if (newDNGLine.contains(">>") && newDNGLine.contains("SQS")) {
                // reset the arraylist when a new spectrum is read
                peptideList = new ArrayList();
                if (newDNGLine.contains("(")) {
                    spectrumNG = newDNGLine.substring(newDNGLine.indexOf("20080311"), newDNGLine.indexOf("(") - 1);
                }
            }

            //get the score
            int j = newDNGLine.indexOf("\t");
            if (j != -1) {
                score = newDNGLine.substring(j, newDNGLine.indexOf("\t", j + 1));
            }

            //get index (if line does not contain an index this will be overwriten and onl used when it does contain one)
            String index = newDNGLine.substring(0, 1);

            // get the peptide sequence
            // create the peptide objects
            // get the peptideList and add the list to the spectrumMap
            int i = newDNGLine.lastIndexOf("\t");
            int mod = 0;
            if (i != -1 && !newDNGLine.contains("#")) {
                String peptideNG = newDNGLine.substring(i + 1);
                if (peptideNG.contains("+")) {
                    mod = 1;
                }
                ArrayList<String> modlist = getMods(peptideNG);
                peptideNG = removeMod(peptideNG);
                //change all the I's to L's because some I's are there only when it comes from a modification
                peptideNG = peptideNG.replace("I", "L");
                Peptide peptide = new Peptide(peptideNG, index, score, mod, modlist);
                peptideList.add(peptide);
                peptideDNGMap.put(spectrumNG, peptideList);
            }
        }
    }

    private ArrayList<String> getMods(String sequence) {
        ArrayList<String> tempList = new ArrayList();
        Pattern check = Pattern.compile("[A-Z][^A-Z]{2,6}");
        Matcher matcher = check.matcher(sequence);
        while (matcher.find()) {
            if (matcher.group().length() != 0) {
                tempList.add(matcher.group());
            }
        }
        return tempList;
    }

    private String removeMod(String toRemove) {
        String temp = "";
        Pattern check = Pattern.compile("[A-Z]");
        Matcher matcher = check.matcher(toRemove);
        while (matcher.find()) {
            if (matcher.group().length() != 0) {
                temp = temp + matcher.group();
            }
        }
        return temp;
    }

    private ArrayList<String> getAllSpectra(File file) throws FileNotFoundException {
        ArrayList<String> allSpectrums = new ArrayList();
        try (BufferedReader rd = new BufferedReader(new FileReader(file))) {
            while (true) {
                String nextLine = rd.readLine();
                if (nextLine == null) {
                    break;
                }
                if (nextLine.contains("TITLE=")) {
                    String spectrumTitle = nextLine.substring(6);
                    allSpectrums.add(spectrumTitle);
                }
            }
        } catch (IOException ex) {
            System.out.println("invalid file");
        }
        return allSpectrums;
    }

    private boolean ValidForMatch(String spectrum) {
        if (peptideSGMap.containsKey(spectrum) && peptideDNGMap.containsKey(spectrum)) {
            return true;
        } else {
            if (peptideSGMap.containsKey(spectrum)) {
                onlyInSG.add(spectrum);
            } else if (peptideDNGMap.containsKey(spectrum)) {
                onlyInDNG.add(spectrum);
            } else {
                noPSM.add(spectrum);
            }
            return false;
        }
    }

    public void compare(File file) throws FileNotFoundException {
        int k = 0;
        int matched = 0;
        int misMatched = 0;
        ArrayList<String> allSpectrums = getAllSpectra(file);
        System.out.println(allSpectrums.size());
        for (String spectrum : allSpectrums) {
            if (ValidForMatch(spectrum)) {
                boolean matchHappend = false;
                ArrayList<Peptide> SGPeptides = peptideSGMap.get(spectrum);
                ArrayList<Peptide> DNGPeptides = peptideDNGMap.get(spectrum);
                for (Peptide SGPeptide : SGPeptides) {
                    for (Peptide DNGPeptide : DNGPeptides) {
                        // !!!! the order of contains makes a 483 matches difrence becous some DNG pep are a substring of SG pep!!!!!!
                        // some spect in SG generate same peptide!!!
                        if (SGPeptide.getSequence().contains(DNGPeptide.getSequence())) {
                            Match Matched = new Match(SGPeptide, DNGPeptide);
                            matchHappend = true;
                            matchList.add(Matched);
                            break;
                        }
                    }
                }
                if (matchHappend) {
                    matched++;
                } else {
                    misMatched++;
                }
            }
        }
        System.out.println("");
    System.out.println("No of matches "+matched);
    System.out.println("No of mismatches "+misMatched);
    System.out.println("ratio "+(double) matched / misMatched);
    System.out.println("matches+mismatches "+ (matched + misMatched));
    System.out.println("");
    System.out.println("non overlapping spectra "+(onlyInDNG.size() + onlyInSG.size() + noPSM.size()));
    System.out.println("total amount of spectra in the mgf "+ (onlyInDNG.size() + onlyInSG.size() + noPSM.size() + matched + misMatched));
    System.out.println("");
    int[][] test = matchList.get(1).getRankList();
    System.out.println("No of rank1 "+test[0][0]);
    System.out.println("No of rank2 "+test[0][3]);
    System.out.println("No of rank3 "+test[0][4]);
    System.out.println("No of rank4 "+test[0][5]);
    System.out.println("No of rank5 "+test[0][6]);
    System.out.println("No of rank6 "+test[0][7]);
    System.out.println("No of rank7 "+test[0][6]);
    System.out.println("No of rank8 "+test[0][7]);
    System.out.println("No of rank9 "+test[0][8]);
    System.out.println("No of rank10 "+test[0][9]);

    }
}

This is what I'm interested in at the end:

output sample

share|improve this question
    
I think this is a good case where comments would come in handy, as the problem domain is not too familiar to many people. Also, break individual classes into their own code blocks so it's clear where one class ends and another begins. I think this would help with your answers, and there's a lot of potential for great ones here. –  lealand Jun 13 at 22:32
    
Sample input would also help you to get complete reviews. –  lealand Jun 13 at 23:38
add comment

2 Answers 2

Your code looks pretty good to me, though admittedly I have a hard time understanding immediately what it's doing. I'll go through line by line and see if that helps my understanding.

Main.java

  • searchguicompare is a pretty awkward package name, but that's a trivial point.

  • Your class searchGUIcompare should be called either SearchGUICompare or SearchGuiCompare to conform with Java naming conventions, though still, I don't know what that class does from the name alone.

    File SGFile = new File("C:\\Users\\Paulo\\Documents\\NetBeansProjects\\searchGUIcompare\\src\\main\\recourses\\Paulo.txt");
    File DNGFile = new File("C:\\Users\\Paulo\\Documents\\NetBeansProjects\\searchGUIcompare\\src\\main\\recourses\\20080311_CPTAC6_07_6A005.mgf.out");
    File mgfFile = new File("C:\\Users\\Paulo\\Documents\\NetBeansProjects\\searchGUIcompare\\src\\main\\recourses\\20080311_CPTAC6_07_6A005.mgf");
    main.readSequencerOutput (SGFile, "sg");
    main.readSequencerOutput (DNGFile, "dng");
    main.compare(mgfFile);
    
  • SGFile and DNGFile should follow Java variable naming conventions: sgFile and dngFile, just like mgfFile.

  • main is a terrible name for your searchGUIcompare variable, as I think we're talking about your Main class, not your searchGUIcompare class. I'd prefer to see SearchGuiCompare searchGuiCompare = new SearchGuiCompare(); or something. Remember, in Java, verbosity is your friend!

  • In the last three lines you appear to be calling static utility functions. We'll see below, but I hope those aren't changing the state of the object. If they aren't, make them static and call at the class level (i.e. searchGUIcompare.readSequencerOutput(SGFile, "sg");.

  • It's also a bad sign that you're passing "sg" and "dng" into the method. These should be constants or, even better, enums.

  • Some people would say that you should pass in the files as arguments on the command line, though that would only be needed if you intended to distribute.

Match.java

private Peptide SGPeptide;
private Peptide DNGPeptide;
private static ArrayList<ArrayList<Integer>> rankList = new ArrayList(Collections.nCopies(10, 0));
private static int[][] rankList2 = new int[10][10];
  • SGPeptide and DNGPeptide are again bad variable names, as they look like class names. Call them sgPeptide and dngPeptide.

  • Why are rankList and rankList2 static? I highly suspect they shouldn't be. You should also use the interface collections classes in your declarations unless you have a reason not to. I.e. List<List<Integer>> rankList.

  • Why do you have rankList and rankList2 in the first place? If they're doing different things, name them appropriately. If they're doing the same thing, why do you have two of them? In fact, is rankList even used?

  • 10 is an arbitrary magic number. Replace all instances with a meaningful constant.

  • rankList2[SGPeptide.getIndex()][DNGPeptide.getIndex()]++; looks like some dangerously opaque coding. Others may disagree, but I would never do this.

  • All these fields can be declared final.

Peptide.java

  • All the field variables can also be declared final.

  • Replace all instances of ArrasList with List.

  • modlist should be named modList. peptideNG is fine in my opinion, though why isn't it called sequence? As I said, I don't know the domain. Are only petideNGs going to be passed into this constructor?

searchGUIcompare.java

  • import java.util.regex.*; is a bad sign to me. Never import a whole package. In Eclipse, use Ctrl+Shift+O to automatically import only what you need.

  • In your field variables, replace ArrayList... = new ArrayList.... with List.... = new ArrayList.... And same for you maps. None of these are parametized too. Here is how it should look (Java 7+):

    private String score;
    private String spectrumNG;
    private List<Match> matchList = new ArrayList<>();
    private List<String> onlyInSG = new ArrayList<>();
    private List<String> onlyInDNG = new ArrayList<>();
    private List<String> noPSM = new ArrayList<>();
    private List<Peptide> peptideList = new ArrayList<>();
    private Map<String, List<Peptide>> peptideSGMap = new HashMap<>();
    private Map<String, List<Peptide>> peptideDNGMap = new HashMap<>();
    

void readSequencerOutput(File file, String SA)

  • What does String SA mean?

  • You have raw strings again. These should be constants, or better, enums. Still don't know what the difference between the two input options are though...

  • This method looks suspiciously like a utility method which could be static.

For the rest of the methods, a lot of stuff is going on which I can't understand without seeing some sample input., so I'll leave it at that. I think my initial suspicion is correct, though, and all the methods in this class can be static.

share|improve this answer
    
I should indeed rename a lot of things as you pointed out. The ranklist1 I forgot to remove when i saw my second version did the task better, it is static as i wanted to keep track with evry new match what the rankins were of all previous maches but now thinking of it i could probably have done this in a better way. i'm still fairly new to programming so thank you for the advise –  user3700660 Jun 13 at 23:52
1  
There's a very limited set of reasons why you would want to make a variable static, outside of it being a constant, so be cautious of that. Good job overall, though. Very competent code, even if I still don't know what it does. I do encourage you to edit your question and provide some sample input, as you'll be more likely to get more helpful responses. Specifically, add the contents of your inputs files to the question. –  lealand Jun 13 at 23:59
    
Note that it doesn't matter if you import the whole package or not: at compiletime it is substituted for only the required dependencies. Using the wildcard will in fact keep the code cleaner in the editor (although it also allows for some annoying cornercases like awt.List and util.List) –  Jeroen Vannevel Jun 14 at 0:50
    
@JeroenVannevel that's a great point that I wasn't too familiar with. I did just encounter that annoying case last week at work when another team introduced a library which conflicted with our ten year old legacy code and broke our CI build. Still no reason to use wildcard imports with modern IDEs though. –  lealand Jun 14 at 1:10
add comment

Program to an interface. For instance, use List<Object> instead of ArrayList<Object>. Check out this question for an explanation.

Make your fields final if possible.

Don't use System.out.println() if you can avoid it.

Provide proper documentation for your methods, especially the public ones. Avoid inline comments, if your methods are properly small and concise, they should explain themselves. Whatever explanation you need to provide, can go in the documentation in front of the method.

Try not to use a while(true) construction.

Your methods should be smaller. Remember, they should only do one thing and not know or care about anything else.

Take care selecting proper, descriptive names for methods. There's for instance a public void compare(File file) method. It doesn't have any documentation and doesn't return anything. I have no idea what it does under the water and no real way of finding out aside from going through its lengthy code.

Additions:

The code in your main method contains some filepaths that fall off the right edge of my screen. I have to scroll right to view all the code, which makes the code less readable. (At least to people with small screens.)

Here and there, you use magical values. See this piece of code for instance:

if (SA.toLowerCase().contains("dng") || SA.toLowerCase().contains("denovogui")) {

I have no idea what the significance of those strings is.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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