3
\$\begingroup\$

Objective: Develop an SEO analysis tool, for a website built in Django, that allows users to input a URL and receive an SEO report containing information about the page's optimization strengths and weaknesses.

Using: Python 3.10 and Django 4.1.1

Difficulty: Design. The code works, but unfortunately I don't know whether I've taken the most professional approach. At times, I feel I make my code too convoluted, and miss more elegant solutions. Am I using dataclasses correctly? Is the code hard to read? Given the complexity and importance of this app, I want to make sure I'm on the right track before getting too deep. So I look forward to your feedback, thanks!

seo_dataclasses.py
Store SEO reports for easy access and readability. Future: add WebsiteReport class that stores a list of page results for the given website. Save website reports to db.

@dataclass
class SEOCheckResult:
    error: bool
    check_failed_desc: Optional[str] = None
    check_passed_desc: Optional[str] = None
    solution: Optional[str] = None
    code: Optional[str] = None

@dataclass
class SEOChecks:
    category: str
    category_results: List[SEOCheckResult] = field(default_factory=list)

@dataclass
class PageReport:
    soup: BeautifulSoup
    domain: str
    url: str
    keyword: str
    page_results: List[SEOChecks] = field(default_factory=list)

    def generate_results(self):
        seo_check_functions = [
            analyze_h_tag
            # etc
        ]
        for seo_func in seo_check_functions:
            self.page_results.append(seo_func(self.soup, self.keyword))

seo_checks.py
Contains all functions associated with assessing a page's SEO.

def analyze_h_tag(soup, keyword):
    """
    Analyze h1 tags.
    Counts the number of h1 tags on a page.
    Ensures proper syntax is being used.
    """
    h1 = soup.find_all("h1")
    h1_results = SEOChecks(category="H1 Tags")

    if not h1:
        h1_results.category_results.append(
            SEOCheckResult(
                error=True,
                check_failed_desc="Missing H1 tag.",
                solution="Implement an H1 tag.",
            )
        )
        return h1_results
    else:
        h1_results.category_results.append(
            SEOCheckResult(error=False, check_passed_desc="H1 tag found.")
        )

    # Check number of H1 tags in use
    if len(h1) > 1:
        for h1_tag in h1[1:]:
            h1_results.category_results.append(
                SEOCheckResult(
                    error=True,
                    check_failed_desc="Multiple H1 tags found.",
                    solution="Reduce to one H1 tag.",
                )
            )
    else:
        h1_results.category_results.append(
            SEOCheckResult(
                error=False, check_passed_desc="Single H1 tag present."
            )
        )

    # Get the first h1 tag, all further analysis will be on it alone.
    main_heading = h1[0].text
    lower_heading = main_heading.lower()

    # Check keyword usage
    if keyword not in lower_heading:
        h1_results.category_results.append(
            SEOCheckResult(
                error=True,
                check_failed_desc="Keyword not present in H1 tag.",
                solution="Add keyword.",
            )
        )
    else:
        h1_results.category_results.append(
            SEOCheckResult(
                error=False, check_passed_desc="Keyword present in H1 tag."
            )
        )
        heading_split = lower_heading.split()
        if (
            keyword not in heading_split[: math.trunc(len(main_heading) / 2)]
        ):  # Must be present in the first half
            h1_results.category_results.append(
                SEOCheckResult(
                    error=True,
                    check_failed_desc="Keyword priority.",
                    solution="Move keyword forward.",
                )
            )
        else:
            h1_results.category_results.append(
                SEOCheckResult(
                    error=False, check_passed_desc="Correct keyword placement."
                )
            )
    return h1_results

def analyze_meta_title(soup, keyword):
        pass

def analyze_meta_description(soup, keyword):
        pass

# etc

crawler.py
Crawl page or crawl site. Establish response. Future: Improve security. Limit user to x number of requests per day.

