Return IEnumerable from a private method; use Dictionary or anon. type? Is it okay to return IEnumerable<KeyValuePair<string, string>> from a private method instead of returning a read-only dictionary? This allows for shorter/simpler code.

Note: In the below code, PropertyDictionary is a static property that returns an IReadOnlyDictionary<string, PropertyInfo> that was created through reflecting over the MyClass class looking for public, non-static, string-returning properties bearing a particular attribute. Also, the instance method that calls GetPropertyValues does some formatting, immediately changing the result into an IEnumerable<string>, so the dictionary aspect isn't needed for long.

private static IEnumerable<KeyValuePair<string, string>> GetPropertyValues(MyClass myClass) {
   return
      PropertyDictionary.ToDictionary(
         kvp => kvp.Key,
         kvp => (string) kvp.Value.GetValue(myClass, null)
      )
      .Where(kvp => !string.IsNullOrEmpty(kvp.Value));
}

vs.

private static IReadOnlyDictionary<string, string> GetPropertyValues(MyClass myClass) {
   return
      new ReadOnlyDictionary(
         PropertyDictionary.ToDictionary(
            kvp => kvp.Key,
            kvp => (string) kvp.Value.GetValue(myClass, null)
         )
         .Where(kvp => !string.IsNullOrEmpty(kvp.Value))
         .ToDictionary(kvp => kvp.Key, kvp => kvp.Value)
      );
}

This converts to a dictionary twice. I wish there were a way to do Where and Select at the same time! Perhaps there is a better way. Can you suggest one?

Also, should I use an anonymous type instead of a dictionary for the intermediate step?

private static IEnumerable<KeyValuePair<string, string>> GetPropertyValues(MyClass myClass) {
   return
      PropertyDictionary.Select(
         kvp => new {
            key = kvp.Key,
            value = (string) kvp.Value.GetValue(myClass, null)
         }
      )
      .Where(o => !string.IsNullOrEmpty(o.Value));
    // But now this isn't returning a Dictionary, but an anonymous object,
    // so this isn't going to work
}

and

private static IReadOnlyDictionary<string, string> GetPropertyValues(MyClass myClass) {
   return
      new ReadOnlyDictionary(
         PropertyDictionary.Select(
            kvp => new {
               key = kvp.Key,
               value = (string) kvp.Value.GetValue(myClass, null)
            }
         )
         .Where(o => !string.IsNullOrEmpty(o.value))
         .ToDictionary(o => o.key, o => o.value)
      );
}

Are either of these substantially superior?

Of course, all comments to improve quality are welcome.

A peripheral note on the Reflection shown here

Please stay focused on the requested aspects of the code review. For informational purposes: there are a number of properties in MyClass that have a special label accompanying them. This code is expected to be changed over time with new properties being added. Instead of the labels being associated with each property via a list or procedurally, and being far away in the code from the properties themselves, it seems best to me that each property simply be annotated via an attribute (right next to the property!) stating the property's key for serialization. Getting that attribute takes reflection. The PropertyInfo collection is materialized only once, and then used repeatedly at serialization time. If there is a performance problem, which I don't anticipate, I will use Reflection.Emit to solve it, because I think the value of the label-bearing attribute is high and leads to better and easier to maintain code.

I think the cleanest way to optimize it and to use only one loop is to first get the values with a select, create key/value pairs there and then filter by value:

private static IEnumerable<KeyValuePair<string, string>> GetPropertyValues(MyClass myClass) {
   return
        PropertyDictionary
        .Select(kvp =>
            new KeyValuePair<string, string>
            (
                kvp.Key,
                (string)kvp.Value.GetValue(myClass, null)
            )
        )
        .Where(kvp => !string.IsNullOrEmpty(kvp.Value));
}


Flatten dictionary in Python (functional style) I'm trying to learn how to write functional code with Python and have found some tutorials online. Please note that I know Python is not a promoter for functional programming. I just want to try it out. One tutorial in particular gives this as an exercise:

  Write a function flatten_dict to flatten a nested dictionary by
  joining the keys with . character.

So I decided to give it a try. Here is what I have and it works fine:

def flatten_dict(d, result={}, prv_keys=[]):

    for k, v in d.iteritems():
        if isinstance(v, dict):
            flatten_dict(v, result, prv_keys + [k])
        else:
            result['.'.join(prv_keys + [k])] = v

return result

I'd like to know whether this is the best way to solve the problem in python. In particular, I really don't like to pass a list of previous keys to the recursive call.

Your solution really isn't at all functional. You should return a flattened dict and then merge that into your current dictionary. You should also not modify the dictionary, instead create it with all the values it should have. Here is my approach:

def flatten_dict(d):
    def items():
        for key, value in d.items():
            if isinstance(value, dict):
                for subkey, subvalue in flatten_dict(value).items():
                    yield key + "." + subkey, subvalue
            else:
                yield key, value

    return dict(items())

Alternative which avoids yield

def flatten_dict(d):
    def expand(key, value):
        if isinstance(value, dict):
            return [ (key + '.' + k, v) for k, v in flatten_dict(value).items() ]
        else:
            return [ (key, value) ]

    items = [ item for k, v in d.items() for item in expand(k, v) ]

    return dict(items)


Merging IEnumerable and removing duplicates I have a method that exacts data from an XML and returns an IEnumerable<Foo>.  I am trying to create a method that will merge the results from 2 different XML's files into a single IEnumerable and remove any duplicate data that may be found

A duplicate in this case is defined as 2 Foo objects that have the same Name property even if the other data is different.  If there is a duplicate, I always want to keep the Foo from the first IEnumerable and discard the 2nd.

Am I on the right track with this LINQ query, or is there a better way to accomplish what I am trying to do?

public IEnumerable<Foo> MergeElements(XElement element1, XElement element2)
{
    IEnumerable<Foo> firstFoos = GetXmlData(element1); // get and parse first set
    IEnumerable<Foo> secondFoos = GetXmlData(element2); // get and parse second set

    var result = firstFoos.Concat(secondFoos)
                          .GroupBy(foo => foo.Name)
                          .Select(grp => grp.First());

    return result;
}

The biggest concern I have with this code is that I am not certain the duplicate filtering rules I want will be guaranteed.  I know Concat will just append secondFoos to the end of firstFoos, but when calling GroupBy(), will the resulting IGrouping object always have elements in the same order as the source data?

With things like this, it's usually best to look at the documentation.

Is your approach with GroupBy() going to work? Yes:

  Elements in a grouping are yielded in the order they appear in source.

Can you use Distinct() to do the same thing more concisely? No (at least you shouldn't depend on it):

  The Distinct method returns an unordered sequence that contains no duplicate values.

Is there some other way? Yes, you can use Union():

  When the object returned by this method is enumerated, Union enumerates first and second in that order and yields each element that has not already been yielded.

So, I think the best way to do it is:

var result = firstFoos.Union(secondFoos);


My Calendar generation code is slow. How do I improve it? I have a model I have created to generate iCal and the necessary JSON for the jQuery calendar plugin Full Calendar to work. When I display the page, the JSON generation takes about 7 seconds. The query itself has been running in only a few microseconds. So I know my code is the problem.

I was trying to get the format like what Full Calendar expects thus why I wrote the code how I did. My guess is that the below is the problem. Any thoughts on where I can improve this?

#region Event
public class Event
{
    public string Title { set; get; }
    public string Description { set; get; }
    public string URL { set; get; }
    public string UID { set; get; }
    public DateTime DateTimeStamp { set; get; }
    public DateTime CreatedDateTime { set; get; }
    public DateTime Start { set; get; }
    public DateTime End { set; get; }
    public bool AllDay { set; get; }
}
#endregion

public class Calendar
{
    #region Model
    public string CalendarName { set; get; }
    public string Product { set; get; }
    public string TimeZone { set; get; }
    public List<Event> Events { private set; get; }
    #endregion

    /// <summary>
    /// Creates a calendar json string.
    /// </summary>
    /// <returns></returns>
    public string ToFullCalendarJsonString()
    {
        StringBuilder sb = new StringBuilder();

        sb.Append("[");

        int count = Events.Count;
        int i = 0;

        foreach (Event eventItem in Events)
        {
            sb.Append("{");
            sb.AppendFormat("\"title\":\"{0}\",", eventItem.Title);
            sb.AppendFormat("\"allDay\":\"{0}\",", eventItem.AllDay);

            if (eventItem.AllDay)
            {
                sb.AppendFormat("\"start\":\"{0}T00:00:00\",", eventItem.Start.ToString("yyyy-MM-dd"));
                sb.AppendFormat("\"end\":\"{0}T00:00:00\",", eventItem.End.ToString("yyyy-MM-dd"));
            }
            else
            {
                sb.AppendFormat("\"start\":\"{0}T{1}\",", eventItem.Start.ToString("yyyy-MM-dd"), eventItem.Start.ToString("HH:mm:ss"));
                sb.AppendFormat("\"end\":\"{0}T{1}\",", eventItem.End.ToString("yyyy-MM-dd"), eventItem.End.ToString("HH:mm:ss"));
            }
            sb.AppendFormat("\"url\":\"{0}\"", eventItem.URL);
            sb.Append("}");

            i++;

            if (i < count)
            {
                sb.Append(",");
            }
        }

        sb.Append("]");

        return sb.ToString();
    }

I just read somewhere that AppendFormat() can be slower than simple Append() calls. Being shocked by reading this, I decided to investigate.

For 1,000,000 empty events the times required are:

With AppendFormat: 9297 ticks
Without AppendFormat: 8268 ticks

That is a considerable difference of 11%!

I'm guessing this is due to the lookup of the arguments and such. It would be nice if AppendFormat() would be recompiled to Append() calls only by default.

This is the code:

    /// <summary>
    /// Creates a calendar json string.
    /// </summary>
    /// <returns></returns>
    public string ToFullCalendarJsonStringFaster()
    {
        StringBuilder sb = new StringBuilder();

        sb.Append( "[" );

        int count = Events.Count;
        int i = 0;

        foreach ( Event eventItem in Events )
        {
            sb.Append( "{" );

            sb.Append("\"title\":\"");
            sb.Append(eventItem.Title);
            sb.Append("\",");
            sb.Append("\"allDay\":\"");
            sb.Append(eventItem.AllDay);
            sb.Append("\",");

            if ( eventItem.AllDay )
            {
                // My test never comes here, so I left it out.
            }
            else
            {
                sb.Append("\"start\":\"");
                sb.Append(eventItem.Start.ToString("yyyy-MM-dd"));
                sb.Append("T");
                sb.Append(eventItem.Start.ToString("HH:mm:ss"));
                sb.Append("\",");

                sb.Append("\"end\":\"");
                sb.Append(eventItem.End.ToString("yyyy-MM-dd"));
                sb.Append("T");
                sb.Append(eventItem.End.ToString("HH:mm:ss"));
                sb.Append("\",");
            }
            sb.Append("\"url\":\"");
            sb.Append(eventItem.URL);
            sb.Append("\"");
            sb.Append( "}" );

            i++;

            if ( i < count )
            {
                sb.Append( "," );
            }
        }

        sb.Append( "]" );

        return sb.ToString();
    }

By also applying Snowbear's earlier replies:

Using yyyy-MM-ddTHH:mm:ss as format strings, gives an extra speed difference of 5%
Preallocating StringBuilder size, gives an extra speed difference of 6%

In total, after applying all changes, the code is 22% faster. :)

Bottomline is, there probably isn't a 'magic' solution which can make it go instant with so many events, but you can improve the speed considerably. I suggest you run the processing on a BackgroundWorker.

... this is getting even more ridiculous. Changing the date formatting to the following:

//sb.Append(eventItem.Start.ToString( "yyyy-MM-ddTHH:mm:ss" ) );
sb.Append(eventItem.Start.Year);
sb.Append("-");
sb.Append(eventItem.Start.Month);
sb.Append("-");
sb.Append(eventItem.Start.Day);
sb.Append("T");
sb.Append(eventItem.Start.Hour);
sb.Append(":");
sb.Append(eventItem.Start.Minute);
sb.Append(":");
sb.Append(eventItem.Start.Second);
sb.Append("\",");

... gives another speed increase and makes it 34% faster in total. Might slow down
again if you need more specific formatting.

Beware: this last update is probably erronous. I asked a question about proper usage of PLINQ.

I haven't used Parallel LINQ (PLINQ) yet. But this seemed a nice use for it. After replacing the for by:

Events.AsParallel().AsOrdered().ForAll( eventItem =>
{
    ...
} );

I get a total speed increase of 43%, again 9% faster. :) This is on a dual core processor. PC's with more cores should perform better. I don't know how PLINQ works exactly, but I would think it could work even faster if one iteration doesn't need to wait on another. The StringBuilder and i are exposed as closures. Anyone got any better approaches than ForAll()?


