Parses XML to return the bus times at specified bus stop The code uses the XML-based OC Transpo data feed to create a list of the bus name, where it's headed, and the times. Keep in mind that I am a beginner at python so any advice at all is appreciated.

import subprocess, pprint
from bs4 import BeautifulSoup

def format_set(result_set):
    new_set = []
    for el in result_set:
        new_set.append(str(el.get_text()))
    return new_set

def get_stop_number():
    print('Please enter your desired stop number (or \'quit\'):')
    stop_numb = raw_input('> ')
    try:
        return int(stop_numb)
    except:
        print('Exiting...')
        return 'quit'

def get_stop_info(stopNo):
    try:
        output = subprocess.check_output(('curl -d "appID=ba91e757&apiKey=…'
            '&stopNo={}&format=xml" https://api.octranspo1.com/v1.2/GetNextTripsForStopAllRoutes').format(stopNo), shell=True)
        soup = BeautifulSoup(output, 'xml')
    except:
        print('An error occured!')
        return None
    summary = []
    for el in soup.find_all('Route'):
        routeNo = int(el.find('RouteNo').get_text())
        routeHeading = str(el.find('RouteHeading').get_text())
        times = format_set(el.find_all('TripStartTime'))
        x = [routeNo, routeHeading, times]
        summary.append(x)
    return summary

def is_empty(any_structure):
    if any_structure:
        return False
    else:
        return True

if __name__ == '__main__':
    while True:
        stop_number = get_stop_number()
        if stop_number == 'quit':
            break
        summary = get_stop_info(stop_number)
        pprint.pprint(summary)

Sample output:

Please enter your desired stop number (or 'quit'):
> 3058
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7756  100  7679  100    77   4654     46  0:00:01  0:00:01 --:--:--  4656
[[92, 'St-Laurent', ['11:30', '12:00', '12:30']],
 [92, 'Stittsville', []],
 [96, 'St-Laurent', ['10:51', '11:21', '11:51']],
 [96, 'Terry Fox', ['10:09', '10:39', '11:09']],
 [118, 'Hurdman', ['11:20', '11:40', '12:00']],
 [118, 'Kanata', []],
 [162, 'Stittsville', ['11:55', '12:55', '13:55']],
 [162, 'Terry Fox', []],
 [168, 'Bridlewood', ['11:35', '12:05', '12:35']],
 [168, 'Terry Fox', []]]
Please enter your desired stop number (or 'quit'):
> quit
Exiting...

Imports are okay, you'll find different advice about whether to import
individual functions, so I'll say no more.
You have __name__ ..., good, perhaps just move the contained block
into another function main so that you can easily invoke it from the
REPL.
is_empty is obsolete, at some point you'll just remember that empty
lists are False in a boolean context.  Oh and it's unused as well.
You have a lot of quoting options in Python, so if you need a literal
' in a string, consider delimiting it with " instead of using the
backslash syntax.
You're in Python, so perhaps also use the built-in HTTP library
instead of shelling out to curl.
format_set is more easily written with a list comprehension, e.g.:

def format_set(result_set):
    return [str(el.get_text()) for el in result_set]

Technically get_stop_number accepts more than just numbers and
'quit', so just be aware of that.  I'd inline the stop_numb
variable as well (n.b. why care about saving two characters there?)

...
return int(raw_input('> '))

get_stop_info has a camel-case variable s, that's inconsistent with
the rest and in general not advised in Python.  Again, the loop can be
a list comprehension, but maybe that'll be a bit too dense, your
choice.  In general returning an unstructured list like is will also
prevent the reader from getting a clearer understanding of the code,
but if it's just for display purposes that's fine.

...
return [[int(el.find('RouteNo').get_text()),
         str(el.find('RouteHeading').get_text()),
         format_set(el.find_all('TripStartTime'))]
        for el in soup.find_all('Route')]

I'm a fan of aggressively factoring out common code, you can try and
see how you like e.g. creating smaller functions for
int(el.find(name).get_text()) and so on; that can be helpful later
on as your program(s) grow(s).


Implement a boto3 RDS generator in Python Boto3 does not seem to implement a generator for RDS instances, instead offering a marker and pagination feature. This code does work for my use case and I have been using it for the entire day without any problems; however, to me, not returning anything in a while True loop seems magically dangerous, it works but leaving it here for general criticism, it is also the first time I implement a generator in python and feel a little insecure about it.

def all_rds_instances(config):
    """
    Gets all the RDS instances in a generator (lazy iterator) so you can implement it as:
    `for instance in all_rds_instances(config):`
    """
    marker = ""
    client = boto3.client('rds', region_name=config["region"])
    pool = []

    # min 20, max 100
    page_size = 20
    while True:
        if len(pool) < 1:
            if marker is None:
                break
            # populate a local pool of instances
            result = client.describe_db_instances(MaxRecords=page_size, Marker=marker)
            marker = result.get("Marker")
            pool = result.get("DBInstances")

        if len(pool) > 0:
            this_instance = pool.pop()
            yield this_instance

A sample implementation follows:

for instance in all_rds_instances({"region": "eu-west-1"}):
    print instance["Endpoint"]["Address"]

Your code is a little hard to read, mostly due to you moving what should be an inner for loop, to be two ifs in an infinite while loop.
If you were to instead for loop over pool, then you could make the code easier to understand.

I'd also make page_size be a keyword argument, so the user can change the page size if they wish. And I'd change config to region, as it makes little sense to me, why you're passing a dictionary to extract a single key.

def all_rds_instances(region, page_size=20):
    """
    Gets all the RDS instances in a generator (lazy iterator) so you can implement it as:
    `for instance in all_rds_instances(region):`

    page_size [explain what this does] should be bound between 20 and 100.
    """
    client = boto3.client('rds', region_name=region)
    marker = ""
    pool = []
    while True:
        for instance in pool:
            yield instance

        if marker is None:
            break
        result = client.describe_db_instances(MaxRecords=page_size, Marker=marker)
        marker = result.get("Marker")
        pool = result.get("DBInstances")

This makes reading the code easier, you know that it will exhaust the pool straight away when reading it, and it makes understanding how you infinitely loop a little easier too.


My database class extends MySQLi The following is a small class I extended MySQLi with.  I'm going to use this in my upcoming projects, but my main reason to do this class is learning, so I would like to submit this for your review, tell me if there are any bugs or improvements to be made.

NOTES:

I know about prepared statements, but I am not going to only run the same queries with different parameters, I'm expecting very different queries in each page, So I don't want to use prepared statements. Don't suggest prepared statements.
I am aware of PDO, but I will use MySQL only, so I'm better with MySQLi. So please don't suggest PDO.

class Dbquery extends MySQLi
{
    public $host,$user,$password,$database,$connection;
    public function __construct($host,$user,$password,$database)
    {
        $this -> host = $host;
        $this -> user = $user;
        $this -> password = $password;
        $this -> database = $database;
        $this -> connect_me();
    }
    private function connect_me()
    {
        $this -> connection = $this-> connect($this->host,$this->user,$this->password,$this->database);
        if( $this -> connect_error )
            die($this->connect_error);
    }
    private function extracts($data) // function used to append data and column names for insert query
    {
        $column = array("","");
        foreach ($data as $index => $details)
        {
            $column[0].= $index.",";
            $column[1].= "'".$this ->real_escape_string($details)."',";
        }
        $column[0] = rtrim($column[0],",");
        $column[1] = rtrim($column[1],",");
        return $column;
    }
    private function append($data) // function used to append data and column names for update query
    {
        $string = "";
        foreach($data as $index => $details)
        {
            $string.= $index."='".$this -> real_escape_string($details)."',";
        }
        $string = rtrim($string,",");
        return $string;
    }
    public function insert($table,$data)
    {
        $extracted = $this -> extracts($data);
        $this -> query("INSERT INTO ".$table." (".$extracted[0].") VALUES(".$extracted[1].")");
        if($this -> error)
            return $this -> error;
        else
            return $this -> insert_id;
    }
    public function update($table,$data,$where)
    {
        $extracted = $this -> append($data);
        $this -> query("UPDATE ".$table." SET ".$extracted." WHERE ".$where);
        if($this -> error)
            return $this -> error;
    }
    public function select($table,$column = "",$where = 1,$orderby = "",$limit = "")
    {
        if($column == "")
            $column='*';
        if($orderby == "")
            $orderby = 'NULL';
        if($limit != "")
        {
            $limit = "LIMIT ".$limit;
        }
        $result = $this -> query("SELECT ".$column." FROM ".$table." WHERE ".$where." ORDER BY ".$orderby."".$limit."");
        if($this -> error)
        {
            return $this -> error;
        }
        else
        {
            $index = 0;
            while($row = $result -> fetch_assoc())
            {
                $data[$index] = $row;
            }
            return $data;
        }
    }
    public function delete($table,$where)
    {
        $this -> query("DELETE FROM ".$table." WHERE ".$where."");
        if($this -> error)
        {
            return $this -> error;
        }
        else
        {
            return true;
        }
    }
}

Note: I don't think this is what you want to hear. But I have to write "the truth". This is my personal opinions about your code.
Give me parameterized SQL or give me death!

tell me if there are any bugs or improvements to be made.

Don't suggest prepared statements.

I'm sorry, but those two requests really don't fit together in the same Code Review question.
I understand what you're doing here, and it's nice that you're doing it for learning purposes. It's nice to be able to extend the mysqli class in PHP.
However, I wouldn't recommend using the methods you have in this class. As you say:

I'm expecting very different queries in each page

Consider then: What functionality does this class provide you with? How much code does it save for you? Will it make your code more readable?
As I see it, there are primarily four lines that summarize what your class is all about:
$this -> query("INSERT INTO ".$table." (".$extracted[0].") VALUES(".$extracted[1].")");
$this -> query("UPDATE ".$table." SET ".$extracted." WHERE ".$where);
$result = $this -> query("SELECT ".$column." FROM ".$table." WHERE ".$where." ORDER BY ".$orderby."".$limit."");
$this -> query("DELETE FROM ".$table." WHERE ".$where."");

That is, you're essentially making it a little "easier" to write the SQL queries.
In your comment you ask if you can run different queries each time using prepared statements, and if you can use joins and stuff. The answer to these questions is: YES! Is it efficient? YES!. We're often talking milliseconds of difference between prepared statements and non-prepared statements (except when you perform the same prepared statement multiple times with different parameters, in which a prepared statement performs significantly faster!). If you are worried about the speed of prepared statements, don't be. They're fast. If they're slow, then there's something wrong with your database structure or your query, not with the fact that you're using prepared statements.
Here is what I personally think:

