CMPUT301W15T12 / C301Project

Apache License 2.0
3 stars 2 forks source link

Test Cases + Data Model/Controller Implementation #42

Closed vanbelle closed 9 years ago

vanbelle commented 9 years ago
IchigoKIl commented 9 years ago

Some suggestions/questions about ClaimList:

Instead of using getUserClaims, getFiltered and getSubmittedClaims to return a new ArrayList after every call (which could be tedious/inefficient for updating a list view) I suggest we make a generic filter method that takes in the arraylist to be modified, username, status and tags to filter, and it will clear the arraylist and add from the claims arraylist claims that satisfy the filter conditions.

What is the reason for having returnClaim and approveClaim? Shouldn't that be handled by the controller?

Should we really have a condition that a claim name should be unique for addClaim? And even so why would removeClaim take in a claim name and not a claim to be removed, since the claim should already be known if claim name were to be extracted?

vanbelle commented 9 years ago

I think that sounds good, except that since claims are never filtered by both users/tags and status, what if we just split them in two functions? One that filters claims by user/tags and one that filters by Submitted status? Either way there is only one call and shouldnt be too bad for a listview. also The filter method would simpler.

Also I would move the returnClaim, approveClaim functions to the claim class. Since all those functions should do is change the status and add the approvers name to the list.

qsjiang commented 9 years ago

Hey Raymond, just a heads up.

I am going to take over ClaimList.java

ClaimList.java is the core components siting between Claim.java and ClaimListController, and I am working on both of them. In order to have cohesive data flow when accessing claims, it's best those three tightly coupled components are written by the same person.

IchigoKIl commented 9 years ago

I have already worked on new ClaimList2, ExpenseList, and List classes and have just pushed it onto git. We can look over the implementation during the meeting today and see if there are any issues, but if not I think we should just use what is done and I'll just add any missing pieces in the next couple of days.

qsjiang commented 9 years ago

There are some major issues inside the ClaimList.java. I took a look at the new code some of those issues still exist.

I have written down a list of the issues I saw, was planning to show you during the meeting, but here you go.

1 Use this.var and ClassName.var when you are addressing private variable and static variable inside the same class Why? Can you spot what's wrong with getUserClaims?

    public class ClaimList implements List<Claim>{
        private static ArrayList<Claim> claims;
        private static ArrayList<Listener> listeners;
        private static int counter;

        public ArrayList<Claim> getUserClaims(String Username) {
            ArrayList<Claim> claims = new ArrayList<Claim>();
            for (int i = 0; i < claims.size(); i++) {
                if (claims.get(i).getClaimant().getUserName().equals(Username)) {
                    claims.add(claims.get(i));
                }
            }
            return claims;
        }
    ...

2 Problem with getter and setter. not all variables need a getter and setter, especially the private variables the class needs to protect.

    public int getCounter(){
        return counter;
    }

    public void setCounter(int id_counter){
        counter = id_counter;
    }

3 Pass user object instead of String Username Why? You can redefined the meaning of equals later.

    public ArrayList<Claim> getUserClaims(String Username) {
        ...
    }
qsjiang commented 9 years ago

Although there are problems, but that is not the reason why I am asking to take over the ClaimList.java ClaimList.java is core components connecting all my other of components. It makes no sense to assign this to someone else, this is why I ask for both Claim.java ClaimList.java and ClaimListController.java in the first place.

Your effort is not going to go be wasted, I am going to build on top of your code instead of start over, but I have to take it over from here.

IchigoKIl commented 9 years ago

I don't believe you are looking at the right code. I pushed a new ClaimList2 which extends List2 and uses a singleton pattern.

qsjiang commented 9 years ago

yes I saw it. there are still some problems with the getter and setter

qsjiang commented 9 years ago

but that doesn't matter. the reason I need to work on the claimlist.java is simply because it is closely coupled with my code, and it makes more sense to reassigned the responsibility