Project Euler # 30 Digits fifth power in Python Surprisingly there are only three numbers that can be written as the sum of fourth powers of their digits:

  \$1634 = 1^4 + 6^4 + 3^4 + 4^4\$
  \$8208 = 8^4 + 2^4 + 0^4 + 8^4\$
  \$9474 = 9^4 + 4^4 + 7^4 + 4^4\$

  As 1 = 1^4 is not a sum it is not included.

  The sum of these numbers is \$1634 + 8208 + 9474 = 19316\$.

  Find the sum of all the numbers that can be written as the sum of fifth powers of their digits.

from time import time

def powers(n, power):
    """Assumes n and power integers > 0.
    generates all numbers up to n if total(n digits ** power) = number."""
    nums_pows = {number: sum(int(digit) ** power for digit in str(number)) for number in range(2, n)}
    return (x for x, y in nums_pows.items() if x == y)

if __name__ == '__main__':
    start_time = time()
    print(sum(powers(1000000, 5)))
    print(f'Time: {time() - start_time} seconds.')

Your comprehensions are hard to understand, as you have two on one line. I recomend you spread them over multiple lines to increase readability.
There is no benifit to using a dictionary comprehension, and only makes me think you're abusing comprehensions.

Just use a normal for loop and use yield.
You don't need to pass n, you can find the limit by using power * 9**power.
This causes the program to run in a quarter of the time.

def powers(power):
    for number in range(2, power * 9**power):
        total = sum(int(digit) ** power for digit in str(number))
        if total == number:
            yield number

If you change from using range to just digits and checking that the total is an anagram of the input then you can get the code to run in ~5% the time of the previous code.

def powers(power):
    for size in range(1, len(str(power * 9**power)) + 1):
        for digits in itertools.combinations_with_replacement(range(10), size):
            total = sum(int(digit) ** power for digit in digits)
            if (sorted(str(total)) == sorted(str(digit) for digit in digits)
                and total != 1
            ):
                yield total


Refactor Web Scraper I wrote a simple Zoopla real estate scraper for just practicing what I learned so far in Python, Requests, BeautifulSoup and overall web scraping fundamentals. By looking at my code I feel like there would be a better and more elegant way to write it but unfortunately as a beginner I don't know yet. So, I believe I should let experienced guys here at Stack Exchange to review my code for enhancement.
import requests
import json
import csv
import time
from bs4 import BeautifulSoup as bs

class ZooplaScraper:

    results = []

def fetch(self, url):
    print(f'HTTP GET request to URL: {url}', end='')
    res = requests.get(url)
    print(f' | Status code: {res.status_code}')

    return res

def parse(self, html):
    content = bs(html, 'html.parser')
    content_array = content.select('script[id="__NEXT_DATA__"]')
    content_dict = json.loads(content_array[0].string)
    content_details = content_dict['props']['initialProps']['pageProps']['regularListingsFormatted']

    for listing in content_details:
        self.results.append ({
            'listing_id': listing['listingId'],
            'name_title': listing['title'],
            'names': listing['branch']['name'],
            'addresses': listing['address'],
            'agent': 'https://zoopla.co.uk' + listing['branch']['branchDetailsUri'],
            'phone_no': listing['branch']['phone'],
            'picture': listing['image']['src'],
            'prices': listing['price'],
            'Listed_on': listing['publishedOn'],
            'listing_detail_link': 'https://zoopla.co.uk' + listing['listingUris']['detail']

        })

def to_csv(self):

    with open('zoopla.csv', 'w') as csv_file:
        writer = csv.DictWriter(csv_file, fieldnames=self.results[0].keys())
        writer.writeheader()

        for row in self.results:
            writer.writerow(row)

        print('Stored results to "zoopla.csv"')

def run(self):

    for page in range(1, 5):
        url = 'https://www.zoopla.co.uk/for-sale/property/london/?page_size=25&q=London&radius=0&results_sort=newest_listings&pn='
        url += str(page)
        res = self.fetch(url)
        self.parse(res.text)
        time.sleep(2)
    self.to_csv()

  if __name__ == '__main__':
     scraper = ZooplaScraper()
     scraper.run()

Basically in this scraper I mostly did is JSON parsing. Problem was all the data on the website was coming from JavaScript under the script tag so I have to select that tag and then pass it to json.loads() and parse the JSON dict to find the right key-value pair.

Make a Session instead of issuing individual Requests get; this promotes explicit connection pooling, cookie sharing etc.
There's no need for your current prints. If you find them to be of very high value, convert them into real logging calls
Pre-define your script tag loading via a SoupStrainer
Use urljoin and centralize your root URL definition
Do not keep results as a member; it's the result of a method call
Do not represent results as a list; it can be an iterator so that results can be depaginated and streamed to disk while keeping memory occupation relatively low
Parametrize your fetch function to represent the actual parameters on the web call
Consider using PEP484 type hints
Your open is missing newline=''

Suggested
from functools import partial
from typing import Any, Dict, Iterable, List

import json
import csv
from urllib.parse import urljoin

from bs4 import BeautifulSoup, SoupStrainer
from requests import Session

JSON = Dict[str, Any]

class ZooplaScraper:
    ROOT = 'https://zoopla.co.uk'
    from_root = partial(urljoin, ROOT)

    def __init__(self):
        self.session = Session()

        strainer = SoupStrainer('script', id='__NEXT_DATA__')
        self.load_script = partial(
            BeautifulSoup, features='html.parser', parse_only=strainer,
        )

    def fetch(
        self, query: str = 'London', radius: int = 0,
        sort: str = 'newest_listings', page: int = 1,
    ) -> str:
        with self.session.get(
            self.from_root(f'for-sale/property/{query.lower()}/'),
            params={
                'page_size': 25,
                'q': query,
                'radius': radius,
                'results_sort': sort,
                'pn': page,
            }
        ) as resp:
            resp.raise_for_status()
            return resp.text

    def load(self, html: str) -> List[JSON]:
        script = self.load_script(html)
        data = json.loads(script.string)
        return data['props']['initialProps']['pageProps']['regularListingsFormatted']

    @classmethod
    def serialise(cls, listings: Iterable[JSON]) -> Iterable[JSON]:
        for listing in listings:
            yield {
                'listing_id': listing['listingId'],
                'name_title': listing['title'],
                'names': listing['branch']['name'],
                'addresses': listing['address'],
                'agent': cls.from_root(listing['branch']['branchDetailsUri']),
                'phone_no': listing['branch']['phone'],
                'picture': listing['image']['src'],
                'prices': listing['price'],
                'listed_on': listing['publishedOn'],
                'listing_detail_link': cls.from_root(listing['listingUris']['detail']),
            }

    def run(
        self,
        query: str = 'London', radius: int = 0, sort: str = 'newest_listings',
    ) -> Iterable[JSON]:
        for page in range(1, 5):
            yield from self.serialise(
                self.load(
                    self.fetch(query, radius, sort, page)
                )
            )

    @staticmethod
    def to_csv(results: Iterable[JSON], filename: str = 'zoopla.csv') -> None:
        with open(filename, 'w', newline='') as csv_file:
            first = next(results)
            writer = csv.DictWriter(csv_file, fieldnames=first.keys())
            writer.writeheader()
            writer.writerow(first)
            writer.writerows(results)

if __name__ == '__main__':
    scraper = ZooplaScraper()
    scraper.to_csv(scraper.run())

Experimental
This is an experimental, alternate implementation that:

Streams the HTTP response and does not need the entire response content to complete
Streams the parsed HTML elements and does not need the entire document tree to complete
Streams the JSON body and does not need the entire dictionary tree to complete

It is somewhat iterator-heavy, and built more as a proof of concept to demonstrate that this is possible. Advantages include that worst-case memory usage should be reduced, and that BeautifulSoup is no longer needed. Disadvantages include that a new dependency, JsonSlicer, is needed; and this might introduce subtle HTTP inefficiencies from connections that are reset before complete response transmission.
import csv
import logging
from functools import partial
from html.parser import HTMLParser
from typing import Any, Dict, Iterable, Tuple, Optional
from urllib.parse import urljoin

from jsonslicer import JsonSlicer
from requests import Session, Response

JSON = Dict[str, Any]