Other developers reading your code will be confused as they are not familiar with your Dbquery class
By not using your class you will be more experienced with writing SQL statements, it will probably go faster writing the SQL statements than you think.
Your class does not provide that much functionality. In fact, it can act as a restriction as writing something like select("mytable LEFT JOIN table2 ON (mytable.a = table2.b)", "table2.c, mytable.a", "mytable.a > 4", "table2.c", 5) just feels completely strange, wrong, and hard to remember what the parameters are, and you'll likely end up trying to reconstruct the resulting SQL query in your mind anyway (or is that just me?)

The point I'm trying to make here is: If you expect "very different queries in each page", then write those queries in each page. As long as they're not entirely dynamic queries, use prepared statements! Prepared statements are the way to go in the "real" PHP world.

I am aware of PDO, but I will use MySQL only, so I'm better with MySQLi. So please don't suggest PDO.

Sure, I have nothing against MySQLi myself. That's also what I tend to use when working in PHP. I think the choice of PDO vs. mySQLi is mostly opinion-based. The main advantage of PDO is that it can work with other databases except mySQL, but as you will use mySQL only, it's perfectly fine in my opinion to use mySQLi. At least you're not using the old mySQL functions!


YUV422 to RGB24 using Intel Performance Primitives I have a C++ method that uses the Intel performance primitives library to convert an image from YUV422 format into RGB 24. I am PInvoking this method from C# (WPF) where the YUV images are coming from a high frame rate camera. The method takes a pointer to the image data, and the width and height and returns a pointer to the modified data.

This single method is taking up 23% of my entire application CPU and must be running at all times (the video is always visible).

I am not a C++ maestro and I am sure that this code could be improved significantly in terms of performance, but I am not sure where to start.

Any suggestions for improvements would be most appreciated

extern "C" __declspec(dllexport)unsigned char* YUV422ToRGB24(unsigned char* input, int width, int height)
    {
        const int       ALIGNMENT_64BIT_MASK = 64u;
        int             stepRGB, stepYUV;

        Ipp8u           *rgbImg, *imgYUV422_IPP;

        bool            success = false;
        IppiSize        size;

        UINT32 width3 = width * 3;
        UINT32 width2 = width * 2;

        unsigned char *ptr = NULL;

        unsigned char* pBGR = new unsigned char[(width * height) * 3];

        // ippi functions require 32Bit alignment, if not a

        if ((((uintptr_t)pBGR % ALIGNMENT_64BIT_MASK) == 0) && (width3 % ALIGNMENT_64BIT_MASK == 0))
        {
            rgbImg = pBGR;
            stepRGB = width3;
        }
        else
        {
            rgbImg = ippiMalloc_8u_C3(width, height, &stepRGB);
            if (rgbImg == NULL)
                return false;
        }

        if ((((uintptr_t)input % ALIGNMENT_64BIT_MASK) == 0) && (width2 % ALIGNMENT_64BIT_MASK == 0))
        {
            imgYUV422_IPP = input;
            stepYUV = width2;
        }
        else
        {
            imgYUV422_IPP = ippiMalloc_8u_C2(width, height, &stepYUV);

            if (imgYUV422_IPP == NULL)
            {
                ippiFree(imgYUV422_IPP);
                return false;
            }

            // copy source to yuv
            ptr = imgYUV422_IPP;

            // simple case where step is compatable, can copy entire row
            if (stepYUV == width2) {
                memcpy(ptr, input, height * stepYUV);
            }
            else {
                for (int col = 0; col < height; ++col)
                {
                    memcpy(ptr, input, width2);
                    input += width2;
                    ptr += stepYUV;
                }
            }
        }

        // convert yu422 to rgb
        size.width = width;
        size.height = height;

        if (ippiYCbCr422ToBGR_8u_C2C3R(imgYUV422_IPP, stepYUV, rgbImg, stepRGB, size) == 0)
        {
            // ouptut destination not same location as result, so copy over memory
            if (pBGR != rgbImg) {
                // copy memory to destination
                ptr = rgbImg;

                // simple case where step is compatable, can copy entire row
                if (stepRGB == width3) {
                    memcpy(pBGR, ptr, height * stepRGB);
                }
                else {
                    for (int col = 0; col < height; ++col) {
                        memcpy(pBGR, ptr, width3);
                        pBGR += width3;
                        ptr += stepRGB;
                    }
                }
            }
            success = true;
        }

        // only free memory if not same as pointers provided
        if (pBGR != rgbImg)         ippiFree(rgbImg);
        if (input != imgYUV422_IPP) ippiFree(imgYUV422_IPP);

        return pBGR;
    }

If your function is called with non-aligned memory, it will allocate two buffers, copy, do the simple fast thing, copy again, and free. That’s a whole lotta work for something simple and fast. And if the step sizes don’t match, the copy will be even slower.

If this the normal execution case, I can imagine this being a significant drain on resources.

You are processing video. Each frame will have the same size. You can keep the internal buffers in this function around from call to call, so you can reuse them and avoid the mallocs.

But even better would be to directly read your video into aligned memory, in the right format for the IPP library. Your functions should just error out if the input pointers are not aligned.

Furthermore, the output data is stored in memory newed. You should pass a data pointer from the caller to write in. If you don’t want to do that, why not directly allocate this output buffer with the right alignment?

I would recommend skipping all the step computations and worries, simply setting

size.width = width * height;
size.height = 1;

If the IPP function believes all pixels are on a single row, then you don't need those padding bytes at the end of odd-sized rows.

There is a bug: In the case that malloc fails, (imgYUV422_IPP == NULL), you free a NULL pointer. Here you probably intended to free the rgbImg buffer if it was allocated earlier.

Similarly, pBGR is never freed on error.


Moving To Object Orientated Programming Please keep in mind I am new and still learning when reading the following.

What I am doing

I have the following code which pulls a sport, tournament and round NR, from a DB table called event where the event is still active.

The Problem

The code works and does what I want it to do, but my problem is looking at the code makes me sick, I know there are more efficient ways to achieve what I am trying to do but I am unsure of where to start to improve the code below, I would like to move to a more object orientated or at least a more efficient way of coding.

I would appreciate it if one of the more experienced members of the community could give the code a look and provide some pointers.

$date = date('Y-m-d');
//get sport & tournament
$sql = "Select distinct sport, tournament, round FROM event WHERE date > $date AND active = 'y'";
$result = mysqli_query($conn,$sql) or die(mysqli_error($conn));
while($row = mysqli_fetch_array($result)){
    $sport[] = $row['sport'];
    $tournament[] = $row['tournament'];
    $round[] = $row['round'];
}
//make form with tournament & sport
?>
<form name="select" name="sport" method="post">
<!--GET SPORT ON SELECT  -->
<select name="sport">
<?php
//get sport
foreach($sport as $index => $sportCode){
    echo '<option value="'. $sport[$index].'">'.$sport[$index].'</option>';
}
?>
</select>
<!--GET TOURNAMENT ON SELECT  -->
<select name="tournament">
<?php
//get tournament
foreach($tournament as $index => $tournamentCode){
    echo '<option value="'. $tournament[$index].'">'.$tournament[$index].'</option>';
}
//get round
?>
</select>
<select name="round">
<?php
foreach($round as $index => $roundNr){
    echo '<option value="'. $round[$index].'">'.$round[$index].'</option>';
}
//get round
?>
</select>
</form>

TL;DR: Read about MVC and understand the basics and then learn laravel

This is not about object oriented approach but more like a separation of concerns thing. You have two concerns for the moment:

You're collecting some data
You're displaying it

so what you have to do is to separate them, start by using a template system like twig or smarty. Collect the data in some php and pass it to the template file and render it so the html code and php code will be separated.

The template will be responsible of "how" you display the data and your php file will be responsible of "preparing the data" for the template.

Second step is to separate the code that is "dealing with the data" from the code that is "setting up the template". You can move the data related code to some class so that you'll have smt. like this in the end:

<?php

// Require classes, setup things etc.

$obj = new Events;
$events = $obj->getAllEvents();

echo $tpl->render('events/index.tpl', array('events' => $events));

Third step may be about the // Require classes, setup things etc. part in the example above. You can use a front-controller to deal with that...

... and the list goes on...

After some struggling you'll start to realize that you need some helper classes to deal with the http requests, responses, sessions etc.

...and you'll need more and more "concerns" to "separate" in the future and in the end you'll understand why people are using mature frameworks these days.

I was writing like this in 1997-1998 (no kidding) because I had to, but you don't :)


Writing/reading data structure to a file using C++ I wrote some piece of code which reads and write multiple data structures on a file
using C++. I would be grateful to get your feedback on what you think about the code (it works at least when I tested). Thanks.

#include "stdafx.h"
#include <iostream>
#include <fstream>
#include <sstream>
using namespace std;

// data structure to be written to a file
struct WebSites
{
    char SiteName[100];
    int Rank;
}s1,s2,s3,s4;

int _tmain(int argc, _TCHAR* argv[])
{
    strcpy(s1.SiteName, "www.ppp.com");
    s1.Rank = 0;

    strcpy(s2.SiteName, "www.rrr.com");
    s2.Rank = 111;

    strcpy(s3.SiteName, "www.code.com");
    s3.Rank = 123;

    strcpy(s4.SiteName, "www.yahoo.com");
    s4.Rank = 14;

    // write
    fstream binary_file("c:\\test.dat",ios::out|ios::binary|ios::app);
    binary_file.write(reinterpret_cast<char *>(&s1),sizeof(WebSites));
    binary_file.write(reinterpret_cast<char *>(&s2),sizeof(WebSites));
    binary_file.write(reinterpret_cast<char *>(&s3),sizeof(WebSites));
    binary_file.write(reinterpret_cast<char *>(&s4),sizeof(WebSites));
    binary_file.close();

    // Read data
    fstream binary_file2("c:\\test.dat",ios::binary|ios::in| ios::ate );
    int size = binary_file2.tellg();
    for(int i = 0; i<size/sizeof(WebSites); i++)
    {
        WebSites p_Data;
        binary_file2.seekg(i*sizeof(WebSites));
        binary_file2.read(reinterpret_cast<char *>(&p_Data),sizeof(WebSites));

        cout<<p_Data.SiteName<<endl;
        cout<<"Rank: "<< p_Data.Rank<<endl;
    }
    binary_file2.close();

    return 0;
}

The trouble with writing binary blobs is that they lead to brittle storage.

The stored objects have a tendency to break over time as the assumptions you make about the hardware no longer hold true (in this case that the sizeof(int) is constant and the endianess of int will not change).

It has become more standard therefore to use a method know as serialization. In this you convert the object to a format that is hardware agnostic (and usually human readable).

Note: Binary blobs have advantages. But you must weigh those against the brittleness. Therefore your first choice should be serialization (unless you have specific requirements that prevents this). Then look at binary Blobs only after you have shown that serialization has too much overhead (unlikely for most situations, but it is a possibility).

In C++ you would do this using the operator<< and operator>>.

I would re-write your code as:

struct WebSites
{
    std::string siteName;
    int rank;