class Crawler:
    def __init__(self, url, keyword):
        self.conn_error = None
        self.url = url if urlsplit(url).scheme else f"http://{url}"
        self.parse = urlsplit(self.url)
        self.domain = self.parse.netloc
        self.request = self._get_request(self.url)
        self.keyword = keyword
        self.soup = (
            BeautifulSoup(self.request.text, "lxml") if self.request else None
        )

    def _get_request(self, url):
        try:
            response = requests.get(url, timeout=40)
            response.raise_for_status()
            return response
        except (
            requests.exceptions.ConnectionError,
            requests.exceptions.Timeout,
            requests.exceptions.RequestException,
        ) as e:
            self.conn_error = str(e)
            return None

    def crawl_page(self):
        page = PageReport(
            soup=self.soup,
            domain=self.domain,
            url=self.url,
            keyword=self.keyword,
        )
        page.generate_results()
        return page.page_results

    def crawl_site(self):
        pass

views.py

def seo_tools(request):
    if request.method == "POST":
        website = request.POST["website"]
        keyword = request.POST["keyword"]

        page = Crawler(url=website, keyword=keyword)
        if page.conn_error:
            messages.error(request, page.conn_error)
            return render(request, "freetools/seo-tools.html")

        results = page.crawl_page()
        context = {"analysis": results}

        return render(request, "freetools/seo-tools.html", context)
    else:
        return render(request, "freetools/seo-tools.html")
\$\endgroup\$
1

2 Answers 2

2
\$\begingroup\$

EOL

Using: Python 3.10

Ummm, it's unclear what business constraints would motivate one to write new code using an interpreter that passed one of its EOL milestones more than a year ago. It no longer gets full support for bugfixes, though it will get security fixes for the next two years.

Conda would be happy to supply your venv with a modern interpreter.

Django 4.1 mainstream support has expired. And the final date for extended support passed last year. You may be happier relying on 4.2 LTS.

DAO

Making SEOCheckResult a @dataclass was an excellent design choice, thank you. It's not clear that all four VARCHAR attributes need to admit of NULL. In particular, check_{failed,passed}_desc feel like they're the same attribute, with a bool column missing. Or maybe they should be of type tuple[bool, str].

Perhaps we want a distinct return type with a check_failed_desc attribute.

serializable type

This surprises me a bit:

class PageReport:
    soup: BeautifulSoup ...

I was kind of expecting html, which is the simplest way to serialize some soup out to the database.

I definitely do like that soup is passed in to various analysis functions. I was just expecting that we might @lru_cache the HTML --> soup relation.

comments lie!

Sometimes.