class StreamParser(HTMLParser):
    def __init__(self, resp: Response):
        resp.raise_for_status()  # If the response failed, it can't be parsed
        self.resp = resp         # Keep the response so we can stream from it
        self.in_tag = False      # Parser state: if we're in the script tag
        self.done = False        # Whether we're done the script tag
        self.queue = []          # Queue of text element chunks in the script
        super().__init__()       # Initialize the base parser

    def __enter__(self):
        # Start the data chunk iterator
        self.chunks = self.data_chunks()
        return self

    def __exit__(self, exc_type, exc_val, exc_tb) -> None:
        # When we're done, tell the HTTP response stream to close
        self.resp.close()

    def data_chunks(self) -> Iterable[str]:
        # Stream in arbitrary-sized chunks from the response
        for chunk in self.resp.iter_content(
            chunk_size=None,  # Get whatever chunks are sent our way
            decode_unicode=True,  # Needed for HTMLParser compatibility
        ):
            logging.debug(
                f'{len(chunk)}-character chunk: '
                f'{chunk[:10]}...{chunk[-10:]}'
            )
            # Feed this chunk to the parser, which will in turn call our handle
            # methods and populate the queue
            self.feed(chunk)
            yield from self.queue
            self.queue.clear()

            # We only care about one tag. Once that's parsed, we're done
            # iterating
            if self.done:
                break

    def read(self, n: Optional[int] = -1) -> str:
        # Will be called by JsonSlicer. We only support partial reads for
        # efficiency's sake; we do not build up our own buffer string.
        if n is None or n < 0:
            raise NotImplementedError('Read-to-end not supported')
        try:
            return next(self.chunks)
        except StopIteration:
            return ''  # end of stream

    def handle_starttag(self, tag: str, attrs: Iterable[Tuple[str, str]]):
        self.in_tag = tag == 'script' and any(
            k == 'id' and v == '__NEXT_DATA__' for k, v in attrs
        )

    def handle_data(self, data: str) -> None:
        if self.in_tag:
            self.queue.append(data)

    def handle_endtag(self, tag: str) -> None:
        if self.in_tag:
            self.in_tag = False
            self.done = True

    def __iter__(self) -> Iterable[JSON]:
        # Iterating over this object will magically produce individual listing
        # dictionaries. We're an iterator; we delegate to the JsonSlicer
        # iterator; and it in turn invokes read() which uses our data_chunks
        # iterator.
        return JsonSlicer(file=self, path_prefix=(
            'props', 'initialProps', 'pageProps', 'regularListingsFormatted', None,
        ))

class ZooplaScraper:
    ROOT = 'https://zoopla.co.uk'
    from_root = partial(urljoin, ROOT)

    def __init__(self):
        self.session = Session()

    def fetch(
        self, query: str = 'London', radius: int = 0,
        sort: str = 'newest_listings', page: int = 1,
    ) -> StreamParser:

        resp = self.session.get(
            self.from_root(f'for-sale/property/{query.lower()}/'),
            params={
                'page_size': 25,
                'q': query,
                'radius': radius,
                'results_sort': sort,
                'pn': page,
            },
            stream=True,
        )

        return StreamParser(resp)

    @classmethod
    def serialise(cls, listing: JSON) -> JSON:
        # Convert from the site's representation of a listing dict to our own
        return {
            'listing_id': listing['listingId'],
            'name_title': listing['title'],
            'names': listing['branch']['name'],
            'addresses': listing['address'],
            'agent': cls.from_root(listing['branch']['branchDetailsUri']),
            'phone_no': listing['branch']['phone'],
            'picture': listing['image']['src'],
            'prices': listing['price'],
            'listed_on': listing['publishedOn'],
            'listing_detail_link': cls.from_root(listing['listingUris']['detail']),
        }

    def run(
        self,
        query: str = 'London', radius: int = 0, sort: str = 'newest_listings',
        max_pages: int = 4,
    ) -> Iterable[JSON]:
        for page in range(1, max_pages + 1):
            with self.fetch(query, radius, sort, page) as stream:
                for n_listings, data in enumerate(stream):
                    yield self.serialise(data)
            logging.info(f'Page {page}: {n_listings} listings')

    @staticmethod
    def to_csv(results: Iterable[JSON], filename: str = 'zoopla.csv') -> None:
        with open(filename, 'w', newline='') as csv_file:
            first = next(results)
            writer = csv.DictWriter(csv_file, fieldnames=first.keys())
            writer.writeheader()
            writer.writerow(first)
            writer.writerows(results)
        logging.info(f'Write to {filename} complete')

if __name__ == '__main__':
    # Will include debugging statements from urllib3
    logging.basicConfig(level=logging.INFO)  # Switch to DEBUG for more verbosity

    scraper = ZooplaScraper()
    scraper.to_csv(scraper.run())


How to efficiently loop through paragraphs and make simple changes with Word VBA (Specially reverse loop to Delete Paragraphs) This is regarding my answer with SO post How to remove paragraph marks with different format in MS-Word.
My primary question, is there any way performance of the code could be improved to operate on document as intended by OP (2.2 MB and has 2.1K pages, 871K words, 4,6M characters including spaces)?
On Secondary, is there a simple way or workaround that I am missing (and making things unnecessary complicated) to perform the sane task efficiently?

Here I reproduce my code after adding some futile measures to improve performance of the code with file of the size specified by the OP.

    Option Explicit
    Sub ReplacePara()
    Dim Para As Paragraph, Xstr As String, Rng As Range
    Dim i As Long, ln As Long, tm As Double, PrCnt As Long
    Dim PrvChrSize As Integer, NextChrSize  As Integer
    Dim PrvChrFont As String, NextChrFont  As String
    Dim PrvChrItalic As Boolean, NextChrItalic As Boolean
    tm = Timer

‘Following measures added to improve performance
‘but on the contrary it’s found instead of increasing time taken
    Application.ScreenUpdating = False
        With Options
        .Pagination = False
        .CheckSpellingAsYouType = False
        .CheckGrammarAsYouType = False
        End With

        With ActiveDocument
        PrCnt = .Paragraphs.Count
        Debug.Print PrCnt
        For i = .Paragraphs.Count To 1 Step -1
        Set Para = .Paragraphs(i)
        ln = Para.Range.Characters.Count

            If ln > 1 Then
                With Para.Range.Characters(ln - 1).Font
                PrvChrSize = .Size
                PrvChrFont = .Name
                PrvChrItalic = .Italic
                End With

                If i < .Paragraphs.Count Then
                    With .Paragraphs(i + 1).Range.Characters(1).Font
                    NextChrSize = .Size
                    NextChrFont = .Name
                    NextChrItalic = .Italic
                    End With
                Else
                NextChrSize = 0
                NextChrFont = ""
                NextChrItalic = False
                End If
            End If

            'Debug.Print i, PrvChrSize, PrvChrFont, NextChrSize, NextChrFont
            If (PrvChrSize = 15 And (PrvChrFont = "Arial" Or PrvChrItalic = True)) _
            And (NextChrSize = 15 And (NextChrFont = "Arial" Or NextChrItalic)) Then
            Para.Range.Characters(ln).Text = " "
            End If
        .UndoClear
        'If PrCnt < 1000 Then Debug.Print i & "/" & PrCnt
        Next
      End With

        With Options
        .Pagination = True
        .CheckSpellingAsYouType = True
        .CheckGrammarAsYouType = True
        End With
    Application.ScreenUpdating = True
    Debug.Print " Seconds taken:" & Timer - tm
    End Sub

The added measures actually found to increase time taken (from 3 odd minutes to 4 odd minutes) with documents of 124 pages. I haven’t ventured far to go for LockWindowUpdate API.

Though the code tested Ok with documents of 100 pages around. I could not finish the task with a makeshift giant file of around 2.4 K pages. It is virtually crashing Word (not recovering from ‘Not responding mode’). I ceated the file with a simple code stub with the sample file linked by OP in the SO post. Code stub was also produced for ease is testing.

Sub makebig()
Dim Rng As Range, MyRange As Range
Dim Wd As Document
Dim i  As Long
Set Wd = ThisDocument
Set Rng = Wd.Content
Rng.Copy

    For x = 1 To 2000
    Set MyRange = Wd.Content
    MyRange.EndOf Unit:=wdStory, Extend:=wdMove
    MyRange.Paste
    Next
End Sub

Running the code with sample file twice (1st time with For x = 1 To 2000 and second time with For x = 1 To 1) will produce a file about 2.4 K pages. For getting a file of 124 pages from the sample file 200 loops are sufficient.

Answering my Own question to share with community the Height of stupidity in my code and gradually how over 1 or 2 sleepless nights of testing, the problem was reduce to workable.

First the height of stupidity is the line

     If i < .Paragraphs.Count Then

even after knowing well that while working with such giant document interaction with document is to keep minimum, I resorted to  .Paragraphs.Count in every loop of 56 K paragraphs. Whereas .Paragraphs.Count had already been assigned to variable PrCnt.  Also the If is used to only avoid the error trying to access next para while in the last loop of paragraphs and intended to act once only.

Replacing it with 'PrCnt` make the code somehow stable and could run through the code while disabling write on the document.

Next Thanks to @Ryan Wildry suggestion and tried to go for For Each Loop. Since I am deleting paragraph marks. I tried to go for a Forward For Each loop and test the conditions and take the paragraphs number of the paragraphs to be deleted to a Array. This loop takes only 1-2 minutes to collect the desired information form 56 K paras.

Now after completing the loop, started replacing the paragraphs in reveres loop in this fashion

 For i = UBound(ParaNumToDelete) To 1 Step -1
 .Paragraphs(ParaNumToDelete(i)).Range.Characters(LnArr(i)).Text = " "

but this also found taking around 6-7 hours to complete 2.4 K page 56 K paragraphs (and 16 k paragraphs marks to replace) document.  150 seconds to replace 50 paragraphs in the start loop (i.e. near bottom of documents) and hardly 1-2 seconds do replace 50 paragraphs at the end of loop (i.e. near start of the document). Code execution is unstable and feared to be in Non Responding state even with single click.

So one more measures added to Save the document at say every 200 replacement, so code could be run again to complete the uncompleted task any time later.

Finally again thanks to  @Ryan Wildry's comment and a Range Array was created in the First For Each loop (taking 1-2 minutes) and next the range array was iterated in reverse order to replace paragraph marks. It takes only around 10 minutes to complete without save (or around 15 minutes  with save at 200 replacement)

The final code:

Sub TestPara()
Dim Para As Paragraph, PrvLn As Long, xRng As Range, PrvRng As Range
Dim i As Long, ln As Long, tm As Double, PrCnt As Long
Dim ChrSize As Integer, PrvChrSize  As Integer, LastChrSize As Integer
Dim ChrFont As String, PrvChrFont  As String, LastChrFont As String
Dim ChrItalic As Boolean, PrvChrItalic As Boolean, LastChrItalic As Boolean
Dim OnOff As Boolean, DelCnt As Long, DoSave As Boolean
Dim RngArr() As Range, Pos As Long
tm = Timer

TurnOn False

  With ActiveDocument
  PrCnt = .Paragraphs.Count
  Debug.Print PrCnt
  DelCnt = 0
  PrvChrSize = 0
  PrvChrFont = 0
  PrvChrItalic = False
  PrvLn = 0
  i = 1

    For Each Para In .Paragraphs
    ln = Para.Range.Characters.Count
    Pos = Para.Range.End
    Set xRng = ActiveDocument.Range(Pos - 1, Pos)

        If ln > 1 Then
            With Para.Range.Characters(ln - 1).Font
            LastChrSize = .Size
            LastChrFont = .Name
            LastChrItalic = .Italic
            End With

            With Para.Range.Characters(1).Font
            ChrSize = .Size
            ChrFont = .Name
            ChrItalic = .Italic
            End With
        Else
        LastChrSize = 0
        LastChrFont = 0
        LastChrItalic = False
        ChrSize = 0
        ChrFont = 0
        ChrItalic = False
        End If

        If (ChrSize = 15 And ChrFont = "Arial" And ChrItalic) _
        And (PrvChrSize = 15 And PrvChrFont = "Arial" And PrvChrItalic) Then
        DelCnt = DelCnt + 1
        ReDim Preserve RngArr(1 To DelCnt)
        Set RngArr(DelCnt) = PrvRng
        End If

        PrvChrSize = LastChrSize
        PrvChrFont = LastChrFont
        PrvChrItalic = LastChrItalic
        PrvLn = ln
        Set PrvRng = xRng

        If i Mod 2000 = 0 Then
        Debug.Print i & "/" & PrCnt
        End If
    i = i + 1
    Next
Debug.Print " paragraph to delete:" & DelCnt
Debug.Print " Seconds taken to Calc:" & Timer - tm

TurnOn False
DoSave = True

    For i = UBound(RngArr) To 1 Step -1
    RngArr(i).Text = " "

        If i Mod 1000 = 0 Then
        .UndoClear
        DoEvents
        Debug.Print i, Timer - tm
        End If

        If DoSave Then
        If i Mod 200 = 0 Then
        .Save
        DoEvents
        Debug.Print "Save at delete Countdown " & i & "/" & Timer - tm
        End If
        End If
    Next

End With

Debug.Print "  Delete completed in Seconds:" & Timer - tm

TurnOn
Debug.Print " pagination Completed:"
Debug.Print " Seconds taken:" & Timer - tm
End Sub
Sub TurnOn(Optional OnOff As Boolean = True)
Application.ScreenUpdating = OnOff
    With Options
    .Pagination = OnOff
    .CheckSpellingAsYouType = OnOff
    .CheckGrammarAsYouType = OnOff
    End With
End Sub

Hope my ordeal will help the community in similar situation


Convert tree to circular doubly linked list I want you to pick my code apart and give me some feedback on how I could make it better or simpler.  This code converts a binary tree to a doubly linked list (in order).

public class BinaryTreeToLinkedList {

    private TreeNode root;

    private static class TreeNode {
        TreeNode left;
        TreeNode right;
        int item;
        public TreeNode(TreeNode left, TreeNode right, int item) {
            this.left = left;
            this.right = right;
            this.item = item;
        }
    }

    private TreeNode appendDLL (TreeNode left, TreeNode right) {
        if (left == null) return right;
        if (right == null) return left;

        // get last element of left linkedlist
        TreeNode leftLast = left.left;
        TreeNode rightLast = right.left;

        leftLast.right = right;
        right.left = leftLast;

        // now we form the list into circular again.
        rightLast.right = left;
        left.left = rightLast;

        return left;
    }

    private TreeNode postOrder(TreeNode node) {
        if (node == null) {
            return null;
        }

        TreeNode leftList = postOrder(node.left);
        TreeNode rightList = postOrder(node.right);

        // convert the current node into doubly LL
        node.left = node;
        node.right = node;

        // hook current node.
        leftList = appendDLL (leftList, node);

        // hook right side linkedlist
        leftList = appendDLL (leftList, rightList);

        return leftList;
    }

  public void convertToLinkedList() {
            root = postOrder(root);
        }
    }

Your code certainly seems to do the job (assuming that the tree is only a root node with two branches; otherwise you will need to implement some recursion).  It doesn't "feel" very Java-esque, but you've said in your previous questions/comments that you don't really want critique of method and variable names, so I'll try to avoid that.

if (left == null) return right;
if (right == null) return left;

This code will return null if I pass in null for both left and right.  Is that intentional?  It's unclear from the way you've written your code that that would be the desired result.  When I think about it, it makes sense that the method would return null in this scenario, but I'm not sure if you understood that consequence when you coded it, just based on the style.

if (node == null) {
    return null;
}

You should try to use consistent style in your code.  If you're going to put these checks and return statements at the beginning, either put them on the same line as the if() statement or don't.  It doesn't matter which you do, as long as you do the same thing throughout.  Makes your code much more readable and easier to maintain.

// get last element of left linkedlist
TreeNode leftLast = left.left;
TreeNode rightLast = right.left;

leftLast.right = right;
right.left = leftLast;

// now we form the list into circular again.
rightLast.right = left;
left.left = rightLast;

return left;

This entire block is very difficult to scan.  What it does isn't readily apparent by the way you've named all your variables.  For example rightLast sounds like the rightmost node in the tree, but in fact it's the left node of the right node in the tree.  Then with all the subsequent processing, it's very confusing without stepping through line-by-line to get a better understanding.  Consider renaming your variables to be more meaningful.  In my opinion, the most beautiful and elegant code is self-documenting.

Also, if you were looking for ways to further improve the code's functionality (besides adding recursion), you might look into generic types so that your nodes can hold any data, rather than just an int.


Flatten an array - loop or reduce? Problem: Concat all sub-arrays of a given array.

Example input: [[0, 1], [2, 3], [4, 5]]

Example output: [0, 1, 2, 3, 4, 5]

Solution A: Use a loop

var flattened=[];
for (var i=0; i<input.length; ++i) {
    var current = input[i];
    for (var j=0; j<current.length; ++j)
        flattened.push(current[j]);
}

Solution B: Use reduce

var flattened = input.reduce(function(a, b) {
    return a.concat(b);
}, []);

Solution B looks much shorter and easier to understand, but, it seems to be much more resource-wasteful - for each element of the input, a new array is created - the concatenation of a and b. On the contrary, solution A uses a single array 'flattened', which is updated during the loop.

So, my question is: which solution is better? Is there a solution that combines the best of both worlds?

Here is the performance test for these two and couple more approaches (one suggested by @elclanrs in the comments).

The performance differences will vary significantly across different browsers, and even different version on same browser, as browser these days try to optimize javascript very aggressively.

Unless you are dealing with very large arrays or this operation is performed repeatedly in quick succession, I would suggest you to use simplest and clearest approach. But, that being said the loop solution is also not that complex or big anyway (and it performs better than others especially on firefox)


K&R Exercise 1-6 & 1-7: working with EOF I'm working through K&R at the moment and I've found my first exercise that seems to be a good use for my first CodeReview post.

Exercise 1-6 - Verify that the expression getchar() != EOF is 0 or 1.

Exercise 1-7 - Write a program to print the value of EOF.

I've completed both of these together in the same file.

I think I've understood the question here, but let me know if I haven't. I wanted to post here before looking up any sort of "official" answer so that I could learn some stuff instead of just "going to the back of the book".

#include <stdio.h>

void main()
{
        int c;

        c = getchar();
        while ((c = getchar()) != EOF == (0 | 1)) {
                putchar(c);
        }
        printf("%d\n", EOF);
}

Compiled with:

gcc putchar.c -o putchar

Sending EOF using:

<ctrl-d>

Runs through fine, and when I ctrl-d I get a -1 return code.

This is a nice, clear, short attempt at the exercise.
You've avoided a couple of common problems:

You're correctly using int for the return value from getchar().  A common mistake is to be misled by the name and to assign directly to a char variable.  That doesn't work, because truncating to char prevents EOF from being distinguished from a valid char value.
You've correctly matched the %d specifier to the type of EOF.

One simple way to improve your code is to get the compiler to point out weaknesses for you (by default, most C compilers are very lenient; you have to specifically ask for more warnings):
gcc -std=c11 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion putchar.c -o putchar

You can make it easier by writing a simple Makefile that contains just:
CFLAGS = -std=c11 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion

And you can then build simply with
make putchar

When you've enabled warnings, you'll see:
198374.c:3:6: warning: return type of ‘main’ is not ‘int’ [-Wmain]
 void main()
      ^~~~

The fix for that should be obvious.

Now to the meat of the program.  Firstly, before the loop we have a statement
    c = getchar();

This consumes and ignores one character.  We probably didn't mean to throw that one away.

Now consider this expression:
 (c = getchar()) != EOF == (0 | 1)

We're actually comparing (after the assignment) c != EOF against 0|1.  But | is the bitwise-or operator, so 0|1 evaluates to just 1.  When c is not EOF, then c!=EOF is 1, and the condition is satisfied.  The last time around the loop (when c becomes EOF), then c!=EOF is 0, and the loop exits.
What we want to do is save the result of c != EOF and compare that to 0 and to 1 separately.  Or, even more simply for this exercise, just print it to standard output:
    do {
        c = getchar();
        printf("getchar() != EOF: %d\n", c != EOF);
    } while (c != EOF);

Note: given that we haven't been asked to copy the input to output, we shouldn't have the putchar() call.
If we really want to prove that c != EOF is always either 0 or 1, we could test it automatically:
    do {
        c = getchar();
        if ((c != EOF) != 0  &&  (c != EOF) != 1) {
            fprintf(stderr, "(c != EOF) was neither 0 nor 1\n");
            return 1;
        }
    } while (c != EOF);

You might choose to use a switch statement instead of the if; I think that's clearer:
        c = getchar();
        switch (c != EOF) {
        case 0:             /* okay */
        case 1:             /* okay */
            break;
        default:
            fprintf(stderr, "(c != EOF) was neither 0 nor 1\n");
            return 1;
        }