    WebSite()
        : siteName("")
        , rank(0)
    {}

    WebSite(std::string const& siteName, int rank)
        : siteName(siteName)
        , rank(rank)
    {}
    void swap(WebSites& other) throws()
    {
        std::swap(rank,     other.rank);
        std::swap(siteName, other.siteName);
    }
};
std::ostream& operator<<(std::ostream& stream, WebSites const& data)
{
    stream << data.rank << " "
           << data.siteName.size() << ":"
           << data.siteName;
    return stream;
}
std::istream& operator>>(std::istream& stream, WebSites& data)
{
    WebSite     tmp;
    std::size_t size;
    if (stream >> tmp.rank >> size)
    {
        tmp.siteName.resize(size);
        if (stream.read(tmp.siteName[0], size)
        {
            data.swap(tmp);
        }
    }
    return stream;
}

Now you can write your code like this:

int _tmain(int argc, _TCHAR* argv[])
{
    WebSite s1("www.ppp.com", 0);
    WebSite s2("www.rrr.com", 111);
    WebSite s3("www.code.com", 123);
    WebSite s4("www.yahoo.com", 14);

    // write
    fstream binary_file("c:\\test.dat",ios::out|ios::binary|ios::app);
    binary_file << s1 << s2 << s3 << s4;
    binary_file.close();

    // Read data
    fstream binary_file2("c:\\test.dat",ios::binary|ios::in| ios::ate );
    WebSites p_Data;
    while(binary_file2 >> p_Data)
    {
        cout << p_Data.SiteName << endl;
        cout << "Rank: "<< p_Data.Rank << endl;
    }
    binary_file2.close();

    // Read data into a vector
    fstream binary_file3("c:\\test.dat",ios::binary|ios::in| ios::ate );
    std::vector<WebSites> v;

    std::copy(std::istream_iterator<WebSites>(binary_file3),
              std::istream_iterator<WebSites>(),
              std::back_inserter(v)
             );
    binary_file3.close();
}


Items derived from one function are getting printed within another for loop in another function I have written some code in python to scrape some items from a webpage. There is a link (titling see more) attach to each container. If I click on that link I can reach a certain page where all the information is available. It is quite convenient to track the see more link and parse all documents from there. However, my goal is to parse the first two documents from the first page and going on to the other page (clicking on the see more button) I'll parse the rest. I have tried to do the same as I described just now. It is doing just fine.

At this point, I'm seriously dubious whether the way I'm doing things is the right one or error-prone because I used two items from earlier page to get printed within the newly defined for loop created in the later function. It is giving me the accurate results though. Any suggestion as to what I did here is ideal or any guidance why I should not practice this will be highly appreciated.

This is the site link. This is the full script:

import requests
from urllib.parse import urljoin
from bs4 import BeautifulSoup

url = 'replace_with_the_above_link'

def glean_items(main_link):
    res = requests.get(main_link)
    soup = BeautifulSoup(res.text,"lxml")
    for item in soup.select('.course-list-item'):
        event = item.select(".lead")[0].text
        date = item.select(".date")[0].text
        link = urljoin(main_link,item.select(".pull-right a")[0]['href'])
        parse_more(event,date,link)

def parse_more(event_name,ev_date,i_link): ## notice the two items (event_name,ev_date)
    res = requests.get(i_link)
    soup = BeautifulSoup(res.text,"lxml")
    for items in soup.select('.e-loc-cost'):
        location = ' '.join([' '.join(item.text.split()) for item in items.select(".event-add")])
        cost = ' '.join([' '.join(item.text.split()) for item in items.select(".costs")])
        print(event_name,ev_date,location,cost) ##again take a look: I used those two items within this newly created for loop.

if __name__ == '__main__':
    glean_items(url)

First of all, usual things (I feel like I'm suggesting these things in most of the web-scraping related discussions):

initialize session as requests.Session() and use session.get() instead of requests.get() - this would speed things up because the underlying TCP connection will be re-used for subsequent queries to the same domain
use SoupStrainer to limit what is parsed when the "soup" object is initialized

And, I am not completely sure about the reliability of the .pull_right a selector. At this point, it reads that you want to follow the link that is on the right in its respective container. The position of the element on the screen may easily change with a design change. What about a selector like a[id*=ReadMore] or .more > a?
Also, note that select()[0] could be replaced with select_one().
Code Style
There is not a lot of things to point out since the code is rather short and straightforward, but there are some PEP8 violations that can be addressed:

watch for the use of blank lines
you miss spaces between the function arguments
url should be named URL, since it is a "constant"
use a single # for comments
group imports properly

Some of the variable names could be more explicit, e.g. event_date in place of ev_date, event_link instead of i_link.
The improved code
from urllib.parse import urljoin

from bs4 import BeautifulSoup, SoupStrainer
import requests

URL = 'replace_with_the_above_link'

def glean_items(session, main_link):
    response = session.get(main_link)

    parse_only = SoupStrainer(class_='course-list-item')
    soup = BeautifulSoup(response.text, "lxml", parse_only=parse_only)
    for item in soup.select('.course-list-item'):
        event = item.select_one(".lead").text
        date = item.select_one(".date").text
        link = urljoin(main_link, item.select_one(".pull-right a")['href'])

        parse_more(session, event, date, link)

def parse_more(session, event_name, event_date, event_link):
    response = session.get(event_link)

    parse_only = SoupStrainer(class_="e-loc-cost")
    soup = BeautifulSoup(response.text, "lxml", parse_only=parse_only)
    for items in soup.select('.e-loc-cost'):
        location = ' '.join([' '.join(item.text.split()) for item in items.select(".event-add")])
        cost = ' '.join([' '.join(item.text.split()) for item in items.select(".costs")])

        print(event_name, event_date, location, cost)

if __name__ == '__main__':
    with requests.Session() as session:
        glean_items(session, URL)


Web-service for wiki-like movie database I'm making a web-service for an app with a (partially) crowdsourced movie database.

These two functions allows users to add and remove which genres a movie is liked to, and get the log of whom have done what changes.

I need to make many very similar functions for other data.

    [HttpPost]
    [Authorize(Roles = "User")]
    public IHttpActionResult PostMovieGenre([FromBody]MovieGenreBindingModel edit)
    {
        string[] IDs = edit.genre_ids.Split(',');

        using (var db = new DBEntities())
        {
            var movie = db.movies.Find(edit.movie_id);

            if (movie == null)//Confirm that the movie was found
                return BadRequest("Movie not found");

            foreach (string sid in IDs)
            {
                int g_id = Convert.ToInt32(sid);
                if (edit.action == "add")
                {
                    movie.movies_genres.Add(new movies_genres
                    {
                        movies_id = edit.movie_id,
                        genres_id = g_id
                    });
                }
                else if (edit.action == "remove")
                {
                    var m_g = movie.movies_genres.Where(g => g.genres_id == g_id).First(); //Find the movies_genres object
                    db.movies_genres.Remove(m_g); //Remove it
                }
                else
                {
                    return BadRequest("Invalid action");
                }

                db.movies_genres_changes.Add(new movies_genres_changes //Log the change
                {
                    action = edit.action,
                    genres_id = g_id,
                    movies_id = edit.movie_id,
                    time = DateTime.UtcNow,
                    user_id = UserManager.FindByName(User.Identity.Name).Id
                });
            }
            try
            {
                db.SaveChanges();
            }
            catch (Exception e)
            {
                return BadRequest();
            }
        }
        return Ok();
    }

    [ResponseType(typeof(List<RMovieGenresChange>))]
    [HttpGet]
    [Authorize(Roles = "User")]
    public IHttpActionResult GetMovieGenresChanges(int movie_id)
    {
        List<RMovieGenresChange> retchanges = new List<RMovieGenresChange>();

        using (var db = new DBEntities())
        {
            try
            {
                var dbRevs = db.movies.Find(movie_id).movies_genres_changes;

                if (dbRevs == null)
                    return BadRequest("No revision for an movie entry with id " + movie_id);

                foreach (movies_genres_changes m in dbRevs)
                {
                    retchanges.Insert(0, new RMovieGenresChange() // Put newest change first in list
                    {
                        action = m.action,
                        genre = new RGenre
                        {
                            id = m.genres_id,
                            name = m.genres.name
                        },
                        movie_id = m.movies_id,
                        time = m.time,
                        user = new RUserShort
                        {
                            user_id = m.user_id,
                            user_name = User.Identity.Name
                        }
                    });
                }
            }
            catch (Exception e)
            {
                return BadRequest();
            }
        }

        return Ok(retchanges);
    }

A general comment: Extract logic to small private methods. Note that your methods are long which makes it hard to understand what they do. Every independent piece of code should be extracted into a private method. Like

private void RemoveMovie(id)

If you use the Add method on a List object it will insert the first item into the first place the second to the second and so on. No need in retchanges.Insert(0, new RMovieGenresChange()

GetMovieGenresChanges is a good method since it does only one thing unlike PostMovieGenre method which handles both the addition and the removal of the geners. This is a violation of the Single Responsibility Principle and hence you should split it to two methods: one for the addition AddGenereToMovie and another for the removal - RemoveGenereFromMovie.
This way yo keep your methods short, clean, readable and maintainable.


A practice shared_ptr implementation I am trying to build a class similar to std::shared_ptr just to learn and improve my way of thinking. I am hoping that you can tell me if this implementation is correct? If so, how can I improve it?
Here is the code:
#include <iostream>
#include <map>

template<typename dataType>
class shared_ptr
{
public:
    shared_ptr()
    {
        id++;
        internalId = id;
        countMap[id] = 1;
        ptr = new dataType();
    }
    shared_ptr(shared_ptr& a)
    {
        this->ptr = a.ptr;
        this->internalId = a.internalId;
        countMap[internalId]++;
    }

    void operator=(shared_ptr& a)
    {
        this->ptr = a.ptr;
        this->internalId = a.internalId;
        countMap[internalId]++;
    }

    ~shared_ptr()
    {
        countMap[internalId]--;
        if(countMap[internalId]==0)
        {
            delete ptr;
            countMap.erase(internalId);
        }
    }

private:
    static int id;
    int internalId = 0;
    static std::map<int,int> countMap;
    dataType* ptr;
};
template<typename  dataType>
 int shared_ptr<dataType>::id = 1;
 template<typename  dataType>
 std::map<int,int> shared_ptr<dataType>::countMap;

int main()
{
    {
    shared_ptr<int> sp1;
    {
        shared_ptr<int> sp3;
        shared_ptr<int> sp4(sp3);
        shared_ptr<int> sp5;
        sp5 = sp4;
    }
    shared_ptr<int> sp2(sp1);
    }
    return 0;
}

Design review
Trying to roll your own shared_ptr is actually a really good practice project. This is not true for many other standard library components, but it is true for shared_ptr. You could try to duplicate the behaviour of std::shared_ptr exactly… but you don’t need to, because there are actually many, many different smart pointer designs. The ones in the standard library are just nice general-purpose smart pointers, but it’s very common to see custom smart pointers in libraries built for specific purposes.
Your shared_ptr actually has one important benefit over std::shared_ptr: it’s usually going to be half the size. The reason for that is std::shared_ptr uses a control block to keep track of how many shared pointers share the same pointer (among other things), so in a std::shared_ptr, there’s usually the actual pointer, and then the pointer to the control block… that’s two pointers, so double the size of a single pointer. You use a static map instead, so each of your shared pointers only holds the actual pointer. That could be an important benefit.
However, you also truck around an int as an ID. Is that necessary? Why not just use the pointer value itself as the key?
Something like so:
std::unordered_map<std::uintptr_t, std::size_t> count_map;

template <typename T>
class shared_ptr
{
public:
    constexpr shared_ptr() noexcept = default;

    explicit shared_ptr(T* p) : _ptr{p}
    {
        _increment_count();
    }

    shared_ptr(shared_ptr const& other) : _ptr{other._ptr}
    {
        _increment_count();
    }

    ~shared_ptr()
    {
        if (_ptr)
        {
            // find the iterator to the count
            //
            // this is better than `count_map[n]` because we might need to use
            // the iterator more than once: once to decrement and check the
            // count, and possibly once more to erase
            auto const n = static_cast<std::uintptr_t>(_ptr);
            if (auto const p = count_map.find(n); p != count_map.end())
            {
                // decrease the count, and see if it's zero
                if (--(p->second) == 0)
                {
                    delete _ptr;

                    // remove the map entry tracking the pointer
                    count_map.erase(p);
                }
            }
            else
            {
                // something went really wrong!
                //
                // you should never get here unless there's a bug somewhere
            }
        }
    }

    auto operator=(shared_ptr const& other) -> shared_ptr&
    {
        auto temp = other;

        std::ranges::swap(_ptr, temp->_ptr);

        return *this;
    }

    // other stuff...

private:
    auto _increment_count() -> void
    {
        if (_ptr)
        {
            // convert the pointer to a number
            auto const n = static_cast<std::uintptr_t>(_ptr);

            // add the number as a new key in the count map
            //
            // note that if the key already exists, this will just increment the
            // count, so your shared_ptr will safely work with duplicates...
            // something std::shared_ptr won't do!
            ++(count_map[n]);
        }
    }

    T* _ptr = nullptr;
};

By using the pointer value itself as the key, rather than an int ID, your shared_ptr can now be the size of a single raw pointer.
So your design—using an external map to keep track of the count—has some benefits: notably it is now possible to make your smart pointer the same size as a raw pointer. But, of course, it has some drawbacks:

If you have a lot of shared_ptrs, that map can get huge.
Copying and destroying your shared_ptrs will be slow. std::shared_ptr has the pointer to the control block right on hand, so it just needs to do a single pointer indirection. Especially for making copies (and increasing the count), that’s fast (relatively speaking). Your shared_ptr has to search through a map (which is probably not going to be hot in cache) for each operation. That means that while auto sp1 = sp2; is really fast for std::shared_ptr it will be slow for your shared_ptr (and will get slower the more pointers you have). But that’s a pretty standard engineering size-versus-speed trade-off; your shared_ptr takes up less space, at the cost of speed.

Testing
Rather than simply wondering whether your code works or not, you should test it.
Now, something like shared_ptr is not easy to test… but not impossible. As a start, you could run your shared_ptr through all of its operations, and use a tool like Valgrind to check for bad memory access or leaks. Actually properly unit-testing something like shared_ptr is complex, and way beyond the scope here.
But a really easy trick is to simply insert a bunch of std::cout statements in your class member functions to print all the data about what’s going on. Yes, this isn’t the “sexy” way to test your code, but you’re a beginner; focus more on your code actually being correct than on doing things the “sexy” way.
In your destructor, you should also print the contents of the count map. That will give you a way to spot leaks, or other problems with the counts.
As I said, this isn’t technically the “right” way to do testing, but screw it, it’s quick, it’s easy, and it works. Even I develop code this way sometimes, if I’m experimenting with something difficult or “clever”. It’s cheap and hacky, but in the early stages of figuring something out, it does the job.
Code review
For starters, you should really be putting your own classes and stuff into your own, personal namespace. For example, I always use indi:
namespace indi {

template <typename T>
class shared_ptr
{
    // ... [snip] ...
};

} // namespace indi

auto main() -> int
{
    indi::shared_ptr<int> sp1;
    // ...
}

Putting your stuff in your own namespace is a good habit to get into in any case.
template<typename dataType>

dataType is a bad name for a template parameter. By convention, template parameters are written in “UpperCamelCase”. Also by convention, T is most commonly used. And it’s a hell of a lot shorter than dataType.
    shared_ptr()
    {
        id++;
        internalId = id;
        countMap[id] = 1;
        ptr = new dataType();
    }

Your default constructor actually allocates a new object. That’s… not a great idea. Default construction should be simple, fast, and—ideally—no fail.
Not only that, this behaviour is pretty surprising for a pointer. Consider:
using pointer = int*;

auto p1 = pointer{};                // p1 == nullptr, no-fail and fast
auto p2 = std::unique_ptr<int>{};   // p2 == nullptr, no-fail and fast
auto p3 = std::shared_ptr<int>{};   // p3 == nullptr, no-fail and fast
auto p4 = shared_ptr<int>{};        // p4 != nullptr, very slow, and the allocation might throw

Default construction for every other smart pointer I’ve seen gives you a null pointer. That should be the case for your smart pointer, too.
If you want a constructor that actually allocates the object—which is a good idea—you can use a “tagged constructor”. A good tag is std::in_place:
    constexpr shared_ptr() noexcept = default;

    explicit shared_ptr(std::in_place_t)
    {
        id++;
        internalId = id;
        countMap[id] = 1;
        ptr = new dataType();
    }

And you’d use it like this:
auto sp1 = shared_ptr<int>{std::in_place};

You could get even more advanced with perfect forwarding:
    template <typename... Args>
    explicit shared_ptr(std::in_place_t, Args&&... args)
    {
        id++;
        internalId = id;
        countMap[id] = 1;
        ptr = new dataType{std::forward<Args>(args)...};
    }

So now you could do:
auto sp = shared_ptr<std::string>{std::in_place, '*', 8};
// sp points to a string containing 8 asterisks

But let’s dig into the meat of the constructor:
    shared_ptr()
    {
        id++;
        internalId = id;
        countMap[id] = 1;
        ptr = new dataType();
    }

The first line can’t fail. So no problems yet (or so it seems!).
The second line also can’t fail. This is the only safe line in this entire constructor. (And even then, it’s not perfectly safe; it would be dangerous in concurrent code. But concurrency is a whole other beast.)
This line can fail. When you do countMap[id], the map will try to find a key matching id, and will (presumably) fail. So it will need to allocate a new node. That allocation might fail. This won’t be too serious a problem… all it means is that you’ll skip an ID.
This line can also fail, either in the allocation phase, or the construction of the new object. Either way, this is bad, because now countMap holds a node with an ID and count of 1 that will never be freed. Since no other shared pointer will use the same ID, this is only a minor leak… but it’s still a leak.

If you don’t care about losing IDs, you could fix this constructor by doing something like so:
    shared_ptr()
    {
        id++;
        internalId = id;
        countMap[id] = 1;

        try
        {
            ptr = new dataType();
        }
        catch (...)
        {
            countMap.erase(id);
            throw;
        }
    }

But the moral here is to always consider which expressions can throw, and what that means for the function as a whole.
    shared_ptr(shared_ptr& a)
    {
        this->ptr = a.ptr;
        this->internalId = a.internalId;
        countMap[internalId]++;
    }

The correct form for a copy constructor is to take the argument by const reference.
Failure to do so means this code won’t work:
auto const sp1 = shared_ptr<int>{};
auto const sp2 = sp1; // won't compile

Other than that, this copy constructor is okay. Most C++ programmers would gripe about the this-> stuff, because it’s unnecessary and ugly, but it’s not actually wrong, or harmful. (You could also use a member initializer list, but meh. For these simple types it doesn’t really matter.)
    void operator=(shared_ptr& a)
    {
        this->ptr = a.ptr;
        this->internalId = a.internalId;
        countMap[internalId]++;
    }

@1201ProgramAlarm has already pointed out one of the bugs in this function, but the best way to fix it is to rethink the whole function entirely, and use the “copy-and-swap” technique.
You see, if you follow @1201ProgramAlarm’s advice directly… you’ll have another problem. Let me show you—suppose you did this:
auto operator=(shared_ptr const& a) -> shared_ptr&
{
    // first decrement the count of the current pointer, and delete if necessary
    --countMap[internalId];
    if (countMap[internalId] == 0)
        delete ptr;

    // now set the new data
    ptr = a.ptr;
    internalId = a.internalId;
    ++countMap[internalId];

    return *this;
}

This looks innocuous… but it’s a trap. It won’t explode easily, but it will explode if you do this:
auto sp = shared_ptr<int>{};
sp = sp; // self-assignment

Consider what happens during the self-assignment above. Before the self-assignment, the count for the pointer is 1 (because it’s the only smart pointer that owns it). So the first line of the assignment operator does the decrement… and now the count is zero. So, of course, the object gets freed.
Then the “new” data gets transferred over. But… the new data is the same as the old data. So ptr is set to… itself… the pointer which now points to the deleted object. internalId is also set to itself, but that’s harmless. Then the count gets re-incremented, so it goes from zero to one again.
The net result is that your shared pointer appears to still be working—still keeping the count to some pointer—but the pointer points to deleted memory. If you access it, boom. And of course, when the last shared pointer pointing to it is destroyed, it will try to free it again. Double delete. Boom.
So I recommend the “copy-and-swap” technique:
auto operator=(shared_ptr const& a) -> shared_ptr&
{
    // copy a into a temporary...
    auto temp = a;

    // ... then swap the contents of the temporary with this
    std::ranges::swap(ptr, temp->ptr);                  // cannot fail
    std::ranges::swap(internalId, temp->internalId);    // cannot fail

    return *this;

    // when temp goes out of scope, it will be destroyed
    //
    // since it now holds the *OLD* internalID that this had, it will
    // decrement *THAT* count, and if it's zero, free the *OLD* ptr this held
    //
    // in the case of self-assignment, all the copy will do is increase the
    // count (to 2), and then the destruction will decrease it (back to 1),
    // so no harm, no foul
}

“Copy-and-swap” is technically less efficient… but it avoids very subtle bugs, like the self-assignment bug I described. You should do “copy-and-swap” in copy assignment operators by default.
Also, your copy-assignment operator is malformed. It should take a const reference, and it should return a non-const reference to this… not void.
The destructor is fine, so I’ll skip it.
    static int id;
    int internalId = 0;
    static std::map<int,int> countMap;

Okay, there are a couple of issues here.
First, I don’t know if you realize it, but since shared_ptr is a template, that means that there will be multiple id and countMap objects… one for each template parameter set.
In other words:
auto sp1 = shared_ptr<int>{};
auto sp2 = shared_ptr<long>{};

You probably think that the internalId for sp1 will be 2, and for sp2 it will be 3. Nope. It will be 2 for both. Why? Because sp1 is using shared_ptr<int>::id, and sp2 is using shared_ptr<long>::id. Those are two different ints.
In fact:
auto sp01 = shared_ptr<char>{};
auto sp02 = shared_ptr<signed char>{};
auto sp03 = shared_ptr<unsigned char>{};
auto sp04 = shared_ptr<int>{};
auto sp05 = shared_ptr<unsigned int>{};
auto sp06 = shared_ptr<short>{};
auto sp07 = shared_ptr<unsigned short>{};
auto sp08 = shared_ptr<long>{};
auto sp09 = shared_ptr<unsigned long>{};
auto sp10 = shared_ptr<std::string>{};
auto sp11 = shared_ptr<std::u8string>{};
auto sp12 = shared_ptr<std::vector<int>>{};
auto sp13 = shared_ptr<std::vector<long>>{};

The above code will create thirteen different id ints, and thirteen different countMap std::map<int, int>s. Every unique type you instantiate a shared_ptr with will create a new id and countMap. If you make shared_ptrs using many different types, that will balloon your static memory usage. And the bad thing about static memory usage is that it exists for the whole program. It is created when the program starts even if you never actually use it, and it exists until the program ends, even if you’re long done with it. Indeed, if you only allocate a single shared_ptr<foo> one time in the middle of your program, you’re paying for it for the whole program. You’re even paying for it if you allocate zero shared_ptr<foo> objects… but only mention one as a conditional possibility!
So this is not good. You probably only want a single map and id counter for all types in your program. To do that, you need to pull those static objects out of the template. You have multiple options for how you can do that. You can simply use global objects (probably hidden in a detail namespace). Or you can use a non-template base class and put all the statics in there.
Another issue is the use of int for both the ID and the count. int is only guaranteed to hold 65,536 values, though in practice it will usually hold at least 4,294,967,296. Four billion sounds like a lot, and it probably is, but it is not inconceivable that if a program runs long enough, and does enough allocating and deallocating, it could run out. (Far more likely to have that happen if you only have 65,536 values, of course.)
Whenever you’re keeping a count of something, the standard type to use in C++ is std::size_t. On most 64-bit systems, this will give you at 18,446,744,073,709,551,616 different IDs. That should be enough. You should use this for both the ID and the count.
So, perhaps you might do this:
// these could be hidden in a detail namespace, or a base class
//
// watch out for the static initialization order fiasco!
static auto next_id = std::size_t{0}; // in C++23 "= 0uz;" would work
static auto count_map = std::map<std::size_t, std::size_t>{};

// since the static variables are not in the template, they won't be
// duplicated for every T
template <typename T>
class shared_ptr
{
    // ...
};

Finally, you should consider using std::unordered_map rather than std::map. You don’t need the ordering, and the unordered map may be faster.
All that’s left is main():
shared_ptr<int> sp1;

You should really avoid this kind of declaration. The gold standard in modern C++ is the “always auto” style:
auto sp1 = shared_ptr<int>{};

There are a number of reasons why this form is better, too many to get into here. But as a bonus, once you get in the habit of using auto, numerous other bugs and quirks go away, and many lines of code become simpler. For example, instead of:
shared_ptr<int> sp2(sp1);

You could just do:
auto sp2 = sp1;

Shorter, simpler, clearer, and it avoids all kinds of bugs.


React multi-select-like component with filter I am new to react (and javascript in general) but am enjoying learning it.
I have decided to make a little multiselect component which should work as follows:

I start with my data in one list, when I select an item from the list it goes into a second list (I can 'unselect' as well).

I also can filter the first list with a text input.

Before you review, I realize that there are likely existing solutions for this functionality that I could use, but I am just doing this to learn.

codepen here https://codepen.io/str8wavedave/pen/ZqWXQp

class MultiSelect extends React.Component {
  render(){
    return(

      {this.props.objects.map((object, index) =>
 this.props.onClickFunction(index)}>{object.name}
)}

    );
  }
}

class DoubleMultiselect extends React.Component {
  state = {
    value: '',
    filteredUnselectedObjects: this.props.objects,
    unselectedObjects: this.props.objects,
    selectedObjects: [],
  }
  filterList = (event) => {
    var updatedList = this.state.unselectedObjects;
    updatedList = updatedList.filter(function(item){
      return item.name.toLowerCase().search(
      event.target.value.toLowerCase()) !== -1;
    });
    this.setState({
      filteredUnselectedObjects: updatedList,
      value: event.target.value
    })
  }
  handleSelect = (index) => {
    var obj = this.state.filteredUnselectedObjects[index]
    var newSelected = this.state.selectedObjects.slice()
    newSelected.push(obj)

    var newUnselected = this.state.unselectedObjects.slice()
    var originalIndex = newUnselected.indexOf(obj);
    newUnselected.splice(originalIndex, 1)

    var newFilteredUnselected = this.state.filteredUnselectedObjects.slice()
    newFilteredUnselected.splice(index, 1)

    this.setState({
      selectedObjects: newSelected,
      unselectedObjects: newUnselected,
      filteredUnselectedObjects: newFilteredUnselected
    });
  }

  handleUnselect = (index) => {
    var obj = this.state.selectedObjects[index]
    var newUnselected = this.state.unselectedObjects.slice()
    newUnselected.push(obj)

    var newFilteredUnselected = this.state.filteredUnselectedObjects.slice()
    if(obj.name.toLowerCase().search(this.state.value.toLowerCase()) !== -1)
    {
      newFilteredUnselected.push(obj)
    }
    var newSelected = this.state.selectedObjects.slice()
    newSelected.splice(index, 1)
    this.setState({
      unselectedObjects: newUnselected,
      selectedObjects: newSelected,
      filteredUnselectedObjects: newFilteredUnselected
    });
  }

  render() {
    return(

        <input type="text" onChange={this.filterList}/>
        <MultiSelect objects={this.state.filteredUnselectedObjects} onClickFunction={this.handleSelect}/>
        <MultiSelect objects={this.state.selectedObjects} onClickFunction={this.handleUnselect}/>

    );
  }
}
const PLAYERS =[
  {name: 'Charlie', id: 1},
  {name: 'David', id: 2},
  {name: 'Eric', id: 3},
  {name: 'Emily', id: 4},
  {name: 'Peter', id: 5},
  {name: 'Sam', id: 6},
  {name: 'Doug', id: 7}
]
React.render(<DoubleMultiselect objects={PLAYERS}/>, document.getElementById('app'));

Overall this is good, but there are few things we can do improve this and make it more "modular".

Lets say someone wants to use your component and instead of an array of player objects, they have just an array of player names, this introduces a couple of problems.

The prop to pass this in the list is called objects which doesn't reflect the type of data that they have and their list is long so they can't manually change it.
Your component is expecting the data passed in objects to be an array of objects.

How can we resolve this?

Lets first rename the prop from objects -> data, this is better because it generalizes what this prop receives and the person using this can possibly assume you are able to handle multiple data types.

Next, we should update places where we are assuming the data to be an object and instead check to see if it is an object before handling it. One example is in <MultiSelect />, where you are iterating over each item. We can change that to be:

class MultiSelect extends React.Component {
  render() {
    return (

        {this.props.data.map((item, index) => (

 this.props.onClickFunction(index)}>
            {typeof item === "object" ? item.name : item}

        ))}

    );
  }
}

Now we can display data from an array of strings!

But what if the person using your component was using an array of objects? Unfortunately there is no documentation so when he created the objects he used the property name firstName instead of name. This now breaks the code we re-wrote above, but we can fix it...

Lets set a new prop on <DoubleMultiselect /> called something like nameField, which would be a string that is equal to the object property to display in the list.

So now wherever we were accessing the name property of the player we can replace it with this.props.nameField, re-writing the code above can now be:

class MultiSelect extends React.Component {
  render() {
    return (

        {this.props.data.map((item, index) => (

 this.props.onClickFunction(index)}>
            {typeof item === "object" ? item[this.props.nameField]: item}

        ))}

    );
  }
}

