HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Comparing 2 lists of peptide to spectrum rankings generated by 2 different algorithms

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
generatedcomparingspectrumpeptidealgorithmsrankingsdifferentlists

Problem

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.

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()]++

Solution

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> 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> 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 matchList = new ArrayList<>();
private List onlyInSG = new ArrayList<>();
private List onlyInDNG = new ArrayList<>();
private List noPSM = new ArrayList<>();
private List peptideList = new ArrayList<>();
private Map> peptideSGMap = new HashMap<>();
private Map> 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.

Code Snippets

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);
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];
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<>();

Context

StackExchange Code Review Q#54199, answer score: 6

Revisions (0)

No revisions yet.