Modified code
#include <stdio.h>

int main()
{
        int c;

        do {
            c = getchar();
            switch (c != EOF) {
            case 0:             /* okay */
            case 1:             /* okay */
                break;
            default:
                fprintf(stderr, "(c != EOF) was neither 0 nor 1\n");
                return 1;
            }
        } while (c != EOF);

        printf("%d\n", c);
}


Merge sort C# Implementation Its been years (10+) since I have worked with sorting algorithms directly.  So I have been trying to go over them again to refresh my memory. The thing is my ideas of how to do it doesn't seam to match the way I have the tutorials doing it.

Does it matter?  Assuming that my code is working I would almost mean that its correct.  However I have also been looking into the Big-O I really cant remember ever seeing this before.  I would almost think that if you code it incorrectly it will slow things down and mess up the performance.

public class MergeSort2
    {

        public MergeSort2()
        {
            var data = Util.CreateData.CreateRandomIntArray(200);  // just a method that spits out an array of ints
            var sortedData = Sort(data);

        }

       private int[] copyArray(int[] A, int start, int end)
        {

            int[] result = new int[end-start];

            int cnt = 0;
            for (int i = start; i < end; i++)
            {
                result[cnt] = A[i];
                cnt++;
            }

            return result;
        }

        public int[] Sort(int[] data)
        {

            if (data.Length <= 1)
                return data;

            var length = data.Length / 2;

            int[] LeftArray = copyArray(data,0,length);
            int[] RightArray = copyArray(data, length,data.Length);

            Array.ForEach(data, a => Console.Write(a + ","));
            Console.Write("\r\n");
            Array.ForEach(LeftArray, a => Console.Write(a + ","));
            Console.Write("\t - \t");
            Array.ForEach(RightArray, a => Console.Write(a + ","));
            Console.Write("\r\n");

            RightArray = Sort(RightArray);
            LeftArray = Sort(LeftArray);

            var sorted = merge(RightArray, LeftArray);

            Console.Write("Sorted: ");
            Array.ForEach(sorted, a => Console.Write(a + ","));
            Console.Write("\r\n");

            return sorted;
        }

        public int[] merge(int[] right, int[] left)
        {

            int[] merged = new int[right.Length + left.Length];

            int cntright = 0;
            int cntleft = 0;

            for (int i = 0; i < merged.Length; i++)
            {
                if (cntright == right.Length)
                {
                    merged[i] = left[cntleft];
                    cntleft++;
                }
                else if (cntleft == left.Length)
                {
                    merged[i] = right[cntright];
                    cntright++;
                }
                else if (right[cntright] <= left[cntleft])
                {
                    merged[i] = right[cntright];
                    cntright++;
                }
                else
                {
                    merged[i] = left[cntleft];
                    cntleft++;
                }
            }

            return merged;
        }

    }

So I guess my question is two fold:

How is my implementation of merge sort?
Does it matter that I'm implementation isn't exactly the same as everyone else?

Inside of your copyArray you should not have the cnt variable separate from the for loop, you should just include it in the declaration. The name of that Method should be in PascalCase not it camelCase.  You should also give operators some space (ie [end - start])

so this:

   private int[] copyArray(int[] A, int start, int end)
    {

        int[] result = new int[end-start];

        int cnt = 0;
        for (int i = start; i < end; i++)
        {
            result[cnt] = A[i];
            cnt++;
        }

        return result;
    }

would become this

private int[] CopyArray(int[] A, int start, int end)
{
    int[] result = new int[end - start];
    for (int i = start, cnt = 0; i < end; i++, cnt++)
    {
        result[cnt] = A[i];
    }
    return result;
}

you can remove the cnt++ from inside the loop and just add it to the for loop declaration.

Variables should be camelCase

 int[] LeftArray = copyArray(data,0,length);
 int[] RightArray = copyArray(data, length,data.Length);

These should be

 int[] leftArray = CopyArray(data,0,length);
 int[] rightArray = CopyArray(data, length,data.Length);

And

public int[] merge(int[] right, int[] left)

should be

public int[] Merge(int[] right, int[] left)

Because Methods should always be in PascalCase

And

int cntright = 0;
int cntleft = 0;

should be

int cntRight = 0;
int cntLeft = 0;

for the same reason


Component implementation I'm going to implement something like the Unity3D component model for my C++ game engine. Please, review my draft of the Component base class:

class Component
{
public:
    template<class T>
    static T* create(void)
    {
        if (Type<T>::id == 0)
            Type<T>::id = ++_idCounter;

        T* component = new T();
        component->_id = Type<T>::id;
        return component;
    }

    template<class T>
    inline bool is(void) const
    {
        return Type<T>::id == _id;
    }

protected:
    Component(void) {}

private:
    static unsigned int _idCounter;
    unsigned int _id;

    template<class T> struct Type { static unsigned int id; };
};

unsigned int Component::_idCounter = 0;
template<class T> unsigned int Component::Type<T>::id = 0;

It has some RTTI-like functionality, and I can use it this way:

class ComponentA: public Component {};
class ComponentB: public Component {};

...

ComponentA* a = Component::create<ComponentA>();
ComponentB* b = Component::create<ComponentB>();

std::cout << a->is<ComponentA>() << std::endl; // 1
std::cout << b->is<ComponentB>() << std::endl; // 1
std::cout << b->is<ComponentA>() << std::endl; // 0
std::cout << a->is<ComponentB>() << std::endl; // 0

And I have a question. How can I prevent any construction of any descendant of Component outside of Component::create?

Sorry, but this is simply not a good design.

Your reliance on memory-owning raw pointers means that the library is easy to use wrongly, and that the user will leak memory. This is a big no-no in modern C++. First, because it’s harmful and makes user code brittle and complex; and second, because it is unnecessary. Modern C++ gives you all the tools to avoid manual memory management.

Think carefully about ownership in your component model and choose an appropriate smart pointer. Since you control the object hierarchy, boost::intrusive_ptr may be the best match, but consider other alternatives which are less generic (such as std::unique_ptr).

Finally think about ways to avoid pointer use completely, if possible. In fact, the code you’ve shown doesn’t use runtime polymorphism at all so there is no use of pointers. Just handle objects directly.

Regarding your implementation of RTTI: Without knowing the detailed requirements of your object model it’s hard to comment here; however, you say that you don’t want to get the RTTI overhead. What makes you think C++ RTTI has more overhead than your solution? In fact the implementation you’ve shown is, if anything, less efficient than C++ RTTI because your use-case can be resolved at compile-time.

But let’s assume that you just forgot to make your classes polymorphic, and that the real code cannot resolve the type information at compile time. Then your RTTI mechanism stores redundant information and is still less efficient than C++’ built-in mechanism.

Finally, if you are afraid of RTTI overhead (and have data to back this up) then the real solution is not to roll your own system, it’s to avoid RTTI be redesigning your classes such that they have a uniform interface and don’t need to be distinguished at runtime. In fact, this is the idiomatic solution in C++, and RTTI is the exception rather than the norm.


Graph (adjacency list) for BFS and DFS in Golang I am trying to implement a Graph data structure in Golang and choose to use adjacency list representation. When starting to implement adjacency list, I have an idea that if I use map instead of list. I think (not sure though) this is much much easier to implement and I can find element in \$O(1)\$ rather using \$O(n)\$. I hope anyone can help review my code and the correct way to do adjacency graph.

I try to make it general by declaring payload as a new Interface type which needs a Compare() method to use in BFS and DFS later on.

type Graph struct {
    nodes map[int]Node
}

type Node struct {
    payload  Interface
    adjNodes map[int]struct{}
}

func (g *Graph) addNode(key int, payload Interface) {
    if _, ok := g.nodes[key]; ok {
        return
    }
    g.nodes[key] = Node{payload: payload, adjNodes: make(map[int]struct{})}
}

func (g *Graph) addEdge(src, dst int) {
    // expand if no this node in graph
    if src < 0 || dst < 0 {
        panic("Node number can't be below 0")
    }
    // check if node exists
    if _, ok := g.nodes[src]; !ok {
        panic("No node " + string(src))
    }
    if _, ok := g.nodes[dst]; !ok {
        panic("No node " + string(dst))
    }
    // connect by adjacent list
    g.nodes[src].adjNodes[dst] = struct{}{}
    g.nodes[dst].adjNodes[src] = struct{}{}
}

I don't know Go so I'll limit this to a more general review. If I recommend non-standard things, please point them out so someone else doesn't pick up bad habits. :)

Unless the error result is always named ok in Go, I would prefer found in the cases where you are searching for a node by key to clarify its meaning.

A comment in addEdge says missing nodes will be created automatically ("make new node if doesn't have one"), but the code doesn't match. The problem is that there's no payload for the new node(s).

The graph will let you add nodes to which you cannot attach edges. Move the key checking to a new getNode method, and call it from addNode and addEdge.

func (g *Graph) addNode(key int, payload Interface) {
    if node, found := g.getNode(key); !found {
        node = Node{payload: payload, adjNodes: make(map[int]struct{})}
        g.nodes[key] = node
    }
    return node
}

func (g *Graph) getNode(key int) {
    if key < 0 {
        panic("Node key can't be negative")
    }
    if node, found := g.nodes[key]; !found {
        panic("Node key not found")
    }
    return node
}

func (g *Graph) addEdge(src, dst int) {
    if srcNode, found := g.getNode(src); !found {
        panic("No node " + string(src))
    }
    if dstNode, found := g.getNode(dst); !found {
        panic("No node " + string(dst))
    }
    // connect by adjacent list
    srcNode.adjNodes[dst] = struct{}{}
    dstNode.adjNodes[src] = struct{}{}
}

Map vs. List