We've now added support for dynamic data, allowing anyone with any type of data to use this!


Refactoring code to implement clean code using repositories I've been refactoring code today, try to implement a clean code setup using repositories. Is this on track? Can any improvements be made?

public class IndexModel : PageModel
{
    private readonly MembershipFormRepository _membershipFormRepository;
    private readonly IMembershipFormViewModelService _membershipFormViewModelService;

    public IndexModel(IMembershipFormViewModelService membershipFormViewModelService, MembershipFormRepository membershipFormRepository)
    {
        _membershipFormViewModelService = membershipFormViewModelService;
        _membershipFormRepository = membershipFormRepository;
    }

    [BindProperty]
    public MembershipFormViewModel MembershipFormModel { get; set; } = new MembershipFormViewModel();

    public async Task OnGetAsync()
    {
        MembershipFormModel = await _membershipFormViewModelService.GetViewModel();
    }

    public async Task<IActionResult> OnPostAddMemberAsync()
    {
        if (!ModelState.IsValid)
        {
            return Page();
        }

        await _membershipFormRepository.AddMember(MembershipFormModel);

        return RedirectToPage("Index");
    }
}

MembershipFormViewModelService

public class MembershipFormViewModelService : IMembershipFormViewModelService
{
    private readonly MembershipContext _context;
    private readonly MembershipFormRepository _membershipFormRepository;

