Collection size 1,100 exceeds maximum size of 1,000. Limited my query to 100 items but still getting the error

Question

public class Checkbox_Class
{

    List<assetswrapper> assetsList = new List<assetswrapper>(); 
    List<Asset> selectedAssets = new List<Asset>();
    Integer x;

    public List<assetswrapper> getAssets() 
    {



        for(Asset a : [Select //Select Asset name, Serial Number, Quantity and whether it's been RMAed from Assets
                       SerialNumber, Name, Quantity, Returned__c, Retired__c, RMAed__c 
                        FROM Asset
                        WHERE (Retired__c != true AND Returned__c != true) 
                        limit 100] )     Limit the amount of returned Assets to 100 because of SOQL 1000 query limit.



        assetsList.add(new assetswrapper(a)); //as each asset is processed, we create a new assetswrapper object and add it to the assetslist
        return assetsList; // return the assetsList list when called. 
    }



    public PageReference getSelected()
    {//We create a new list of Assets that we populate only with Assets if they get selected
        selectedAssets.clear(); //
       //cycle through assetswrapper accwrapper list and we will check to see if the selected property is set to true, 
       //if it is, we add the Asset to the selectedAssets list. 
        for(assetswrapper accwrapper : assetsList)
        if(accwrapper.selected == true)
        selectedAssets.add(accwrapper.acc);
        return null;
    }

     public void getSelected2()
    {//same as the above method. Except we just want to return an integer value here to see how many assets were selected. 
       //selectedAssets.clear(); //
        for(assetswrapper accwrapper : assetsList)
            if(accwrapper.selected == true)
                selectedAssets.add(accwrapper.acc);        
        x = selectedAssets.size();        
    }


    public Integer GetSelectedAssets()
    {
        if(selectedAssets.size()>0){
        x = selectedAssets.size();
         return x;
        }
        //return selectedAssets;

        else
        return null;
    }   


    //The wrapper/container class. Wrapper contains both the Asset object and a boolean value. 
    public class assetswrapper
    {
        public Asset acc{get; set;}
        public Boolean selected {get; set;}
       //When we create a new assetswrapper object here, we pass an asset. Selected value is also set to False. 
        public assetswrapper(Asset a)
        {
            acc = a;
            selected = false;
        }
    }
}

The above is the code which is almost exactly similar to the create a checkbox datatable wiki on developer force: http://wiki.developerforce.com/page/Checkbox_in_DataTable

I am not trying to show 1000 items on a list. In fact, I've limited the list to only show 100 items max. However, I still get that collection size error every time I select 20+ items on that checkbox datatable. Any clues as to where I'm going wrong in the code? I see that this is a duplicate question but the other similar questions did not have the solution that I was looking for:

VISUALFORCE CODE:

    <apex:page controller="Checkbox_Class" Tabstyle="Asset">
    <apex:form > 
        <apex:pageBlock Title="Assets with CheckBoxes">
            <div style="overflow:auto; width:500px; height:500px">  
                <apex:pageBlockSection Title="List of Available Assets">
                    <apex:dataTable value="{!assets}" var="a" columnswidth="50px,50px" cellpadding="4" border="1">
                        <!--makes a call to the getAssets() method in the Checkbox_Class Controller-->
                           <apex:column >
                                    <apex:facet name="header">
                                        <apex:inputCheckbox>
                                            <apex:actionSupport event="onclick" onsubmit="checkAll(this,'checkedone')"/>
                                        </apex:inputCheckbox>
                                    </apex:facet>
                            <apex:inputCheckbox value="{!a.selected}" id="checkedone"></apex:inputCheckbox>
                            </apex:column>
                            <apex:column headervalue="Asset Name" value="{!a.acc.Name}" />
                            <apex:column headervalue="Serial Number" value="{!a.acc.SerialNumber}" />
                            <apex:column headervalue="Quantity" value="{!a.acc.Quantity}" />    
                            <apex:column headervalue="RMAed" value="{!a.acc.RMAed__c}" />
                    </apex:dataTable>   
                </apex:pageBlockSection>
           </div>
                <apex:pageBlockSection Title="Selected Assets" >
                    <apex:commandButton action="{!getSelected}" value="Create Quote"/>
                    <apex:outputText value="{!SelectedAssets}"/>
                </apex:pageBlockSection>                
        </apex:pageBlock>
    </apex:form>