To your comment, since the caller is free to use sparse keys, e.g., 1, 29187, 23957172, 6832, 4772589, etc., the map (assuming it's a hash map under the hood) will give you \$O(1)\$ access for the most part.

If you were to assign the keys as indices into a list in addNode and return the new key, you could use a list instead. I don't know how Go's internal map and list are implemented, but I assume it must provide some sort of growable array class. If not, they aren't difficult to build.

As a user, I would prefer to have the graph assign the keys for me.


Body mass index calculator in Swift Today I created my second app - a simple body mass index calculator. The BMI is calculated by the following formula:

$$\dfrac{\text{weight}}{\text{(height)}^2}$$

The general meaning of the resulting BMI values:

\$<18.5\$ = underweight
\$18.5-25\$ = normal weight
\$25-30\$ = overweight
\$>30\$ = obese

My goal was to improve my skills and make something useful at the same time. I'm sharing my code in hope to get some constructive feedback. Is there anything I could improve?

My model file:

import Foundation

struct CalculatorBrain {
let weightInKG: Double
let heightInCM: Double

init(weightInKG: String, heightInCM: String) {
    self.weightInKG = Double(weightInKG) ?? 0.0
    self.heightInCM = Double(heightInCM) ?? 0.0
}

func calcBmi() -> Double {
    return weightInKG / ((heightInCM / 100) * (heightInCM / 100))
}
}

The ViewController:

class ViewController: UIViewController {

@IBOutlet weak var weightTextField: UITextField!

@IBOutlet weak var heightTextField: UITextField!

@IBOutlet weak var resultTextField: UILabel!

@IBAction func calculateBtn(_ sender: Any) {
    guard let weightInKG = weightTextField.text,
        let heightInCM = heightTextField.text else {
            return
    }
    let cm = CalculatorBrain(weightInKG: weightInKG, heightInCM: heightInCM)
    let result = cm.calcBmi()
    resultTextField.text = "BMI: \(result)"
}

}

I have some minor tips about naming:

weight is entirely different than mass, thus I advice you to change that, because some users might be annoyed by this common confusion, if it is present in the user interface of the app. See Mass versus weight
CalculatorBrain does not really feel related to BMI. Maybe BMICalculator would be a better fit here

When it comes to your struct, if that is all you are planning to do with it, this gets quite useless. Instead, I would simply use a function, taking two Double parameters (to avoid String conversion). Also, I would not use ((heightInCM / 100) * (heightInCM / 100)), because it does an unnecessary multiplication between two known numbers, two of 100 in this case. This would be an example:

func calculateBMI(massInKilograms mass: Double, heightInCentimeters height: Double) -> Double {
    return mass / ((height * height) / 10000)
}

It is shorter and, in my opinion, more elegant and efficient. And now you do not need to initialize a struct and call one of its methods each time, instead you only need to call it:

calculateBMI(massInKilograms: x, heightInCentimeters: y)


A version of operator<< that returns ostringstream instead of ostream I always thought it would be handy to be able to write:

const std::string s = (std::ostringstream() << "hi" << 0).str();

This doesn't work in C++ by default because operator<< returns the ostringstream as a ostream, which obviously doesn't have the str() member.

I was pleasantly surprised though to discover that this can be done in C++11:

#include <iostream>
#include <sstream>
#include <string>

template <typename CharT, typename traits, typename T>
std::basic_ostringstream<CharT,traits>&& operator<<(std::basic_ostringstream<CharT,traits>&& out, const T& t) {
  static_cast<std::basic_ostream<CharT,traits>&>(out) << t;
  return std::move(out);
}

int main() {
  const std::string s = (std::ostringstream() << "hi" << 0).str();
  std::cout << s << std::endl;
}

It needs rvalue references and move semantics to bind the temporary to a reference here.

Is there a good reason why this isn't provided by the standard library by default? Is it an oversight? Making this idiom legal code seems like it might offer a number of benefits.
In my local C++11 project, I provide some "utility" code. Is there any reason why I shouldn't include this and use it widely?

How about the more generic version:

template <typename S, typename T, class = typename
               std::enable_if<std::is_base_of<std::basic_ostream<typename S::char_type,typename S::traits_type>, S>::value>::type>
S&& operator<<(S&& out, const T& t) {
  static_cast<std::basic_ostream<typename S::char_type,typename S::traits_type>&>(out) << t;
  return std::move(out);
}

Bjarne gives a pretty succinct description of why this (and many similar) features aren't in the standard. He points out that virtually everybody who talks to him about C++ has essentially the same thing to say: "C++ is way to big and way too complex. You should really work at making it a lot smaller and simpler. Oh, but while you're at it, you should add this one really great feature I thought of"

It kind of reminds me of things I've seen about how questions of political polls can be/ are phrased. If I ask: "Do you think this country should do more to [increase jobs | help the needy | etc.]?" I can count on a large majority of people saying "yes." If I ask "Do you think we should raise taxes by X%?", I can count on an equally large majority saying "No". I can give nearly a guaranteed outcome from my poll just by choosing whether to ask about the hoped-for outcome or the cost.

Bottom line: Each feature you add has a cost, and focusing on the outcome doesn't remove the cost. To design a usable language/library, you need to restrict features to the few that provide the greatest positive outcome for the lowest cost. In the case of the C++ committee, they've been pretty clear about a few things, and one is that most new features will barely be considered at all unless they enable fairly substantial improvements in code that uses the new feature and have extremely minimal cost, especially in terms of breaking any existing code.


Find word in two dimensional char array Task: I have two dimensional array of chars filled with random values [a-z] and need to find if the word ala appears vertically, horizontally or diagonally in it.

In my program, I go through all characters and try to find word in all dimensions.

I feel there is more intelligent way to do this task which will:

Work with words of different length without hard-coding.
Replace bunch of if statements so it will be easier.

Any advice?

import java.util.Random;

public class Ala {
    public static void main(String[] args) {
        Random randomGenerator = new Random();
        char[][] arr = new char[50][50];
        for (int i = 0; i < arr.length; i++) {
            for (int j = 0; j < arr[i].length; j++) {
                arr[i][j] = (char) (randomGenerator.nextInt(26)+97);
            }
        }
        for (int i = 0; i < arr.length; i++) {
                System.out.printf("%-3d", i);
        }
        System.out.println();
        for (int i = 0; i < arr.length; i++) {
            for (int j = 0; j < arr[i].length; j++) {
                System.out.print(arr[i][j]+"  ");
            }
            System.out.println();
        }
        boolean ala = false;
        for (int i = 0; i < arr.length; i++) {
            for (int j = 0; j < arr[i].length; j++) {
                if (j>1) {
                    if (arr[i][j] == 'a' && arr[i][j - 1] == 'l' && arr[i][j - 2] == 'a') {//left
                        ala = true;
                        System.out.println(i + " " + j);
                    }
                }
                if (j<arr[i].length-2) {
                    if (arr[i][j] == 'a' && arr[i][j + 1] == 'l' && arr[i][j + 2] == 'a') {//rigth
                        ala = true;
                        System.out.println(i + " " + j);
                    }
                }
                if (i<arr.length-2) {
                    if (arr[i][j] == 'a' && arr[i + 1][j] == 'l' && arr[i + 2][j] == 'a') {//bot
                        ala = true;
                        System.out.println(i + " " + j);
                    }
                }
                if (i>1) {
                    if (arr[i][j] == 'a' && arr[i-1][j] == 'l' && arr[i - 2][j] == 'a') {//top
                        ala = true;
                        System.out.println(i + " " + j);
                    }
                }
                if (i<arr.length-2 && j<arr[i].length-2) {
                    if (arr[i][j] == 'a' && arr[i + 1][j + 1] == 'l' && arr[i + 2][j + 2] == 'a') {//bot&rigth
                        ala = true;
                        System.out.println(i + " " + j);
                    }
                }
                if (i>1 && j>1) {
                    if (arr[i][j] == 'a' && arr[i - 1][j - 1] == 'l' && arr[i - 2][j - 2] == 'a') {//top&left
                        ala = true;
                        System.out.println(i + " " + j);
                    }
                }
                if (i>1 && j<arr[i].length-2) {
                    if (arr[i][j] == 'a' && arr[i - 1][j + 1] == 'l' && arr[i - 2][j + 2] == 'a') {//top&rigth
                        ala = true;
                        System.out.println(i + " " + j);
                    }
                }
                if (i<arr.length-2 && j>1) {
                    if (arr[i][j] == 'a' && arr[i + 1][j - 1] == 'l' && arr[i + 2][j - 2] == 'a') {//bot&left
                        ala = true;
                        System.out.println(i + " " + j);
                    }
                }
            }
        }
        System.out.println(ala);
    }
}

Your code is indeed sub-optimal.

When looking at a task do not start writing some loops, but think about it is made up of simpler tasks.

A word is in a two-dimensional crossword grid if any of the below is True:

The word is in any of the rows.
The word is in any of the columns.
The word is in any of the diagonals.

So, the general outline of the method may be:

class WordGrid {
  public contains(String word) {
    return any(word in row for row in getRows())                ||
           any(word in column for column in getColumns())       ||
           any(word in diagonal for diagonal in getDiagonals())
   }
}

Now you just have to translate this pseudocode to real code and implement the three get methods and the problem will be solved.

This is my suggestion because:

The general idea can be understood at a glance, if the reader wants more details he can read the helper methods.
It is made of many small pieces, that may be reused and are simpler to write on their own.
It is general working, for any grid and word size.


Alternative code to classes in Javascript? I'm new to JavaScript and came across classes in ES6. Though they are great, I don't want to learn something that can be done in other ways. So, I created this code that does somehow similar job to what classes in JS do. Please give me feedback on whether classes does more than what I think they can do and whether my code is missing something that classes can do.
This is the JS code:

<script>
const person = function(first, second, age, sex){
    return {
        firstName:first,
        lastName:second,
        age:age,
        gender:sex,
        fullName:function(){
            return this.firstName + " " + this.lastName;
        }
    }
}
var John = person("John", "Anderson", 21, "Male");
var Mary = person("Mary", "Smith", 32, "Female");

document.getElementById("demo1").innerHTML = John.firstName;
document.getElementById("demo2").innerHTML = Mary.fullName();
</script>

And the output of the code is:

John

Mary Smith

A short review;

In stacktraces, person will be a nameless function, use function person(){} instead
There is no good reason to have different names (first->firstName) for your attributes. Using the same name allows you to use some nifty JS syntax
Your code will create a new function for each instance of person, unless you have a smart VM, that could be heavy on memory
I would extend the .prototype of person for fullName to prevent that memory issue
I also strive to name functions with a <verb><thing> scheme so I would call it getFullName()
Ideally functions that are akin to a class are capitalized, so person -> Person

So, all in all, this is my counter proposal:

    function Person(firstName, lastName, age, gender){
      Object.assign(this, {firstName, lastName, age, gender});
    }

    Person.prototype.getFullName = function personGetFullName(){
      return this.firstName + " " + this.lastName;
    }

    const John = new Person("John", "Anderson", 21, "Male");
    const Mary = new Person("Mary", "Smith", 32, "Female");
    console.log(John.firstName);
    console.log(Mary.getFullName());


Print file out 1 character at a time in the terminal - Python This program opens a file and prints the text out to the terminal 1 character at a time and at a predetermined speed using time.sleep()

Simple and maybe, if you think like me, pretty cool..

GitHub

from time import sleep
import sys

BLINK = 0.2
chars_in_line = 50

def open_file(fn):
    with open(fn, "r") as f:
        for line in f:
            if line:
                yield line

def type_out_text_to_line(text, end="", sep="", line_chars=None, blink=0.2):
    if not text:
        return ""

    c = line_chars
    for i, j in enumerate(text):
        sys.stdout.flush()
        sleep(blink)
        print(j, end=end, sep=sep)
        if line_chars:
            if i + 1 == c:
                print()
                c += line_chars
    return ""

print("\n\tSTART PRINTING\n")

for text in open_file('example.txt'):
    if len(text) > 1:
        print(type_out_text_to_line(text, line_chars=chars_in_line, blink=BLINK, sep=""))

print("\n\tEND PRINTING\n")

I would recommend just using the built in functionality of print to do the flushing. Though I am an advocate for breaking things out into classes and functions, you should use built in functionality when you can.

Making code complex for no reason results in two things:

1) More bugs.
2) More programmers who want to wring your neck down the road because they have to maintain or fix your software.