    public MembershipFormViewModelService(MembershipContext context, MembershipFormRepository membershipFormRepository)
    {
        _context = context;
        _membershipFormRepository = membershipFormRepository;
    }

    public async Task<MembershipFormViewModel> GetViewModel()
    {
        var viewModel = new MembershipFormViewModel()
            {
                Titles = await SetTitlesDropDown(),
            };

        return viewModel;
    }

    private async Task<IEnumerable<SelectListItem>> SetTitlesDropDown()
    {
        var titlesList = new List<SelectListItem>();

        var titles = await _membershipFormRepository.GetTitles();

        titlesList.AddRange(titles.Select(title => new SelectListItem() {
            Value = title.TitleId.ToString(),
            Text = title.Title
        }));

        return titlesList;
    }
}

IMembershipFormViewModelService

public interface IMembershipFormViewModelService
{
    Task<MembershipFormViewModel> GetViewModel();
}

MembershipFormRepository

public class MembershipFormRepository : IMembershipFormRepository
{
    private readonly MembershipContext _context;
    private readonly IMapper _mapper;

    public MembershipFormRepository(MembershipContext context, IMapper mapper)
    {
        _context = context;
        _mapper = mapper;
    }

    public async Task<IEnumerable<Title>> GetTitles()
    {
        return await _context.Title.ToListAsync();
    }