def analyze_h_tag(soup, keyword):
    """
    Analyze h1 tags. ...

Or maybe the identifier lies. My first reading was we'd analyze h1 .. h6. But then the docstring suggests otherwise.

Recommend you harmonize them.

magic number

There's no motivation disclosed in # comments or """docstrings""" for why 1 is a special number:

    if len(h1) > 1:

Apparently there is some Specification describing what a well-formed SEO web page looks like, but we've not seen any mention of it.

alternate SEO formats

For example, the page you're currently viewing has an <h2>alternate ... tag, and decidedly more than a single <h1> tag:

  • h1 SEO Page Analysis Tool ...
  • h1 EOL
  • h1 DAO
  • h1 serializable type
  • ...

extract helper

This comment was omitted, but we can easily imagine it:

    ... # Check for missing H1 tag

And then we see:

    # Check number of H1 tags in use
    ...
    # Check keyword usage

Your instinct to explain "Here comes some stuff!" is a good one. But when tempted to write a # foo comment, take a step back and consider whether you should really be breaking out a def foo(): helper.

FP slice index

Well, color me surprised!

    ... heading_split[: math.trunc(len(main_heading) / 2)]       

If there's some reason we didn't just take int( ... ) of that quantity, I'm not seeing it.

Also, why did we use / 2 to construct a FP result in the first place? Wouldn't a // 2 integer be a more convenient slice index?

non-code

def analyze_meta_title(soup, keyword):
        ...
def analyze_meta_description(soup, keyword):

These don't make any sense in the current submission.

Please elide.

complex ctor

This perhaps does more than is desirable.

class Crawler:
    def __init__(self, url, keyword):
        ...
        self.request = self._get_request(self.url)

Consider deferring the web GET request until caller demands it. The whole business of self.soup maybe being available is troublesome. It would be nicer to enforce stricter class invariants, as that makes it easier to reason about the objects we've created.

exception attributes

This regrettably strips some useful information, such as source line numbers related to the exception and any possible predecessor (wrapped) exceptions.

            self.conn_error = str(e)

Consider simply assigning e instead. Callers can choose to invoke str() if they wish to.

Consider doing raise e. As written, caller is burdened with None checks.

context=None

It's unclear why this is helpful:

        return render(request, "freetools/seo-tools.html", context)
    else:
        return render(request, "freetools/seo-tools.html")

Just default context to None. And then if we later can fill it in with a dict value, so much the better. But pass along its value always, whether it's defaulted or not.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ And 1 is not magic enough! There should be at most one h1 per page for SEO, because semantically h1 means "page title". Extracting MAX_H1_TAGS_PER_PAGE would be no less questionable than using 1 directly, IMO. (though a comment would be helpful, indeed) \$\endgroup\$
    – STerliakov
    Commented Jul 9 at 12:02
  • \$\begingroup\$ Thank you, I appreciate the great advice @J_H. I've updated my post with a refactor based on your suggestions. There are still a couple of issues I haven't addressed yet. However, I'm now storing the html, and generating the soup through a cache and property. I had to set eq to false to make the PageReport class hashable for the lru_cache, but I don't see that being an issue. I eliminated the redundant description attributes. I moved response/connection logic to its own class. However, I'm not sure whether I'm handling associated exceptions in the best manner possible. \$\endgroup\$ Commented Jul 11 at 1:13
  • \$\begingroup\$ I'm still experimenting with raise e instead of returning None. But when doing so, I'm not sure what should be done in the caller. Still researching. \$\endgroup\$ Commented Jul 11 at 1:16
2
\$\begingroup\$

REFACTOR based on feedback:

@dataclass
class SEOCheckResult:
    error: bool
    description: str
    solution: Optional[str] = None
    code: Optional[str] = None

@dataclass
class SEOChecks:
    category: str
    category_results: List[SEOCheckResult] = field(default_factory=list)

@dataclass(eq=False)
class PageReport:
    html: str
    domain: str
    url: str
    keyword: str
    page_results: List[SEOChecks] = field(default_factory=list)

    def generate_results(self):
        seo_check_functions = [
            analyze_h_tag
        ]
        for seo_func in seo_check_functions:
            self.page_results.append(seo_func(self.soup, self.keyword))

    @property
    @lru_cache(maxsize=None)
    def soup(self):
        return BeautifulSoup(self.html, "lxml")
from .http import http_client


class Crawler:
    def __init__(self, url, keyword):
        self.conn_error = None
        self.url = url if urlsplit(url).scheme else f"http://{url}"
        self.parse = urlsplit(self.url)
        self.domain = self.parse.netloc
        self.keyword = keyword

    def _html(self):
        try:
            response = http_client.get(self.url)
            return response.text
        except (
            requests.exceptions.HTTPError,
            requests.exceptions.ConnectionError,
            requests.exceptions.Timeout,
            requests.exceptions.RequestException,
        ) as e:
            self.conn_error = e
            return None

    def crawl_page(self):
        html = self._html()
        if html is None:
            return None

        page = PageReport(
            html=html,
            domain=self.domain,
            url=self.url,
            keyword=self.keyword,
        )
        page.generate_results()
        return page.page_results
class HttpClient:
    def __init__(self):
        self.session = requests.Session()

    def get(self, url):
        r = self.session.get(url, timeout=15)
        r.raise_for_status()
        return r

http_client = HttpClient()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.