So you can simplify your code like this:

from time import sleep

BLINK = 0.2

print("\n\tSTART PRINTING\n")

with open('example.txt') as f:
    for line in f:
        for char in line:
            print(char, flush=True, end='')
            sleep(BLINK)

print("\n\tEND PRINTING\n")

If you want to make this into a function (to make it easier to import/export) then I would do following:

from time import sleep

def time_print_file(filename, print_rate, **print_kwargs):
    """
    INPUT:
        filename: The path to the file to be printing
        print_rate: The delay used to print each character
        print_kwargs: keyword arguments accepted by pythons
           print statement. (Except `end` and `flush`)
    """
    print_kwargs.pop('flush', None) # We don't allow the flush print-keyword
    print_kwargs.pop('end', None) # We don't allow the end print-keyword
    with open(filename) as f:
        for line in f:
            for char in line:
                print(char, flush=True, end='', **print_kwargs)
                sleep(print_rate)

if __name__ == '__main__':
    print("\n\tSTART PRINTING\n")
    time_print_file(filename='example.txt', print_rate=0.2)
    print("\n\tEND PRINTING\n")


Next multiple with only 1 and 0 as digit Problem:
Find a multiple of a given decimal number \$N\$ that looks like a binary number.

The input will consist of at most \$ 2 \times 10^5 \$ lines, each line consist of an integer \$N\$ (\$0 \lt N \lt 10^{12} \$). Find its described multiple \$M (\ne 0\$), this number \$M\$ must be less than \$10^{12}\$. If there is no such number, print '\$-1\$' on the output.

My solution to this problem is quite simple. Just generated all binary-like numbers less than \$10^{12}\$ excluding \$0\$ (there are \$4095\$ such numbers).

For each number from input, just do a linear search on the binary numbers list. Is it possible to do it faster?

#include <cstdio>
#include <vector>

std::vector<long long> bits;

void generateBits(){

    for (int i = 1; i < 4096; ++i) {

        long long e = 1; //e is the exponencial of 10
        long long n = i; //n will be used to extract the bits from the number i
        long long d = 0;//binary-like decimal number...

        while (n) {
            d += ((n&1)*e);
            n >>= 1, e *= 10;
        }

        bits.push_back(d);
    }
}

long long getBitMultiple(long long n){

    for (auto &bit : bits) {
        if(bit%n == 0){
            return bit;
        }
    }

    return -1;
}

int main(void){

    generateBits();

    long long n;

    while (scanf("%lld",&n) != EOF) {
        printf("%lld\n",getBitMultiple(n));
    }

    return 0;
}

I normally don't do C++ reviews, so I might be off on a few accounts, but here are some style and code review points.

Style and Code Review