    public async Task AddMember(MembershipFormViewModel membershipForm)
    {
        var membership = new Membership();

        membership = _mapper.Map(membershipForm, membership);

        _context.Membership.Add(membership);

        await _context.SaveChangesAsync();
    }
}

IMembershipFormRepository

public interface IMembershipFormRepository
{
    Task<IEnumerable<Title>> GetTitles();
    Task AddMember(MembershipFormViewModel membershipForm);
}

You are close. The only thing that jumps out at me is your use of the MembershipFormRepository type instead of the IMembershipFormRepository interface. This prevents you from being able to test consumers of it easily. For example, consider this case:

You have an object of type Foo that takes an injected instance of type Bar that takes an injected instance of a DB context context. You want to test that something on the Foo type works. Here is your test setup:

var dbContext = new Moq<DbContext>();
dbContext
    .Setup(s => s.Get())
    .Returns(new List<Item>());

var bar = new Bar(dbContext.Object);
var foo = new Foo(bar);

// assert foo.DoSomething()

Notice how you have to mock up a full Bar object, and even more troublesome, rely on Bar working correctly, for the test to pass. If Bar has a bug, your test can fail; this test, because it tests Foo, should only fail if Foo has a bug. If you injected a type of IBar into the Foo instead, you could mock that, instead of the DB context:

var bar = new Moq<IBar>();
bar.Setup(s => s.GetItems())
   .Returns(new List<Item>());

var foo = new Foo(bar.Object);

// assert foo.DoSomething()

Now, instead of relying on Bar.GetItems not being buggy, you just create a mocked object that always returns the same data. This test also won't fail if Bar changes. The first test would fail if you changed Bar.GetItems to call DbContext.Where instead of DbContext.Get because you hadn't mocked the Where call, which would once again falsely report a bug in the Foo type because the Bar type had changed or was buggy.


Structuring unit tests I have settled with this type of structuring of unit tests.

Basic idea is that there is a base test class for the tested class, and a subclass for every tested method. They use the mocks and everything from the base test class, the point being that the test would remain small, and you don't have to create everything in the test.

Idea for this came from Haacked http://haacked.com/archive/2012/01/02/structuring-unit-tests.aspx/

public class HomeControllerTest
{
    protected HomeController Controller;
    protected Mock<IUnitOfWork> UnitOfWork;
    protected Mock<IRepositoryOne<Entity>> Repository;
    protected Fixture Fixture;

    public HomeControllerTest()
    {
        UnitOfWork = new Mock<IUnitOfWork>();
        Repository = new Mock<IRepositoryOne<LoanRequirement>>();

        Controller = new HomeController(
            UnitOfWork.Object,
            Repository.Object);

        Fixture = new Fixture();
    }

    public class GetHomeMethod : HomeControllerTest
    {
        [Fact]
        public void ReturnsEntityDataFromDatabase()
        {
            Repository.Setup(x => x.GetId(1)).Returns(new Entity());

            var result = Controller.GetOne(1) as OkNegotiatedContentResult<HomeModel>;
            Assert.NotNull(result);
            var model = result.Content;

            var entityToModel = Mapper.Map<HomeModel>(entity);
            Assert.Equal(model, entityToModel,HomeModel.HomeModelComparer);

        }
    }

    public class PostHomeMethod : HomeControllerTest
    {
        // Test the post method
    }

    public class PutHomeMethod : HomeControllerTest
    {
        // Test the put method
    }
}

I ask in general is there anything wrong about doing it like this, any dangers that I don't spot? Any comments are welcome. Main point for asking a question like this is to polish my unit tests, so they would be even more organized.

There are a few major differences between what I understand from your code and what I understand from the post you mention.

Inheritance vs Nesting
Your code suggests that there is a subclass for each tested method, although the post talks about nested classes:

  The structure has a test class per class being tested. That’s not so
  unusual. But what was unusual to me was that he had a nested class for
  each method being tested.

Class per test vs Class per Method
Also, your code example implies that each class contains a single test, which loses all meaning for the class. The post talks about a class per method tested, which mean that it may contain more than one test, but all tests are relevant to the same method.

using Xunit;

public class TitleizerFacts
{
    public class TheTitleizerMethod
    {
        [Fact]
        public void ReturnsDefaultTitleForNullName()
        {
            // Test code
        }

        [Fact]
        public void AppendsTitleToName()
        {
            // Test code
        }
    }

This kind of structure is very similar to how an RSpec test would be structured, having multiple layers of describe blocks for different scopes - class, method, feature...

describe HomeController do

  let (:fixture) { double('fixture') }

  before(:each) do
    # something before each test
  end

  describe '#get_home' do

    before(:each) do
      expect(repository).to receive(:get_id).with(1).and_return(entity)
    end

    let(:result) { subject.GetOne(1) }

    it 'returns result' do
       expect(result).to_not be_nil
    end

    it 'returns entity data from database' do
       expect(result.content).to eq entity
     end
  end
end


Javascript - Extract data from html table I want to extract the headers data and column data (not row data) from an HTML table using JavaScript.

Is this a good approach of doing it? And how can I simplify this using jQuery?

let table = document.getElementById('tab')
debugger
let headers = Array.from(table.rows[0].cells).map(x => x.innerText)
let columnData =
Array.from(table.rows).
      slice(1, table.rows.length).
      map(row =>Array.from(row.cells).map(x => x.innerText))
     .reduce((acc,rowData)=>{
           rowData.forEach((value,index)=>{
           acc[index]= acc[index] || [ ]
           acc[index].push(value) })
      return acc },[])
console.log(headers)
console.log(columnData)

      Name

      Age

      Location

      Jason

      22

      Texas

      Lawson

      21

      Florida

      Jose

      25

      London

Always use const to declare variables - only use let when you must reassign. This keeps code readable, because then a reader of the code doesn't have to constantly keep in mind that a variable might be reassigned later. (If you use let but then don't reassign, it can still be confusing - in professional code, one might think "Why is let being used here? Was this meant to be reassigned in a section of code that was later removed, or something?)

Array.from accepts an optional mapper function as a second parameter. Any time you have:

Array.from(arrayLike).map(mapper)

you may replace it with

Array.from(arrayLike, mapper)

(If all you're doing is converting an array-like object into an array, some prefer spread syntax because it's even more concise: [...arrayLike])

innerText is a weird property introduced by Internet Explorer (outside of web standards originally) that has a number of odd quirks. Unless you're deliberately looking to invoke those quirks, it would be a better idea to use textContent instead to retrieve text from an element.

You can easily distinguish the first tr from the other trs by using the query string #tab tr:first-child or #tab tr:nth-child(n + 2):

const headers = Array.from(
  document.querySelectorAll('#tab tr:first-child th'),
  th => th.textContent.trim()
);
// Make an empty array for every item in headers:
const data = Array.from(headers, () => []);
for (const tr of document.querySelectorAll('#tab tr:nth-child(n + 2)')) {
  [...tr.children].forEach((th, i) => {
    data[i].push(th.textContent.trim());
  });
}
console.log(headers);
console.log(data);

      Name

      Age

      Location

      Jason

      22

      Texas

      Lawson

      21

      Florida

      Jose

      25

      London

That's already quite simple, IMO. I think adding jQuery to the mix would make things unnecessarily more complicated, not less.

I refactored it out, but I don't think it's good idea to use reduce when the accumulator is going to be the same object every time. See: Is reduce bad? by Google devs. If it's always going to be the same object, it'll be a bit easier to read if that object is declared as a standalone variable in the outer scope.

The HTML is a bit weird. A  is a table header. It makes sense for the headers to be s, but the table data should probably be s instead.


Vertical Text Printing (Java) I was trying one of the problems found on HP Codewars 2007: (2)

  Task Description:
  The CodeWars judges and scorekeepers need help printing the team names above each column on our score sheet. But we want them to look nice, so they need to be printed vertically. All we have to use is a list of team names. Please write
  us a program that will take a list of team names, and print them vertically.

  Program Input Your program will prompt for a team name. Continue prompting for names until you receive “END”. Unlike the real CodeWars,
  this one will be limited to 16 teams maximum.

