I'll start off by saying this is actually really good. There are a few minor [PEP8][0] errors: - In `after_login` you call `Request`. the argument `url` shouldn't have a space on the right side of the equals. - Another thing in `Request`. The argument `callback` needs at least one more indent. However you should either _aligned with opening delimiter_ or use _hanging indents_. And so you should change this to either of: yield Request(url=baseurl + page + "?id=1", callback=self.action) yield Request( url=baseurl + page + "?id=1", callback=self.action) - Variables and functions should be named in `snake_case`. `pagelist` should be `page_list`, `pageurl` `page_url`, etc. Whilst not being a PEP8 error you may want to change `page_list` to say `pages`. This is as multiple pages are probably a list, and sounds more fluid. I say the above as consistency is the best way to allow others to easily read your work. But they are quite minor. Personally I'd move the `baseurl` out of the class and make it a constant. This allows you to easily change the url, say the company you're scraping change their company/domain name or their top-level domain. I'd also move other system settings, `'UserNameHere'`, `'PasswordHere'`, `'http://www.example.com'`. I may even say that `pagelist` should be moved out too. Now for comments on your business logic, as `parse` and `after_login` are very short 3 line functions, there's honestly not much to say about them. `action` however has a few problems: - [`response.url` is a string][1] and so you shouldn't cast it to one. This confused me as I it made me think `response.url` would be something that isn't a string. - You seem to be getting the `response` as an instance of `TextResponse`, which contains [`selector`][2]. This also contains the shortcut `xpath` which is equivalent to `selector.xpath`. Which allows you to reduce the size of that large command. - Unfortunately scrapy doesn't document `extract_first`, but looking at the source they use `Parsel`. The [documentation on `extract_first`][3] was also kinda lacking, so I read the source code again, and it seems like [they always return strings][4]. So you probably don't need the `str` around `extract_first` either. - You don't define `pagename` unless `page` is not `None`. But you go on to use it regardless. This is wrong, and can lead to errors. There are two ways to come at this, either silently fail. Or raise an exception, where `NameError: name 'pagename' is not defined` is not a good error. - Finally it's strange to see `yield` rather than `return`, you're only going to ever return one thing. And so unless it's a requirement imposed by scrapy I'd change it to `return`. And so if I were to improve it, leaving `yield` and failing silently, I'd end with: BASE_URL = 'http://www.example.com' USER_NAME = 'UserNameHere' PASSWORD = 'PasswordHere' PAGES = ['page1.aspx', 'page2.aspx', 'page3.aspx', 'page4.aspx'] class ShareSpider(scrapy.Spider): name = "sharespider" start_urls = [BASE_URL + '/public/login.aspx'] def parse(self, response): yield scrapy.FormRequest.from_response( response, formxpath='//form[@id="login"]', formdata={ 'UserName': USER_NAME, 'Password': PASSWORD, 'Action':'1', }, callback=self.after_login) def after_login(self, response): base_url = BASE_URL + '/public/' for page in PAGES: yield Request( url=base_url + page + "?id=1", callback=self.action) def action(self, response): page = re.search('public/(.*)id=1', response.url) if page: page_name = page.group(1) title = response.xpath('//title/text()').extract_first().strip() item = PageItem() item['pagename'] = page_name item['description'] = title yield item [0]: https://www.python.org/dev/peps/pep-0008/#indentation [1]: http://doc.scrapy.org/en/latest/topics/request-response.html?highlight=request#scrapy.http.Response.url [2]: http://doc.scrapy.org/en/latest/topics/request-response.html?highlight=request#scrapy.http.TextResponse.selector [3]: https://parsel.readthedocs.io/en/latest/usage.html#using-selectors [4]: https://github.com/scrapy/parsel/blob/master/parsel/selector.py#L218