Avoid cramped up statements – Mostly your code reads easily, but avoid stuff like if(bit%n == 0){, open it up to become if (bit % n == 0) {. Especially important since you seem to have decided on having the starting brace on the same line. It needs to stand out to be recognised. Along the same line of reasoning, add a space after commas like in the printf("...", getBitMultiple(n));
Aim for better names – Names like e, n and d are not really intuitive. Neither is bit or getBitMultiple, it is not a bit but a decimal number looking like a binary. Confusing. Maybe change bits into binaryLookingDecimals?
Add more comments – Parts of the code is unclear, like what you are aiming for in your functions, and where do you get your numbers from, and how does your while loop work
Avoid using globals – The main table of your code, bits, is a global variables. As it reads now, the generateBits() and getBitMultiple(n) are separate non-related functions. If however you add something like binaryLookingDecimals = generateBinaryLookingDecimals() and findMultiple(n, binaryLookingDecimals) they are clearly related

Edited: My solution review was based on a misunderstanding of the problem statement. As a foreigner I confused multiple/multiply/multiplicand, and thought you had done the same. Your code does however return the binary looking decimal which is a multiple of \$N\$ as requested, and that renders my previous solution void, which found the factor you need to multiply to \$N\$ to get a binary looking decimal.

The only comment I then have to your actual solution, is whether it is worth it to test whether the candidate is greater than \$N\$ before starting the modulo operation, but that has to be tested if it gives any significant speed increase. Most likely it is not worth it.

Thanks to Barry for correcting me regarding my mistake on the solution review


Python script to read CSVs and regroup it into the data types I need This code does the following:

Read CSV file and determine sub_type based on series_desc.
Turn row into dictionaries new_person and new_subscriptions.
Check list people for the new_person and add the person if it is actually new.
Check each person in the list people for the new_subscription and add it if it is not there.

My desired output is a list of dictionaries called people. A dictionary in people might look like this:

{'id': group_id, 'subscriptions': [sub_dict1, sub_dict2]}

where a sub_dict looks like this:

{'id': sub_id, 'season': season_desc, 'sub_type': sub_type}

The code:

import csv

people = []

#open csv file
csv_file = csv.reader(open(filename, 'rb'))

#read CSV file into a list of people
for record in csv_file:
    sub_id = record[0]
    group_id = record[6]
    season_desc = record[11]
    series_desc = record[12]
    price = record[13]

    if "MW" in series_desc:
        sub_type = "Masterworks"
    elif "Pops" in series_desc:
        sub_type = "Pops"
    elif "Chamber" in series_desc:
        sub_type = "Chamber"

    new_person = {'id': group_id}
    new_subscription = {'id': sub_id, 'season': season_desc, 'sub_type': sub_type}

    #if this is the first time through the loop
    if len(people) == 0:
        new_person['subscriptions'] = []
        new_person['subscriptions'].append(new_subscription)
        people.append(new_person)

    #if this is not the first time through the loop
    else:
        #check if this person has already been recorded
        if any(person['id'] == new_person['id'] for person in people) == True:
            #check if this subscription has already been added to this person
            #use enumerate to get a key to search people
            for i in enumerate(people):
                if i[1]['id'] == new_person['id']:
                    key = i[0]

                    if any(sub['id'] == new_subscription['id'] for sub in people[key]['subscriptions']) == True:
                        pass
                else:
                    people[key]['subscriptions'].append(new_subscription)

        #if not, add the subscription to them and save the person
        else:
            new_person['subscriptions'] = []
            new_person['subscriptions'].append(new_subscription)
            people.append(new_person)

The challenge: It is REALLY slow.

I have for/if/else loops nested inside for/if/else loops, so I am sure that is the cause. What can I do differently?

The opened file is never closed, a better habit is to use
with open(filename, 'rb') as foo:.
foo == True is rarely needed; unless you really need to
differentiate between True/False and the various other truthy
values it's easier just to test for the variable instead.  Same goes
for the empty list for that matter.
The pass doesn't do anything; price is unused.
Instead of linear search you could keep a set or dict of already
recorded people around.
With enumerate it's easier to destructure the result directly in the
loop; that said, the enumerate is also not necessary here.
The use of key looks buggy:  In the else branch it has never been
set (except maybe because of Pythons moronic scoping rules).  In any
case it'd better to make clear how that works.

That would be something the lines of the following then:

import csv

people = []
people_ids = set()

def add_person(group_id, new_subscription):
    people.append({
        'id': group_id,
        'subscriptions': [new_subscription]
    })
    people_ids.add(group_id)

#open csv file
with open(filename, 'rb') as file:
    csv_file = csv.reader(file)

    #read CSV file into a list of people
    for record in csv_file:
        sub_id = record[0]
        group_id = record[6]
        season_desc = record[11]
        series_desc = record[12]

        if "MW" in series_desc:
            sub_type = "Masterworks"
        elif "Pops" in series_desc:
            sub_type = "Pops"
        elif "Chamber" in series_desc:
            sub_type = "Chamber"

        new_subscription = {
            'id': sub_id,
            'season': season_desc,
            'sub_type': sub_type
        }

        #if this is the first time through the loop
        if len(people) == 0:
            add_person(group_id, new_subscription)
            continue

        #check if this person has already been recorded
        #if not, add the subscription to them and save the person
        if group_id not in people_ids:
            add_person(new_subscription)
            continue

        #check if this subscription has already been added to this person
        #use enumerate to get a key to search people
        for person in people:
            if person['id'] != group_id:
                person['subscriptions'].append(new_subscription)

I also shuffled the branches around, which together with continue
makes for less indented blocks.

If you're going to extend this, I'd recommend more factoring into functions, a
main function, etc.


C++ Stack implementation with linked-list It's proving difficult to find implementations of a Stack with linked list and Im trying to figure out if my implementation is correct/not missing anything obvious.

template <class T>
class Stack {
private:
  struct node {
    T data;
    node* next = nullptr;
    node(T data, node* next) : data(data), next(next) {};
  };
  node* top_ = nullptr;
public:
  Stack() {}
  Stack(T seed[], size_t len) {
    for(size_t i = 0; i < len; ++i) {
      Push(seed[i]);
    }
  }
  void Push(T value) {
    node* ele = new node { value, top_ };
    top_ = ele;
  }
  bool IsEmpty() {
    return top_ == nullptr;
  }
  T Peek() {
    if (IsEmpty())
      throw std::runtime_error("Stack is empty");
    return top_->data;
  }
  T Pop() {
    if (IsEmpty())
      throw std::runtime_error("Stack is empty");
    node* popped = top_;
    T result = popped->data;
    top_ = popped->next;
    delete popped;
    return result;
  }
};

My tests (sample basic ones below) do show working functionality as well, so its really just my code structure/procedures I'm worried about

int seed[3] { 5,4,3 };
Stack<int> stack { seed, 3 };
REQUIRE( stack.Pop() == 3 );
REQUIRE( stack.Pop() == 4 );
REQUIRE( stack.Pop() == 5 );
REQUIRE_THROWS_WITH( stack.Pop(), "Stack is empty" );
REQUIRE_NOTHROW( stack.Push(9) );
REQUIRE_NOTHROW( stack.Push(3) );
REQUIRE( stack.Peek() == 3 );
REQUIRE( stack.Pop() == 3 );

This is the archaic form of template declaration:

template <class T>

The more modern form is:

template<typename T>

Technically not difference but class implies user defined type while typename implies any type. Most modern libraries will use the newer version. There is also one obscure corner case template templates were it makes a difference.

You don't need to define a constructor for node.

  struct node {
    T data;
    node* next = nullptr;
    node(T data, node* next) : data(data), next(next) {};
  };

The initializer list contruct will have the same affect.

  struct node {
    T data;
    node* next;
  };
  ...
  new node {data, top};

Also I would use standard naming convention. User defined types start with an upper case letter while variables and methods start with a lower case letter. This helps you distinguish between object creation and function calls very easily.

You have missed the destructor. This means a non empty stack will leak all its members when it goes out of scope. So you should define a destructor to cleanup when the object is destoryed.

Now that you have a destructor:

You don't obey the rule of 3 or 5 (somebody else mentioned the rule of 7 but I don't know what that is, if somebody would like to clarify in the comments?).

Rule of 3: If you define a destructor, copy constructor or copy assignment then you probably need all three.

Rule of 5: Rule of 3 + the 2 move operators. Allows you define move operators for your object so that it is move compatible.

The alternative to using the Rule of 3/5 is to use the Rule of 0. This basically means the node object cleans itself up. This would require using std::unique_ptr. The top in your list and the next in the node would need to use this type.

Personally I don't think this is appropriate for building lists (but it is definitely an option and other people would recommend it).

You are passing by value here:

  void Push(T value)

This is fine for simple types of T. But if T is expensive to copy then you copy it to the Push method. Then you copy it again when you construct the object.

Try pushing by reference and moveable reference.

 void Push(T const& value);  // Pass value by reference.
 void Push(T&& value);       // Pass by r-value ref allowing a move.

This function does not change the state of the object.

  bool IsEmpty() {
    return top_ == nullptr;
  }

You should probably mark it as const. This allows you to pass the stack by const reference and still check the state of the object.

DRY you code.
The following code is repeated in two places

    if (IsEmpty())
      throw std::runtime_error("Stack is empty");

That means it is a candidate to be put into its own function.

You can not implement Pop() that returns a value in an exception safe manner. As such the standard stack uses two separate methods top() which returns a reference to the top value and pop() that returns void but removes the top item from the stack.

You should probably follow this convention.

These two functions return by value.

  T Peek()
  T Pop()

This means you make a copy of the value. This is fine for simple types like integer but for complex classes this is potentially expensive. Another reason to use top() and void pop(). But here at least Peek() should return a reference to avoid the copy.


Populate an ArrayList of different instances of a class First of all, here is an overview of my application. I want to create a multiple-choice app, on android, using data stored on a remote MySQL database. The DB has 100 questions that will appear in random order.

My php script returns a JSON array with the following results

[
 {
    "id": 1,
    "Question": " blahblahblahblahblah?"
},
{
    "id": 2,
    "Question": "blehblehblehblehblehblehblehblehbleh?"
},
]

Since the Question table on my database has integers and strings I thought that it would be a better approach to copy the information of the JSON Objects to different instances of a Class Question. I want the JSONArray to be created once in order to avoid multiple requests to the server.That's why I wrote the following code:

public class Question {
    int question_id;
    String question_txt ;
    JSONArray json ;

public int setQuestion_id(int i) {
    try {this.question_id = this.json.getJSONObject(i).getInt("id");}
    catch (JSONException e) {e.printStackTrace();}

    return question_id;
}

public String setQuestion_txt(int i) {
    try {this.question_txt=this.json.getJSONObject(i).getString("Question");}
    catch (JSONException e) {e.printStackTrace();}

    return question_txt;
}

public JSONArray setJson()throws JSONException, IOException {
    java.net.HttpURLConnection connection = null;
    URL url = new URL("url_to_my.php");
    connection = (java.net.HttpURLConnection) url.openConnection();
    connection.setRequestMethod("GET");
    connection.setRequestProperty("Content-length", "0");
    connection.setUseCaches(false);
    connection.setConnectTimeout(0);
    connection.setReadTimeout(0);
    connection.connect();
    int status = connection.getResponseCode();
    JSONArray json = null;

    switch (status) {
        case 200:
        case 201:

            BufferedReader br = new BufferedReader(new InputStreamReader(connection.getInputStream()));
            StringBuilder sb = new StringBuilder();
            String line;

            while ((line = br.readLine()) != null) {
                sb.append(line + "\n");
            }

            this.json = new JSONArray(sb.toString());
            br.close();
    }
    return this.json;

}

}

What I tried doing is creating an ArrayList questionnaire and add to it different instances of the Class Question with different values.

ArrayList<Question> questionnaire = new ArrayList<Question>();
Question q = new Question();

for(int i=0; i< q.setJson().length() ;i++){
    Question newQuestion = new Question();
             newQuestion.json = q.json;
             newQuestion.setQuestion_id(i);
             newQuestion.setQuestion_txt(i);
             questionnaire.add(newQuestion);}

Everything seems to work fine, at least for now. Later on, I want the JSON result to return more information like the possible answers and which is the correct one.

Good effort to get your code working so far. But your code is highly unusual.

You've got the class Question which has an id and question_txt, but each Question also has a reference to a JSONArray of all the questions. Worse than that, for each Question, when you create a new Question and set its reference to all questions, it will come from the API each time.

You've recognised that the behaviour of getting Questions from the API might belong in the Question class (or related class), instead of forcing main (or whatever consumer) to know the details of the API. But it's surely not the responsibility of a single Question instance to fetch all the Questions.

You want the class Question to hold it's data first (and later, some behaviour), so here's a typical data class.

public class Question {

    private int id;
    private String text;

    public Question (int i, String s) {
        this.id = i;
        this.text = s;
    }

    public int getId() {
        return id;
    }

    public String getText() {
        return text;
    }

}

You'll see this type of class referred to  as Java Bean or POJO. In this case, because we've used private and only provided get methods, this is a read-only object after its been created.

Then add some behaviour to the class (meaning we're going to use the static keyword) for retrieving the questions:

public static List<Question> fetchQuestions () throws JSONException, IOException {

    HttpURLConnection connection = (HttpURLConnection) new URL("url_to_my.php").openConnection();
    connection.setRequestMethod("GET");
    connection.setRequestProperty("Content-length", "0");
    connection.setUseCaches(false);
    connection.setConnectTimeout(0);
    connection.setReadTimeout(0);
    connection.connect();
    int status = connection.getResponseCode();
    if (!(status == 200 || status == 201)) {
        throw new IOException("Bad status code: " + status);
    }

    BufferedReader br = new BufferedReader(new InputStreamReader(connection.getInputStream()));
    StringBuilder sb = new StringBuilder();
    String line;
    while ((line = br.readLine()) != null) {
        sb.append(line + "\n");
    }
    br.close();
    String response = sb.toString();

    List<Question> questions = new ArrayList<Question>();
    for (Object arrayItem: new JSONArray(response)) {
        JSONObject questionAsJSON = (JSONObject) arrayItem; // throws on unexpected response format
        questions.add(new Question(questionAsJSON.getInt("id"),
                questionAsJSON.getString("Question")));
    }

    return questions;

}

And then when we want to grab the questions from the API, it's just a case of invoking the static method:

List<Question> questionnaire = Question.fetchQuestions();


Dynamic attribute key selector I'm basically trying to build a function that searches all nodes on the DOM to check for data attributes, then swap them out with whatever is stored with the "data-intl-" attribute. For example, if someone had data-intl-attr, whatever attr is on, that node would get replaced by what's in data-intl-attr.

The code works; I just want to know if I could have approached it differently.

var all = $('*');

$.map(all,function(el) {

    filter($(el).data(), function(clean){
      var originalAttrVal = $(el).attr(clean);
      var intlAttr = $(el).attr("data-intl-" + clean);

      $(el).attr(clean, intlAttr);

    });

});

function filter(foo, cb){
        var capture;
    for(var i in foo) {
      if (foo.hasOwnProperty(i)) {
        i = i.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase();
        i = i.replace(/intl-/g, '');
        capture= i;
     }
     cb(capture);
  }

}

Input HTML:

test href

Output HTML:

test href

Naming Conventions

The variable all doesn't tell what it contains. allElements does reflect what it contains.

clean, i, foo, capture - huh?

filter, the name of the function has nothing to do with filtering. It is iterating over some data and calling provided callback for each property.
Global Variables

all is global variable. Global variables is bad practice. Please see Why are global variables considered bad practice? and I've Heard Global Variables Are Bad, What Alternative Solution Should I Use?.
Using Proper Methods

map is used to update each element in collection. Here, map is used just to iterate over the elements. You can use each, for, forEach, etc. in such case.
Diving into DOM

In filter callback, you're diving into DOM three times to get the same element. The element can be cached and reused.
Method Chaining

replace() in filter() can be chained.
Unnecessary code

foo.hasOwnProperty(i) is not required as .data() only returns data-* properties in an object.
.toLowerCase() can be safely removed and i flag on the next replace regex can be used.

Also, HTML is case insensitive. No transformation is needed when changing attribute.

That is too much code for such a small functionality. I've rewritten the code. See the explanation in the code comments.

// Select all elements in DOM
// and iterate over them
$('*').each(function() {
  // Cache the element reference
  // $(this) refers to the current element in the collection
  var $this = $(this);

  // Get the key-value of all `data-*` attributes
  // on current element
  var data = $this.data();

  // Get all the keys of data-attributes
  var keys = Object.keys(data);

  // Check if this element have data-* attribute on it
  if (keys.length) {

    // Iterate over each data attribute
    keys.forEach(function(key) {

      // Check if attribute starts with `intl` followed by Uppercase character
      if (/^intl[A-Z]/.test(key)) {
        // Get the attribute name
        // As the attribute name is camelCase, last word is the attribute name
        var attribute = key.match(/[A-Z][a-z]*$/)[0];

        // Update the attribute in DOM
        $this.attr(attribute, data[key]);
      }
    });
  }
});

test href