  Program Output Your program will output to the screen each team’s name, in the order it was entered, arranged vertically such that it is
  read from top to bottom. There should be two spaces between each name
  as printed.

I attempted the problem and created the Java solution below:

import java.util.Scanner;
import java.util.ArrayList;

public class Prob02 {
    public static void main(String[] args) {
        Scanner stdin = new Scanner(System.in);

        ArrayList<String> teams = new ArrayList<>();
        ArrayList<Integer> log = new ArrayList<>();

        do{
            System.out.print("Enter team name: ");
            teams.add(stdin.nextLine());
            //System.out.println(); Optional for clearer display. But not required.
        } while (!"END".equals(teams.get((teams.size()-1))));
                    //while END is not equal to last element of teams

        teams.remove(teams.get((teams.size()-1)));

        //Find largest String length
        //set defaults - index is 0 and elementLength is first length
        int index = 0;
        int elementLength = teams.get(0).length();

        //If the first one was indeed the longest one and the only one...
        log.add(index);

        for(int i = 0; i < teams.size(); i++) {
            if ((teams.get(i)).length() > elementLength) {
                //overwrite index and elementLength if the new one is larger than the default
                index = i;
                elementLength = (teams.get(i)).length();

                log.add(index); //now, we only need index (or team).
            }

            //repeat.
        }

        String longest = teams.get( log.get(log.size()-1) );

        int longestLengthOfTeam = longest.length();

        int letterYouArePrinting = 0;

        for(int i = 0; i < (longestLengthOfTeam); i++){

            for(int x = 0; x < teams.size(); x++) {
                String team = teams.get(x);
                String[] teamSplit = team.split("");
                //CONVERT !! !!! !!!!!
                ArrayList<String> teamTheNewOne = new ArrayList<>();
                for(int ttno=0; ttno < teamSplit.length; ttno++) {
                    teamTheNewOne.add(teamSplit[ttno]);
                }

                boolean inBoundChecker = (letterYouArePrinting >= teamTheNewOne.size());
                if (inBoundChecker == true) {
                    System.out.print("   "); //not a whole line! 3 Spaces, [1] + 2 req.
                    continue;
                }

                    //Print first letter of all teams
                    System.out.print(teamTheNewOne.get(letterYouArePrinting) + "  " /*Two spaces rule*/);

            }

            System.out.println();
            letterYouArePrinting++;
        //Now, print next letter
        }

    }
}

Is there a more efficient way to solve this problem?

P.S.: Problem can be found at http://www.hpcodewars.org/past/cw10/problems/Prob02%20--%20Vertical%20Printing.pdf .

Yes, I think there is more straightforward ways to implement this:

Compute the length of the largest string.
Create a 2D char[][] matrix with the amount of rows equal to the length of the largest string. The amount of "columns" depends how much space you want between two neighbour columns.
Fill out the matrix with characters.
Create a StringBuilder and fill it with the chars from the matrix.

All in all

I had this in mind:

private static final int DEFAULT_COLUMN_SPACING = 2;

public static String makeVerticalText(List<String> lines) {
    return makeVerticalText(lines, DEFAULT_COLUMN_SPACING);
}

public static String makeVerticalText(List<String> lines,
                                      int columnSpacing) {
    if (lines.isEmpty()) {
        return "";
    }

    int maximumLength = lines.stream()
                             .max(Comparator.comparing(String::length))
                             .get()
                             .length();

    char[][] matrix = new char[maximumLength]
                              [(columnSpacing + 1) * lines.size()
                              - columnSpacing];
    int columnIndex = 0;

    for (String line : lines) {
        int rowIndex = 0;

        for (char c : line.toCharArray()) {
            matrix[rowIndex++][columnIndex] = c;
        }

        columnIndex += 1 + columnSpacing;
    }

    int numberOfColumns = matrix[0].length;
    StringBuilder sb =
            new StringBuilder(maximumLength * (matrix[0].length + 1));

    for (int rowIndex = 0; rowIndex < maximumLength; ++rowIndex) {
        for (columnIndex = 0; columnIndex < numberOfColumns; ++columnIndex) {
            char currentChar = matrix[rowIndex][columnIndex];
            sb.append(currentChar == '\u0000' ? ' ' : currentChar);
        }

        if (rowIndex < maximumLength - 1) {
            sb.append('\n');
        }
    }

    return sb.toString();
}

Hope that helps.


Random alphanumeric string generator function (C, Windows, bcrypt.h) The function uses the Windows "Cryptography API: Next Generation" (bcrypt.h)'s BCryptGenRandom function to generate random bytes - one for each character.
It returns the number of characters (excluding NUL-terminator) written to the buffer, -1 in the case of a bad argument, or -2 in the case of an error.
Example usage of the function:
wchar_t str[9]; // 8 chars + NUL
rand_alphanum_str(str, sizeof (str));
printf("8 random alphanumeric characters: %wS\n", str);

Function code:
int rand_alphanum_str(WCHAR *buffer, size_t buffer_size) {
    if(!buffer || buffer_size < 2) { return -1; }

    int chars = buffer_size / sizeof (WCHAR) - 1; // -1 for null terminator

    // need one random byte per character to be generated
    unsigned char *rand_bytes = malloc(chars);
    if(!rand_bytes) { return -2; }

    // filling rand_bytes buffer
    if(!NT_SUCCESS(BCryptGenRandom(NULL, rand_bytes, chars,
                                   BCRYPT_USE_SYSTEM_PREFERRED_RNG))) {
        free(rand_bytes);
        return -2;
    }

    for(int i = 0; i < chars; ++i) {
        // rand_bytes[i] is in range [0, 255].
        // need a number in range [0, 61] - as there are 62 alphanumeric
        // characters

        // bit-twiddling to attempt to maintain a uniform distribution:

        // discard 2 least-significant bits - rand_bytes is now in [0, 63]
        rand_bytes[i] >>= 2;
        if((rand_bytes[i] >> 5) & 1U) {   // if the now most-significant bit is set,
            rand_bytes[i] &= ~(1UL << 1); // clear the 2nd least-significant bit
            // (only cleared if most-significant bit is set so as to avoid
            // throwing distribution off by having a bit which is never set)
        }
        // rand_bytes[i] is now in [0, 61]

        // of the 62 possible values of rand_bytes[i]:
        //   - [0, 9] represent numeric digits
        //   - [10, 35] represent uppercase letters
        //   - [36, 61] represent lowercase letters
        // the offset of rand_bytes[i] from the low end of the range it lies in
        // is added to said range's first ASCII value to give a random character
        // in that range
        if(rand_bytes[i] <= 9) {
            buffer[i] = L'0' + rand_bytes[i];
        } else if(rand_bytes[i] <= 35) {
            // -10 for offset from beginning of numeral range
            buffer[i] = L'A' + rand_bytes[i] - 10;
        } else {
            // -36 for offset from beginning of uppercase range
            buffer[i] = L'a' + rand_bytes[i] - 36;
        }
    }
    buffer[chars] = L'\0';

    free(rand_bytes);
    return chars;
}

Thank you!

The output is not uniformly distributed
The resulting characters in the output string are not uniformly distributed. In fact, some characters never occur in the output. After shifting a random byte right by two, if the result is larger than or equal to 32, you always clear the second least significant bit. So all values up to 32 are possible, but then it is 32, 33, 36, 37, 40, 41 and so on. And each value >= 32 that your algorithm generates is twice as likely as each value < 32.
The best solution is to first shift the random byte right by two, and then check i it is smaller than 62. If so, use it as it is. Otherwise, generate a new random byte and repeat the process.
Avoid allocating an array of random bytes
You are allocating an array to store the random bytes in. While it might seem efficient to do this, because you assumed you then only need one call to BCryptGenRandom(), this might be an issue if you want to generate large random strings. Also, if you want to have a proper uniformly distributed output, you might need to generate more random bytes than you originally anticipated.
Consider having a small array on the stack that you fill with random bytes, and use those to generate the alphanumeric characters. If you used up all the bytes, you fill the array with a new batch of random bytes.


Javascript velocity converter, fully chainable I created this out of curiosity and it has some code duplication issues and the fact that sticking .yield() on the end of a call to a unit converter is pretty strange.

How would you improve it?

/**
 * A very special velocity converter
 * Designed mainly for a very natural syntax
 * It allows full chainable conversion calls and maintains its on internal state.
 *
 * example:
 * remotewind.convert(10).knots().to.mph().to.kmh().yield();
 *
 * Each unit has a yield "method" which returns the last value (the internal state)
 *
 * @param val
 * @return Converter
 */
remotewind.convert = function(val) {

    var val = val;

    function Unit(){
        this.yield = function(){
            return val;
        }
    }

    function Ms(ms){
        val = ms;
        this.to = {
            kmh : function(){ return new Kmh(val * 3.6) },
            mph : function(){ return new Mph(val * 2.2369362920544) },
            knots: function(){ return new Knots(val * 1.943844492) }
        }
    }
    Ms.prototype = new Unit();

    function Kmh(kmh){
        val = kmh;
        this.to = {
            ms : function(){ return new Ms(val * 0.277777778) },
            mph : function(){ return new Mph(val * 0.621371192) },
            knots: function(){ return new Knots(val * 0.539956803) }
        }
    }
    Kmh.prototype = new Unit();

    function Mph(mph){
        val = mph;
        this.to = {
            ms : function(){ return new Ms(val * 0.44704) },
            kmh : function(){ return new Kmh(val * 0.621371192) },
            knots: function(){ return new Knots(val * 0.868976242) }
        }
    }
    Mph.prototype = new Unit();

    function Knots(kn){
        val = kn;
        this.to = {
            ms : function(){ return new Ms(val * 0.514444444) },
            mph : function(){ return new Mph(val * 1.150779448) },
            knots: function(){ return new Knots(val * 1.852) }
        }
    }
    Knots.prototype = new Unit();

    function Converter(val){
        return {
            ms: function(){  return new Ms(val) },
            kmh: function(){  return new Kmh(val) },
            mph: function(){  return new Mph(val) },
            knots: function(){  return new Knots(val) }
        }
    }

    return new Converter(val);
}

Here's a fairly cryptic rewrite, focussing mostly on extensibility and minimal repetition. The trick I'm using is to pick one unit as our "base" unit from which all others are derived. The base-unit value (m/s in this case) is carried through the chain.

var convert = (function () {
  var conversions = {
    ms:    1, // use m/s as our base unit
    kmh:   3.6,
    mph:   2.23693629,
    knots: 1.94384449
    // feel free to add more
  };

  function Unit(unit, ms) {
    this.value = ms * conversions[unit];
    this.to = {};
    for(var otherUnit in conversions) {
      (function (target) {
        this.to[target] = function () {
          return new Unit(target, ms);
        }
      }).call(this, otherUnit);
    }
  }

  Unit.prototype = {
    yield: function () {
      return this.valueOf();
    },

    toString: function () {
      return String(this.value);
    },

    valueOf: function () {
      return this.value;
    }
  };

  return function (value) {
    var units = {};
    for(var unit in conversions) {
      (function (unit) {
        units[unit] = function () {
          return new Unit(unit, value / conversions[unit]);
        };
      }(unit));
    }
    return units;
  }
}());

Edit: Just for fun, here's an ultra-convoluted version that handles different measurement types. Couldn't get temperature conversions in there, as simple multiplication/division isn't enough to deal with C/K/F conversions. Boo.

Really though, the code's pretty dense, and I can't recommend coding like this. But again, this was just for fun.

Anyway, the usage would be something like

convert.speed(60).mph().to.kmh();  // ~97km/h
convert.distance(100).m().to.ft(); // ~330ft
convert.mass(1000).g().to.lb();    // ~2.2lb

And here's the hairy code:

var convert = (function () {
  var conversions = {
    speed: {
      ms:    1, // use m/s as our base unit
      kmh:   3.6,
      mph:   2.23693629,
      knots: 1.94384449
    },

    distance: {
      m:      1, // use meters as our base
      inches: 39.3700787402, // can't use "in" as that's a keyword. Darn.
      ft:     3.280839895,
      mi:     0.000621371192,
      nm:     0.000539956803 // nautical miles, not nanometers
    },

    mass: {
      g:  1, // use grams as our base
      lb: 0.002204622622,
      oz: 0.0352739619
    }
  };

  function Unit(type, unit, base) {
    this.value = base * conversions[type][unit];
    this.to = {};
    for(var otherUnit in conversions[type]) {
      (function (target) {
        this.to[target] = function () {
          return new Unit(type, target, base);
        }
      }).call(this, otherUnit);
    }
  }

  Unit.prototype = {
    yield: function () {
      return this.valueOf();
    },

    toString: function () {
      return String(this.value);
    },

    valueOf: function () {
      return this.value;
    }
  };

  // my god, it's full of scopes!
  var types = {};
  for(var type in conversions) {
    (function (type) {
      types[type] = function (value) {
        var units = {};
        for(var unit in conversions[type]) {
          (function (unit) {
            units[unit] = function () {
              return new Unit(type, unit, value / conversions[type][unit]);
            }
          }(unit));
        }
        return units;
      };
    }(type));
  }

  return types;
}());


Print the number of vowels in C I created this program to print the number of vowels of a given text in C; it is only a small exercise. I consider a vowel only the letters "aeiou", both in upper case and lower case.

I provided a small command line interface: if you give -h or --help you get the help message, if you give the text (e.g. vowel "This is a text") it directly prints the number of vowels. If you run the program without any argument or text, it reads from the stdin until the user types CTRL-D two times.

Examples:

user@computer:$ ./vowels
This is
a
text
4
user@computer:$ ./vowels "This is a text"
4

The source code is organized in a main.c and a Makefile.

Makefile

.PHONY = clean all

PROGNAME = vowels

CC ?= gcc

CFLAGS = -Wall -Wextra

all : $(PROGNAME)

$(PROGNAME) : main.o
    $(CC) -o $(PROGNAME) $^

clean :
    $(RM) *.o $(PROGNAME)

main.c

#include <stdio.h>
#include <stdio.h>
#include <stdbool.h>
#include <string.h>
#include <ctype.h> /* For tolower(). */

#define BUFFER_SIZE 1024

/* It returns true if the given character is a vowel.
 *
 * Arguments:
 *      - chr : the character to check.
 *
 * Return value:
 *      True if 'chr' is a vowel, otherwise false.
 *
 * Notes:
 *      The vowels considered are English vowels (a, e, i, o, u), both in
 *      upper case and lower case.
 */
bool is_vowel(char chr)
{
        static const char VOWELS[] = "aeiou";

        chr = tolower(chr);

        char *result = strchr(VOWELS, chr);

        return (!result) ? false : true;
}

/* It returns the number of vowels found in a null terminated byte string.
 *
 * Arguments:
 *      - str : the null terminated byte string.
 *
 * Return value:
 *      It returns the number of vowels found in the string.
 *
 * Warning:
 *      The behavior is undefined if str is not a pointer to a null-terminated
 *      byte string.
 */
int vowels_in(char *str)
{
        int vowels = 0;

        while (*str) {
                if (is_vowel(*str))
                        vowels++;

                str++;
        }

        return vowels;
}

/* It prints the help message.
 */
void print_help(void)
{
        static const char message[] =
"usage: vowels [-h|--help] <text>\n\n"
"It shows the number of vowels (aeiou) found in a given text. If you don't\n"
"specify text in the command line, the program reads text from stdin.\n\n"
;

        printf("%s", message);
}

int main(int argc, char *argv[])
{
        char buffer[BUFFER_SIZE];
        int vowels = 0;

        if (argc == 2) {
                if (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help")) {
                        print_help();
                        return 0;
                } else {
                        vowels = vowels_in(argv[1]);
                        printf("%d\n", vowels);
                }
        } else if (argc == 1) {
                size_t chars;

                /* Repat until the user type CTRL-D or there is an error. */
                do {
                        chars = fread(buffer, sizeof(char), BUFFER_SIZE, stdin);

                        vowels += vowels_in(buffer);
                } while (chars == BUFFER_SIZE);

                printf("\n%d\n", vowels);
        } else {
                fprintf(stderr, "Invalid options; see 'vowels --help'.\n");
                return -1;
        }

        return 0;
}

Your code is buggy and can quickly invoke undefined behavior.

The function tolower must not be called with a char directly, its argument must be cast to unsigned char first. Read the tolower documentation for details.

bool is_vowel(char chr) {
    int lower = tolower((unsigned char)chr);
    return strchr("aeiou", lower) != NULL;
}

In the part that counts the vowels from stdin, you have a serious bug. You read a large buffer and then check the whole buffer for vowels. But you must only check the part that you have actually read, which can be less.

To demonstrate, create a file of size 1025 that is filled with vowels. Your program will count 2048 vowels in that file. Or even more if you are unlucky. Or it crashes. Now that's what undefined behavior means.

To fix this, remove all the buffering code from your program. Instead, read the input one byte at a time:

size_t count_vowels_in_file(FILE *f) {
    size_t vowels = 0;
    int ch;   // It's important to use "int" here, not "char" or "unsigned char".
    while ((ch = fgetc(f)) != EOF) {
        if (is_vowel(ch)) {
            vowels++;
        }
    }
    return vowels;
}

This code is much simpler than reading buffers, and it is almost as fast, since stdin is already buffered.

In short, you didn't test your code well. Code that reads data must be tested with empty input, short input, long input, random input.

To get comparison data, you can use the following shell command:

tr -cd 'AEIOUaeiou' | wc -c

These commands read from stdin. The tr command deletes all characters except the vowels, and the wc command counts the remaining characters, which are all vowels. For every input file, your C program should produce the same result as the tr | wc command combination.


Drawing and Painting a Menu Component I have produced a working menu component for a project I'm working on, but would like to reduce the amount of code used and improve the methods - especially in the Paint implementation - still further, I feel like the function I've used and the way I've passed the variables to it is a bit 'hacky' - IOW, I bodged it.

What methods could I use to increase the amount of encapsulation used in the drawing routine so I'm not defining variables in the Paint routine? The y2 variable in particular is irritating.

These are my drawing function and paint routine:

// Rectangle drawing function
Function DrawRect(cv : TCanvas; x1, y1, x2, y2, chR, clR, i : integer; txts : AnsiString; Lrect : TRect) : TRect;
begin
  with cv do begin
    // Test against Rect variables to set colour
    if (chR = i) and (clR = -1) then
      Brush.color := stateColor[bttn_on]
    else if (chR = i) and (clR = i) then
      Brush.color := stateColor[bttn_dwn]
    else
      Brush.color := stateColor[bttn_off];