<script>
function checkAll(cb)
{
var inputElem = document.getElementsByTagName("input");
for(var i=0; i<inputElem.length; i++)
{
    if(inputElem[i].id.indexOf("checkedone")!=-1)
inputElem[i].checked = cb.checked;
}
}   
</script>

</apex:page>

Answer 1

The problem is that the action function is querying data and piling the data onto the list each time it is called. This is an inherent risk factor with queries inside of getter or setter methods. It would be more appropriate to simply expose the list of records to the user directly. I've wrote an essentially same version (virtually cut-and-copy) of what you've done with the class, included below.

Revision

public with sharing class Checkbox_Class {
    // The list of assets we're viewing
    public AssetsWrapper[] assets { get; set; }
    // The total count of assets selected
    public Integer selectedAssets { get; set; }

    // Initialize our data here
    public Checkbox_Class() {
        assets = new AssetWrapper[0];
        selectedAssets = null;
        refreshAssetList();
    }

    // Get the currently selected assets and update counter
    Asset[] getSelectedAssetList() {
        Asset[] results = new Asset[0];
        for(AssetWrapper item: assets) {
            if(item.selected) {
                results.add(item.acc);
            }
        }
        selectedAssets = results.isEmpty()? null: results.size();
        return results;
    }

    // Assumption: We'll be returning a page reference somewhere later.
    public PageReference getSelected() {
        getSelectedAssetList();
        return null;
    }

    // Query the database for a list of assets meeting the criteria
    public void refreshAssetList() {
        Map<Id, Asset> selectedAssets = new Map<Id, Asset>(getSelectedAssetList());
        assets.clear();
        for(Asset record: [SELECT   Id, SerialNumber, Name, Retired__c, Returned__c, Quantity, RMAed__c
                           FROM     Asset
                           WHERE    Retired__c = false AND Returned__c = false
                           ORDER BY CreatedDate DESC    
                           LIMIT    1000]) {
            assets.add(new AssetsWrapper(
                selectedAssets.containsKey(record.Id)?selectedAssets.get(record.Id):record,
                selectedAssets.containsKey(record.Id)
            ));
        }
    }

    // Wrapper class
    public class AssetsWrapper {
        public Asset acc { get; set; }
        public Boolean selected { get; set; }
        public AssetsWrapper(Asset record, Boolean isChecked) {
            acc = record;
            selected = isChecked;
        }
    }
}

Helpful Tips

Getters and setters may be called more than once per page transaction, and in particular, the getAssets function originally posted kept piling on records to the master list 100 at a time (duplicates). This is why the error occurred at exactly 1100 records. Instead, initialize lists only once, and refer to them as necessary.

The refresh function provided here could be linked to a button to refresh the list from scratch (e.g. in the event new assets were added), while still retaining the old checkboxes, if any. This is generally useful especially if the users need updated information.

In general, avoid returning a pagereference if the only possible value is null. In longer functions, this helps visually identify the difference between an action function that will redirect to a new page versus one that will never redirect to a new page. I left that function as is under the assumption that "Create Quote" would probably go to a new page at some point.

Don't use member variables when local variables and parameters will suffice. Each member variable consumes memory, which in turn uses view state (unless it is transient, which is a useful, but often confusing language feature). You don't want your view state getting any larger than necessary, because it slows down loading times and can eventually run afoul of the governor limits.

You can make the page "Read Only" if you want to increase your limits from 1000 items in a list to 10000. This only works if you plan on not creating any data on this page.

You could have used a StandardSetController to emulate all of this functionality, with a maximum query of 2000 records deep, and offer pagination (first, next, last, previous pages), and checkbox capabilities.

Try to avoid calling a variable "x", unless it's in a local loop somewhere that won't be used outside that area. A member variable of "x" is always a bad idea, because it's not apparent what "x" is. Is it the X in an X/Y coordinate? It is a location on the screen? Is it just a simple counter? Does it contain the answer to life, the universe, and everything? Obviously, in this code, it was easy to figure out, but a 10000 line class would be a different story, or even worse, a class that is later subclassed and that subclass uses X to communicate to functions in the main class.