widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Added some diff comments.

Diff comments:

> === modified file 'src/logic/productionsite.cc'
> --- src/logic/productionsite.cc	2015-11-11 09:53:54 +0000
> +++ src/logic/productionsite.cc	2015-11-21 19:58:38 +0000
> @@ -284,33 +284,56 @@
>  }
>  
>  /**
> - * Detect if the workers are experienced enough for an upgrade
> + * Detect if the workers are experienced enough for an target building
> + * Buildable workers are skipped, but upgraded ones (required be target site) are tested
>   * @param idx Index of the enhancement
>   */
>  bool ProductionSite::has_workers(DescriptionIndex targetSite, Game & /* game */)
>  {
>  	// bld holds the description of the building we want to have
>  	if (upcast(ProductionSiteDescr const, bld, owner().tribe().get_building_descr(targetSite))) {
> -		// if he has workers
> +
>  		if (bld->nr_working_positions()) {
> -			DescriptionIndex need = bld->working_positions()[0].first;
> -			for (unsigned int i = 0; i < descr().nr_working_positions(); ++i) {
> -				if (!working_positions()[i].worker) {
> -					return false; // no one is in this house
> -				} else {
> -					DescriptionIndex have = working_positions()[i].worker->descr().worker_index();
> -					if (owner().tribe().get_worker_descr(have)->can_act_as(need)) {
> -						return true; // he found a lead worker
> +
> +			// Iterating over workers positions in target building
> +			for (const auto& wp : bld->working_positions()) {
> +
> +				// If worker for this position is buildable, just skip him
> +				if (owner().tribe().get_worker_descr(wp.first)->is_buildable()){
> +					continue;
> +				}
> +
> +				// This position needs promoted worker, so trying to find out if there is such worker
> +				// currently available in this site
> +				const DescriptionIndex needed_worker = wp.first;
> +				bool worker_available =  false;
> +				for (unsigned int i = 0; i < descr().nr_working_positions(); ++i) {
> +					if (working_positions()[i].worker) {

You are getting working_positions()[i] twice here. Why not just iterate over working_positions() with a range-based for loop?

> +						DescriptionIndex current_worker
> +							= working_positions()[i].worker->descr().worker_index();
> +						if (owner().tribe().get_worker_descr(current_worker)->can_act_as(needed_worker)) {
> +							worker_available = true; // We found a worker for the position

Add a "break" statement to leave the inner loop - no need to check further, we have already found one.

> +						}
>  					}
>  				}
> +				if (!worker_available) {
> +					// We dont have needed workers in the site :(
> +					return false;
> +				}
> +
>  			}
> -			return false;
> +
> +			//if we are here, all needs are satisfied
> +			return true;
> +
> +		} else {
> +			throw wexception("Building, index: %d, needs no workers!\n", targetSite);
>  		}
> -		return true;
> -	} else return true;
> +	} else { //NOCOM

Remove the NOCOM unless you have a question.

> +		throw wexception("No such building, index: %d\n", targetSite);
> +	}
>  }
>  
> -
>  WaresQueue & ProductionSite::waresqueue(DescriptionIndex const wi) {
>  	for (WaresQueue * ip_queue : m_input_queues) {
>  		if (ip_queue->get_ware() == wi) {


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1518223/+merge/278249
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1518223 into lp:widelands.