    // Define Link Rectangle
    Lrect := Rect(x1, y1, x2, y2*(i+1));

    // Draw Rectangle
    Rectangle(Lrect);

    // Add text
    TextRect(Lrect, x1+5,  y1+5, txts);

    // Return co-ordinates of drawn rectangle as function result
    result := Lrect;
  end;
end;

// Canvas painting
procedure TOC_MenuPanel.Paint;
var
  y1, x2, y2, count : Integer;
begin
  inherited Paint;
  // Set length of array
  SetLength(MenuRects, fLinesText.Count);

  // Define Y1,Y2
  x2 := Width - Width div 20;
  y1 := Width div 20;

  with Canvas do begin
  // Draw outerRectangle outside of For loop
  Brush.color := clMenu;
  Rectangle(0, 0, Width, Height);

  // Loop for drawing menu items
  if fLinesText.Count = 0 then exit
  else
    for count := 0 to fLinesText.Count - 1 do begin
      // Define Y2
      y2 := TextHeight(fLinesText.strings[count])*2;

      // Cast rectangle draw function results to array for Mouse actions & draw rectangle
      MenuRects[count] := DrawRect(Canvas, 10, y1, x2, y2, chosenRect, clickedRect, count, fLinesText.strings[count], MenuRects[count]);

      // inc y1 for positioning the next box
      y1 := y1+y2;
    end;
  end;
end;

First, I'll go on about how the code looks, since it displays the most obvious issues.

You have redundant comments all over the place.

// Rectangle drawing function
Function DrawRect

The name DrawRect makes it clear enough that you're drawing a rectangle in there. No need for the comment.

Similarly for:

// Define Y2
y2 := TextHeight(fLinesText.strings[count])*2;

And in the case of:

// Define Y1,Y2
x2 := Width - Width div 20;
y1 := Width div 20;

it's not only redundant, but also misguiding, since you're not setting the value of Y2 anywhere in there.

Code should be self-documenting, comments should not explain what you're doing, at most they should explain why you're doing what it is that you're doing. Otherwise, they're just cluttering the file, requiring extra work to maintain and looking ugly.

You should work towards improving the names of your variables.

// Define Link Rectangle
Lrect := Rect(x1, y1, x2, y2*(i+1));

By calling Lrect LinkRectangle or at least LinkRect, you can also discard the comment above the assignment. LinkRect := Rect(...); makes it clear enough that you're setting up a link rectangle.

chR and clR are some awfully confusing names, and unless you look at what parameters are being passed to the function in an actual call, it's pretty hard to understand what they're supposed to do. Consider chosenRect, and clickedRect instead.

bttn_dwn could become button_down (if this is the style you decided to apply to what seem to be unsigned integer constants). txts could become text.

count is more often used to represent the number of items in a particular collection. For a loop counter, consider using a more common and appropriate name. In your particular case, there should be no issue to rename count to i.

And so on...

There is some inconsistency in the way you use the casing for your names.

Although Lrect is a parameter like all the others, it's being cased differently.

You chose to use Function while also using procedure. This sort of stands out, consider changing Function to function or procedure to Procedure, depending on how you want to be consistent. Personally, I prefer to not start with an uppercase character. Mainly because I don't start the rest of the keywords with one either, and I see you've also done that.

You should try to keep these things consistent throughout your code. Choose a way to name your things, and keep it throughout your project.

You could improve the way your code behaves.

if fLinesText.Count = 0 then exit
  else

could well become if fLinesText.Count > 0 then.

However, this too is redundant because for count := 0 to fLinesText.Count - 1 do begin will do nothing if Count is less than 1.

Consider extracting

if (chosenRect = i) and (clickedRect = -1) then
  Brush.color := stateColor[bttn_on]
else if (chosenRect = i) and (clickedRect = i) then
  Brush.color := stateColor[bttn_dwn]
else
  Brush.color := stateColor[bttn_off];

into a function that returns values of the type the stateColor elements have. Then call it from TOC_MenuPanel.Paint, and pass the result as parameter to DrawRect. This will ease the burden caused by the many parameters on DrawRect (you'll be able to get rid of chosenRect and clickedRect). It will also remove the responsibility of knowing that there are chosen and clicked rectangles, from DrawRect.

In the case of:

Lrect := Rect(x1, y1, x2, y2 * (i + 1));

You seem to be altering the rectangle's size based on the rectangle's index in the menu. There is no reason for DrawRect to even know that there's a menu and that the rectangle being drawn has an index in it. The raw value of y2 is not used anywhere else, so consider passing y2 * (i + 1) as parameter (from within MenuPanel.Paint), and getting rid of i as parameter. If you extract the color choosing bit in a separate function, and call it from Paint, doing this too should be trivial.

Consider wrapping up x1 x2 y1 y2 into some higher level abstraction (a record for example) or at least name them appropriately. (x, y, width, height) would go a long way towards making your code more understandable. Right now you have to carefully analyze how the code is written to find out if you're working exclusively with coordinates or if x2 and y2 somehow represent the width and height of the rectangle.

Using a higher level abstraction will also reduce the number of parameters you pass around, so I'd do that myself.

  What methods could I use to increase the amount of encapsulation used in the drawing routine so I'm not defining variables in the Paint routine? The y2 variable in particular is irritating.

Follow the steps above to improve the way DrawRect works. As for the variables showing up in Paint, it depends on how far you want to go with your refactoring. In some form or another, the logic you used there has to exist. I do agree that it's probably better to take it out of the drawing routines.

Personally, I'd extract everything about positioning into some higher level structure that describes the whole layout. My drawing routines would receive the given layout and show it on the screen.