TLE in FOXLINGS SPOJ Problem Statement: FOXLINGS

  It’s Christmas time in the forest, and both the Fox and the Wolf
  families are celebrating. The rather large Fox family consists of two
  parents as well as \$N\$ (\$1 \leq N \leq 10^9\$) little Foxlings. The
  parents have decided to give their children a special treat this year
  – crackers! After all, it’s a well-known fact that Foxen love
  crackers.

  With such a big family, the parents can’t afford that many crackers.
  As such, they wish to minimize how many they give out, but still
  insure that each Foxling gets at least a bit. The parents can only
  give out entire crackers, which can then be divided and passed around.

  With this many children, not all of them know one another all that
  well. The Foxlings have names, of course, but their parents are
  computer scientists, so they have also conveniently numbered them from
  \$1\$ to \$N\$. There are \$M\$ (\$1 \leq M \leq 10^5\$) unique two-way
  friendships among the Foxlings, where relationship \$i\$ is described by
  the distinct integers \$A_i\$ and \$B_i\$ (\$1 \leq A_i,B_i \leq N\$),
  indicating that Foxling \$A_i\$ is friends with Foxling \$B_i\$, and vice
  versa. When a Foxling is given a cracker, he can use his tail to
  precisely split it into as many pieces as he wants (the tails of Foxen
  have many fascinating uses). He can then pass these pieces around to
  his friends, who can repeat this process themselves.

  Input

  Line 1: 2 integers, \$N\$ and \$M\$

  Next \$M\$ lines: 2 integers, \$A_i\$ and \$B_i\$, for \$i=1\ldots M\$

  Output

  A single integer – the minimum number crackers must be given out, such
  that each Foxling ends up with at least a small part of a cracker.

From the question I attempted with disjoint sets. I applied union by size along with path compression. But the constraints are very high and I could not think how to optimize this further. Offcourse I am getting TLE with solution upon submitting. So can anyone tell me any trick or idea regarding optimization?

public static void main(String[] argh) throws Exception{
        // BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        Reader br= new Reader();
        int n= br.nextInt();
        int m= br.nextInt();
        int[] dj= new int[n+1];
        int[] size= new int[n+1];
        for(int i=1; i<=n; i++){
            dj[i]= i;
            size[i]= 1;
        }
        HashMap<Integer, Integer> map= new HashMap<>();
        int count=0;
        for(int i=0; i<m; i++){
            int n1= br.nextInt();
            int n2= br.nextInt();
            union(dj, size, n1, n2);
        }

        for(int i=1; i<=n; i++){
            findParent(dj, i);
        }

        for(int i=1; i<=n; i++){
            if(!map.containsKey(dj[i])){
                count++;
                map.put(dj[i], 1);
            }
        }

        System.out.println(count);
    }

    private static int findParent(int[] arr, int pos){
        if(arr[pos] == pos) return pos;
        return arr[pos]= findParent(arr, arr[pos]);
    }

    private static void union(int[] arr, int[] size, int pos1, int pos2){
        int posP1= findParent(arr, pos1);
        int posP2= findParent(arr, pos2);

        if(posP1 != posP2){
            if(size[posP1] < size[posP2]){
                int temp= posP1;
                posP1= posP2;
                posP2= temp;
            }
            arr[posP2]= posP1;
            size[posP1] += size[posP2];
        }
    }

  Note: Reader is just another fast inputting

for(int i=1; i<=n; i++){
    findParent(dj, i);
}

for(int i=1; i<=n; i++){
    if(!map.containsKey(dj[i])){
       count++;
        map.put(dj[i], 1);
    }
}

This part is slow and unnecessary - you can count how many times you did union instead.

 int[] dj= new int[n+1];

With 10^9 limit for N, only that line requires 4GB of RAM, when limit for task is 1.5GB. Use HashMap instead and store only unioned ids - there will be at most 2*M of those.


Add HTML to the page using a PHP Class I was trying to make my life easier making a class or something that would create HTML tags in a faster way. I ended up with something that I wouldn't consider the best practice. Take a look:

/**
 * Add HTML to the page
 *
 * Add HTML to the page based on arrays with the tag names. It's gonna open and close the tags.
 *
 * @param array tags The name of the tags
 * @param array attributes The attributes of the tags if you want
 * @param array The elements you want inside the tags
 */
function addHtml($tags = null, $attributes = null, $innerHtml = null)
{
    for($i = 0; $i < count($tags); $i++) {
        // Opens the tag
        echo '<' . $tags[$i];

        // If there're attribures, show them and close
        // If not, just closes the tag
        echo !empty($attributes) ? ' ' . $attributes[$i] . '>' : '>';

        // Show something you wanna put inside the tag. Another tag, text, anything
        echo $innerHtml[$i];

        // Closes the tag
        echo '</' . $tags[$i] . '>';
    }
}

Is there someone with a better way of doing it?

When it comes to outputting HTML from PHP, there are a couple of alternatives:

Using a MVC framework which will separate the logic (Controller) from the data (Model) and the output (View).
Include files that contains HTML, and that can use some PHP as well ("Templates"). MVC frameworks normally makes use of this.
For short HTML, I believe there is nothing that beats using PHP's echo function and write the HTML you want manually.

A big problem with your current function is that to output several HTML tags, you'd have to write things that actually belongs together very far apart. For example:

addHTML(array("p", "p"),
  array('class="some"', 'class="other"'),
  array("Lorem ipsum", "dolor sit amet"));

It's not very readable to see that one tag will become
Lorem ipsum
 and the other
dolor sit amet
.

If you really really want to have some self-written PHP function for this, then I suggest putting things that belong together, together. Something like this:

addHTML(array(array("p", "class" => "some", "Lorem ipsum"),
             array("p", "class" => "other", "dolor sit amet")));

However, simply writing this would be so much simpler, and a whole lot easier to read: (Especially for people who are used to reading HTML)

echo '
Lorem ipsum
';
echo '
dolor sit amet
';

(My PHP is a bit rusty but I hope you get the idea)


Tree implementation for unordered keys The following is an implementation of a tree in C++ designed for keys which do not have an order (otherwise a std::map could be used). T is a key type and H is a hash for T.

#include <iostream>
#include <memory>
#include <utility>
#include <unordered_map>

template<typename T, typename H>
class tree;

template<typename T, typename H>
class tree : public std::unordered_map<T, std::shared_ptr<tree<T, H>>, H> { };

template<typename T, typename H>
std::ostream& operator<<(std::ostream& s, const tree<T, H> &t);

template<typename T, typename H>
std::ostream& operator<<(std::ostream& s, const tree<T, H> &t)
{
    for(auto i : t) {
        s << i.first << std::endl;
        s << *i.second << std::endl;
    }
    return s;
}

At least as it stands right now, quite a bit of your code accomplishes nothing.

template<typename T, typename H>
class tree;

template<typename T, typename H>
class tree : public std::unordered_map<T, std::shared_ptr<tree<T, H>>, H> { };

When the declaration immediately precedes the definition like this, the declaration isn't really accomplishing anything--the definition by itself will do the same, so we can consolidate this down to just:

template<typename T, typename H>
class tree : public std::unordered_map<T, std::shared_ptr<tree<T, H>>, H> { };

Likewise, your declaration of the stream insertion operator:

template<typename T, typename H>
std::ostream& operator<<(std::ostream& s, const tree<T, H> &t);

...can be eliminated with no loss. For these declarations to be meaningful/helpful, you'd do something like putting them in a header, with the definitions in a separate file. For most purposes, the class definitions need to be in a header as well though, so the utility of the class declarations may be open to some question. There are cases such things are used (e.g., <iosfwd> contains just "forward" declarations of the iostreams classes) but they're not very widely used (e.g., when was the last time you wrote code that used #include <iosfwd>?)

Another obvious issue is that you're publicly deriving from a standard container. The standard containers aren't really intended to be used as base classes, so this usage is somewhat fragile. Public derivation means it's trivial to assign the address of Tree object to a pointer an unordered_map. If the object is destroyed this way, you get undefined behavior because the base class doesn't declare its dtor as virtual.

// Note that we don't have a cast or anything like that indicate that we're doing
// something dangerous here.
std::unordered_map<T, H> *base = new Tree<T, H>;
// ...
delete base; // undefined behavior here.

...and there's basically nothing you can do from inside the class to prevent a user from doing this--by using public inheritance, you've told the world (and the compiler) that this usage should be perfectly fine, so the compiler doesn't require an explicit cast to do the conversion.


User system using PDO I'm making a very basic user system, and this is what I have so far:

<?php

/* The class for constructing any user's information
 */

    class User {

        protected $userId, $email, $userGroup;

        protected function getEmail() {
            return $this->email;
        }

        protected function getUserId() {
            return $this->userId;
        }

        public function __construct($userId, $email, $emailGroup) {
            $this->userId = $userId;
            $this->email = $email;
            $this->userGroup = $emailGroup;
        }

        // Method for checking whether user can view a specific page
        public function canViewPage($pageId) {
            $userGroup = new UserGroup($this->userGroup);
            if($userGroup->canViewPage($pageId)) {
                return true;
            }else{
                return false;
            }
        }

        // Method for checking permissions
        public function hasPermissions($alias) {
            /* alias is the name of the permissions. ie can-book
            * create usergroup object based on current user's usergroup */

            $userGroup = new UserGroup($this->userGroup);
            if($userGroup->hasPermissions($alias)) {
                return true;
            }else{
                return false;
            }
        }

    }

    class UserMapper {
        // database connection
        private $db;

        public function __construct($db)
        {
            $this->db = $db;
        }

        public function findByUserId ($userId) {
            $userObject = new User();
            $q = $this->db->prepare("SELECT userId, email, userGroup FROM user WHERE userId = :userId");
            $q->bindValue(":userId", $id);
            $q->setFetchMode( PDO::FETCH_INTO, $userObject);
            $q->execute();
            $q->fetch(PDO::FETCH_INTO);
            return $userObject;

        }
    }
?>

Then on my main script, I include the user.class.php (as shown above) and do this:

<?php
    $dbh = new PDO('mysql:host=localhost;dbname=test', $user, $pass, array(PDO::ATTR_PERSISTENT => true));
    $getUser = new UserMapper($dbh);
    $user = $getUser->findByUserId(41);
    echo $user->getEmail();
?>

Is this all correct? Or have I misunderstood? Is there a way of simplifying this? I've tried to keep the code as simple as possible.  Am I correct for having getters (getEmail, etc.) as well as methods such as canViewPage, within the user class?

Also, should it be:

$userGroup = new UserGroup($this->userGroup);

or should I do something like this again?

$dbh = new PDO('mysql:host=localhost;dbname=test', $user, $pass, array(PDO::ATTR_PERSISTENT => true));
        $getUserGroup = new UserGroupMapper($dbh);
        $userGroup = $getUserGroupMapper->findByUserId($this->userGroup);

Is there a simpler way?

I didn't change much.
I think you're correct in having getters as well as methods such as 'canViewPage'.
The Methods 'canViewPage' and 'hasPermissions' are just wrappers around the 'UserGroup' class, the logic is in 'UserGroup'. It makes sense to create wrappers around 'UserGroup', for example if you want to have also Permission based solely on the User(Example a root user) then you would just have to change these Methods. If you had invoked the Methods directly on the 'UserGroup' - not over the 'User' - you would have to go over all these places. Pherhaps it would be Usefull to have a 'Permissions' class, wich would have a constructor or factory method, where you could give an 'User' as Argument.

<?php
/* The class for constructing any user's information
*/

class User {

    protected $userId, $email, $userGroup;

    protected function getEmail() {
        return $this->email;
    }

    protected function getUserId() {
        return $this->userId;
    }

    public function __construct($userId, $email, $emailGroup) {
        $this->userId = $userId;
        $this->email = $email;
        // You never use userGroup directly, you can just
        // put the object as membervariable, so you don't
        // have to instantiate the object twice
        $this->userGroup =  new UserGroup($emailGroup);
    }

    // Method for checking whether user can view a specific page
    public function canViewPage($pageId) {
        /*
        if(condition){
            return true;
        else
            return false;
        is equals
        return (bool) condition
        */
        return (bool) $this->userGroup->canViewPage($pageId);
    }

    // Method for checking permissions
    public function hasPermissions($alias) {
        /* alias is the name of the permissions. ie can-book
        * create usergroup object based on current user's usergroup */
        return (bool) $this->userGroup->hasPermissions($alias);
    }

}

class UserMapper {
    // database connection
    private $db;

    public function __construct(\PDO $db)
    {
        $this->db = $db;
    }

    public function findByUserId ($userId) {
        $userObject = new User();
        $q = $this->db->prepare("SELECT userId, email, userGroup FROM user WHERE userId = :userId");
        $q->bindValue(":userId", $id);
        $q->setFetchMode( PDO::FETCH_INTO, $userObject);
        $q->execute();
        $q->fetch(PDO::FETCH_INTO);
        return $userObject;

    }
}
?>

I don't understand your Second Questions, pherhaps you made some typos?
You have '$getUserGroup', but use '$getUserGroupMapper' and you give the method 'findByUserId' a UserGroup.

EDIT:
Now I understand. If you have a lot of UserGroups, I think it would be good. If you have just around 5 UserGroups, I thinks it would be too much

Your main program is good, for being an example.


Creating tiles with CSS I am still a beginner in CSS and am trying to make a tiles/cards design. I have reached this code, and I want to know if my layout this way is good or if there are any improvements I need to make.

Is writing margin and padding auto is the right practice? How about using flexbox as am doing?

html{
  font-size: 12px;
  font-family: Arial, sans-serif;
}

body{
  margin: auto;
  padding: auto;
  /* What's the best practice to align content in the middle */
}

h1{
  background-color: red;
  text-align: center;
  height: 50px;
  vertical-align: /*What to put here to make it centered? */  ;
}

.flex{
  display: flex;
}

.item{
  width: 30%;
}
.item  h3{
    color: grey;
  }

   .item span{
    font-style: italic;
  }
<!DOCTYPE html>

    <link rel="stylesheet/less" href="style.less" />

Hello Plunker!

        Project 1

        Time: 10:30AM

        Project 2

        Time: 10:30AM

        Project 3

        Time: 10:30AM

        Project 4

        Time: 10:30AM

        Project 5

        Time: 10:30AM

I'm going to have to heavily disagree with Tolani Jaiye-Tikolo's answer. Using ID's isn't good CSS authoring generally (however this rule can be broken, like all rules.) This is because you create overly specific CSS that can only be used once.

Another point i'd like to make about their feedback: please only use tables for tabular data. Using them as structural elements is not the way to go. If you want to use the properties of a table element, use display: table;.

That said, onto some critique of your code. You asked what the best way to center align content is. There's several methods for achieving this, some very common ways are:

use text-align: center; on a parent container. This only affects inline or inline-block elements.
use margin: 0 auto; on a element. This only works on block type elements.

I noticed you're also looking to vertically align an element. This is a little more complex than horizontally aligning an element (But only slightly). I personally use the following two methods often:

Place display: table; on the parent/container element. Place display: table-cell; and vertical-align: middle; on the child element (the one you want to vertically center). The child element should sit in the middle of your parent vertically.
Other method is to make your element display: inline-block; and give it vertical-align: middle. It will center vertically according to its siblings.

There's other things you can improve on but unfortunately I'm at work so I can't provide examples and a more comprehensive answer, sorry!


Easier way to handle a Console Theme System I'm just curious is there an easier way to handle console colors rather than just doing something
like the code below. The problem is the code bloat I'm experiencing when writing a theming system for my console application. It contains almost 200 lines of code, just to handle the chat theme system. Is there a way to minimalize this?

public static enum UserLevel
{
    Moderator, Broadcaster, Admin, Subscriber, Normal, Turbo
}

public static void WriteLine(UserLevel l, string user, string message)
{
    DateTime ts = new DateTime();
    string t = ts.ToLongTimeString();

    Console.BackgroundColor = ConsoleColor.Black;
    Console.Write("[" + t + "] ");
    Console.ForegroundColor = ConsoleColor.Green;
    Console.Write("<");

    switch (l)
    {
        case UserLevel.Moderator:
            Console.ForegroundColor = ConsoleColor.Blue;
            Console.Write("[M] ");
            Console.ForegroundColor = ConsoleColor.White;
            Console.Write(user);
            break;
        case UserLevel.Broadcaster:
            Console.ForegroundColor = ConsoleColor.Blue;
            Console.Write("[B] ");
            Console.ForegroundColor = ConsoleColor.White;
            Console.Write(user);
            break;
        case UserLevel.Admin:
            Console.ForegroundColor = ConsoleColor.Blue;
            Console.Write("[A] ");
            Console.ForegroundColor = ConsoleColor.White;
            Console.Write(user);
            break;
        case UserLevel.Subscriber:
            Console.ForegroundColor = ConsoleColor.Blue;
            Console.Write("[S] ");
            Console.ForegroundColor = ConsoleColor.White;
            Console.Write(user);
            break;
        case UserLevel.Normal:
            Console.ForegroundColor = ConsoleColor.White;
            Console.Write(user);
            break;
        case UserLevel.Turbo:
            Console.ForegroundColor = ConsoleColor.Blue;
            Console.Write("[T] ");
            Console.ForegroundColor = ConsoleColor.White;
            Console.Write(user);
            break;
        case default:
            Console.ForegroundColor = ConsoleColor.White;
            Console.Write(user);
            break;
    }

    Console.ForegroundColor = ConsoleColor.White;
    Console.Write(user);
    Console.ForegroundColor = ConsoleColor.Green;
    Console.Write("> ");
    Console.ForegroundColor = ConsoleColor.White;
    Console.Write(message);
}

var keyword:

Use the var keyword wherever you can instead of declaring your variables explicitly. The compiler will determine the type and your code looks cleaner. Example:

var dateTime = new DateTime();
var time = dateTime.ToLongTimeString();

Small changes:

Variable names: l, ts or t are names that don't mean a thing, not for you, not for others. Use meaningful names such as userLevel or time.
DateTime: in the chat you want to show the time. I think you mean to use DateTime.Now instead of new DateTime(). The latter will just return 0:00:00 when you're calling the ToLongTimeString().
String.Format: don't concatenate hardcoded values and variables. Use the String.Format method. Following line:

Console.Write("[" + t + "] ");

will become:

Console.Write("[{0}] ", time);

This is easier to read and maintain.

Shorten your code:

Finally the point where you've been waiting for. Although there is a difference depending on the user-level, it is small. And certainly small enough to create a small method to avoid repetition. First, get the code based on the level and use the result to write to the console. This is done in following method:

private static string GetCodeFromUserLevel(UserLevel level)
{
    switch (level)
    {
        case UserLevel.Admin: return "[A] ";
        case UserLevel.Broadcaster: return "[B] ";
        case UserLevel.Moderator: return "[M] ";
        case UserLevel.Subscriber: return "[S] ";
        case UserLevel.Turbo: return "[T] ";
        default: return "";
    }
}

Now that you have this, you can use the result to write your message. This is done in the already existing message, but it looks cleaner now:

public static void WriteLine(UserLevel level, string userName, string message)
{
    var dateTime = DateTime.Now;
    var time = dateTime.ToLongTimeString();

    Console.BackgroundColor = ConsoleColor.Black;
    Console.Write("[{0}] ", time);
    Console.ForegroundColor = ConsoleColor.Green;
    Console.Write("<");

    Console.ForegroundColor = ConsoleColor.Blue;
    Console.Write(GetCodeFromUserLevel(level));
    Console.ForegroundColor = ConsoleColor.White;
    Console.Write(userName);

    Console.ForegroundColor = ConsoleColor.Green;
    Console.Write("> ");
    Console.ForegroundColor = ConsoleColor.White;
    Console.WriteLine(message);
}

This code already looks better but you can take this a step further. You see three similar blocks with following structure:

Set a color
Write a part
Set a color again
Write a part again

This can be extracted in two ways: take step 1 and 2 and place it in a method or take step 1 to 4 and extract it. Here's the method when taking all steps in one method:

private static void WritePart(ConsoleColor startColor, ConsoleColor endColor, string partOne, string partTwo, bool newLine)
{
    Console.ForegroundColor = startColor;
    Console.Write(partOne);
    Console.ForegroundColor = endColor;
    Console.Write("{0}{1}", partTwo, newLine ? Environment.NewLine : "");
}

And here's the usage for the three blocks:

WritePart(ConsoleColor.White, ConsoleColor.Green, String.Format("[{0}] ", time), "<", false);
WritePart(ConsoleColor.Blue, ConsoleColor.White, GetCodeFromUserLevel(level), userName, false);
WritePart(ConsoleColor.Green, ConsoleColor.White, "> ", message, true);

I know that in your code you first set the BackgroundColor, you can add that line separatly before the above three lines.

Perhaps more logic is to only take step 1 and 2 and extract it in a method as it avoids repetition. Here's what it would look like:

private static void WritePart(ConsoleColor color, string value, bool newLine)
{
    Console.ForegroundColor = color;
    Console.Write("{0}{1}", value, newLine ? Environment.NewLine : "");
}

And the usage:

WritePart(ConsoleColor.White, String.Format("[{0}] ", time), false);
WritePart(ConsoleColor.Green, "<", false);

WritePart(ConsoleColor.Blue, GetCodeFromUserLevel(level), false);
WritePart(ConsoleColor.White, userName, false);

WritePart(ConsoleColor.Green, "> ", false);
WritePart(ConsoleColor.White, message, true);

It's what you prefer. Hope this helps!

Update:

Another tip on the method to get the code depending on the level. You have to options here make it shorter/cleaner.

Use following method. This is certainly not the cleanest solution but since the code is the first letter of the enum value you can use this:

private static string GetCodeFromUserLevel(UserLevel level)
{
    return level == UserLevel.Normal ? "" : String.Format("[{0}] ", level.ToString()[0]);
}

The other method is coming from this question on StackOverflow: Enum to String?. Check out the answer and you can apply this too.


Find first index of 1 Given a sorted array consisting 0’s and 1’s. The task is to find the index of first ‘1’ in the given array. I submitted the below code in geeks for geeks and the execution time is 5.77. Need help in optimizing the below code.
class FindIndex {
    public static void main (String[] args) {
        Scanner scanner = new Scanner(System.in);
        int noOfTestCase = scanner.nextInt();

        while (noOfTestCase-- > 0) {
            int n = scanner.nextInt();
            int[] array = new int[n];
            int index = -1;
            for (int i = 0 ; i < n ; i++) {
                array[i] = scanner.nextInt();
                if (array[i] == 1 && index == -1) {
                    index = i;
                }
            }
            System.out.println(index);
        }
    }
}

Source

Trying to outsmart the challenge defeats the purpose
While there is value in learning how to do the bare minimum needed (as you try to detect the number as soon as it is inputted instead of performing a search), there is only so much you can learn by trying to outsmart the challenge and not practice the knowledge this challenge is supposed to help you practice (which in this case is binary search).
Input reading
You can read the input faster using BufferedReader, see this answer's "Reading Data" section for example. You can read a whole line at once and then perform a search.
Binary search
Binary search is a very fast way to find an item in a sorted array where by looking at any element you know if what you're looking for is on the left or right of the current position.
You can perform the search on the string, converting the string to an array of numbers is unnecessary and therefore a waste of time.


Wrapping a .NET Database Connection I have created a DatabaseConnection class as this:

class DatabaseConnection
{
    public SqlConnection sqlconn { get; set; }
    public DatabaseConnection(string server, string db, string user, string pass)
    {
        sqlconn = new SqlConnection(@"Data Source=" + server + ";Initial Catalog=" + db + ";User ID=" + user + ";Password=" + pass);
        OpenConn();
    }

    public SqlConnection OpenConn()
    {
        try{
            if (sqlconn != null && sqlconn.State == ConnectionState.Closed)
                sqlconn.Open(); // MessageBox.Show("Connection has been successfully established");
        }

        catch(Exception ex){
            MessageBox.Show(ex.Message);
        }

        return sqlconn;
    }
}

Its only purpose is to open the SqlConnection. I am just following the Single Responsibility Principle.

And then, in every class that needs a DatabaseConnection, this is my approach:

class DataViewer
{
    private DatabaseConnection dbCon;
    private SqlConnection sqlconn;
    private SqlDataAdapter sqlDA;

    //Here I am applying constructor injection
    public DataViewer(DatabaseConnection dbCon)
    {
        this.dbCon = dbCon;
        this.sqlconn = dbCon.sqlconn;
    }
    public DataGridView getDataInStoredProc(DataGridView dtgrdView, string storedProcName, params SqlParameter[] parameters)
    {
        try{
            //Some logic here
            sqlconn.Close();
        }

        catch (Exception ex){
            MessageBox.Show(ex.Message);
            sqlconn.Close();
        }
        return dtgrdView;
    }
}

Now, I would like to know if I am doing good or bad approach for this. If not, could you please explain me why and give me some advice on how to do it right?

I see a few issues with your class:

Issue 1

This property is open so anyone can set it:

public SqlConnection sqlconn { get; set; }

You probably want to make the setter private so only you can set it from within the class and it can be accessed from outside.

Issue 2

The property name should be SqlCon because the convention is to use Pascal naming in C#.

Issue 3

You have this in your constructor: OpenConn(); which means as soon as someone creates an instance of your class, you open the connection but what if the user of your class does not want to open the connection yet?

Issue 4

Why are you making the assumption that the user of you class will be using windows forms? What if I use your class from a console application, how will MessageBox.Show work?

catch(Exception ex){
    MessageBox.Show(ex.Message);
}

Issue 5

What if the user of your class forgets to close the connection?

Issue 6

What are you getting from this class? I will not use your class because the one in .NET is much better and easier to use. I can use it like this:

using (SqlConnection connection = new SqlConnection(connectionString))
{
    connection.Open();
    // Do my work
}

So if I am on your team and you tell me to use your class, which is a wrapper, first question I would ask you is: Why? And then if I find out that as soon as I create an instance of your class, you are opening database connections, I will not be very happy.

Patterns

Patterns such as Single Responsibility Pattern (actually it's a principle but beyond the point) and many other patterns that are there should be used by programmers who have a black belt, or at least a few belts, in programming. If someone is learning programming, they should not even bother with patterns. Patterns are the cool thing and with names like Singleton, Liskov Substitution Principle, Memento Pattern and other fancy names, I have to admit they are so tempting. People throw these terms around because they want to sound smart but do not give into this temptation. People want to use them instead of just creating a plain solid class.

Many people on StackOverflow ask questions such as "Why do people give so much attention to data structures, will I ever need to create one?" My answer to this question is simple: Yes, you will create them every single time you create a class. Just because you are creating a class called DatabaseConnection or Employee does not mean they are not data structures. They are. They are encapsulating data and you better make sure no one puts the state of your class into a bad state: like you have the sqlconn property public.

Forget Patterns for now

Do not worry about patterns for now. Just study data structures and learn how much effort is put into making the data structure so it is never in a bad state. Pay attention to how the operations are defined so users can interact with the data, add data, manipulate data, delete data and what it does when someone tries something they should not be allowed. In other words, when someone tries something the are not allowed does the data structure show a message box? Does it write to the console? Does it write to a file?

At the end of the day, in C# that is and many other OO languages, data structures are just good solid classes. If you know how to make your own ArrayList class, which is an array that can grow and shrink, and you can make it in a way that the class will never be in a bad state, regardless of how irresponsible or clever the programmer who is using your class is, then you will know how to make good solid classes.

Then you can start looking at patterns.

I am sorry if I sound harsh in any way, it is just because I want so speak plainly and I think this is the best way to get the message across.


Ruby Anagrams Checker I am quite new to Ruby, coming from a JavaScript background. I wrote this simple Ruby script that finds whether two strings are anagrams or not.

To run it: ruby ruby_anagrams.rb 'script1' 'script2'

I am looking for comments and a code review on my script. Can I do anything better? Is this good Rubyism?

# removing whitespace (in case of multi-word anagrams), converting to lowercase and getting
# array representations:
string_1_arr = ARGV[0].gsub(/\s+/, "").downcase.split("")
string_2_arr = ARGV[1].gsub(/\s+/, "").downcase.split("")

if string_1_arr.size != string_2_arr.size
  puts "Not anagrams"
  exit
end

string_1_arr.each do |c|
  print c, " ",string_2_arr, "\n" # debug statement, but left here 'cuz the output looks cool

  # just delete would delete all occurences of the same letter, e.g. "food" would become
  # "fd" if we delete 'o', which is obviously wrong. Have to use delete_at(index) instead
  if i = string_2_arr.index(c)
    string_2_arr.delete_at i
  else
    puts "Not anagrams"
    exit
  end
end

puts "Anagrams!"

You should abstract the code to avoid duplication. Also, use a counter with O(n) performance:

module Enumerable # or require 'facets/enumerable/frequency'
  def frequency
    each_with_object(Hash.new(0)) { |item, counter| counter[item] += 1 }
  end
end

def anagrams?(s1, s2)
  frequency = proc { |s| s.gsub(/\s+/, "").downcase.chars.frequency }
  frequency.(s1) == frequency.(s2)
end

if ARGV.size != 2
  $stderr.puts("Need two words")
  exit(1)
elsif anagrams?(*ARGV)
  puts("Anagrams")
else
  puts("Not anagrams")
end


Reducing Inventory Quantity When Order Is Being Processed I've got a method which removes quantity from inventory when an order is completed. If there is not enough inventory quantity available, the order completion fails and the sql transaction is rolled back. Otherwise, transaction is commited.
I'm using Dapper for MySQL database access
        public async Task<(bool, string)> CompleteInvoice(Invoice invoice, List<InvoiceLineItem> lineItems)
        {
            bool success = true;
            string errorProductNumber = null;

            if (!await UpdateInvoiceLineItemsAsync(lineItems))
                return (false, "Error saving line items. Please try again.");
            //Check to see if there is available quantity
            using (var conn = _InvoiceRepository.DbConnection)
            {
                conn.Open();

                // create the transaction
                using (var trans = conn.BeginTransaction(System.Data.IsolationLevel.RepeatableRead))
                {
                    try
                    {
                        var selectProductSQL = "SELECT InventoryNumber,Quantity FROM Product WHERE Id = @productId";
                        var updateProductSQL = "UPDATE Product SET Quantity = @NewQuantity WHERE Id = @productId";
                        DynamicParameters productParameters;
                        foreach (var lineItem in lineItems)
                        {
                            productParameters = new();
                            productParameters.Add("productId", lineItem.ProductId, System.Data.DbType.String);
                            var cprod = await conn.QueryFirstOrDefaultAsync<Product>(selectProductSQL, productParameters, trans);

                            if (cprod.Quantity >= lineItem.Quantity)
                            {
                                productParameters.Add("NewQuantity", cprod.Quantity - lineItem.Quantity, System.Data.DbType.Int32);
                                if (await conn.ExecuteAsync(updateProductSQL, productParameters, trans) != 1)
                                {
                                    success = false;
                                }
                            }
                            else
                            {
                                errorProductNumber = cprod.InventoryNumber;
                                success = false;
                            }
                            if (!success)
                                break;
                        }
                        if (success)
                        {
                            invoice.InvoiceStatus = "COMPLETE";
                            if (!await UpdateInvoiceAsync(invoice))
                            {
                                success = false;
                                errorProductNumber = "Error saving invoice";
                            }
                        }
                        // if it was successful, commit the transaction
                        if (success)
                            trans.Commit();
                        else
                        {
                            trans.Rollback();
                        }
                    }
                    catch (Exception ex)
                    {
                        // roll the transaction back
                        trans.Rollback();

                        // handle the error however you need to.
                        //throw;
                    }
                }
            }
            return (success, errorProductNumber);
        }

I'm open and any General feedback.
Am I correctly using the SQL Transaction mechanism?(Also with Dapper?)

Are you sure that you need two queries for this?
E.g. you could do something like this:
UPDATE Product
SET    Quantity = Quantity - @usedQuantity
WHERE  Id = @productId
  AND  Quantity >= @usedQuantity

Try to execute that and if it doesn't affect 1 row - you don't have enough quantity for the line item.
The drawback for your current logic is that you no longer get the InventoryNumber for when there's an error. I think there are two reasons why this is a better approach anyway:

You now execute 1 query per line item on the happy path.
I think you have UI logic intermingled here - this code should just deal with ProductId. If you want to display something else, that's on the UI code to do.

Other points:
The (bool, string) return type is not idiomatic C#. I don't mind this pattern but am aware I am not in the majority with that opinion. In C#, an exception would probably be expected here. The other option is to create some kind of Result class, there are many examples of this around. E.g. https://github.com/altmann/FluentResults. It's still not recognized by most C# developers though.
_InvoiceRepository.DbConnection - this is not a good place to get configuration from. An IConnectionProvider interface that is used by all the data access may be a better option. Without more code, it's hard to tell.
// if it was successful, commit the transaction - don't comment like this. Anyone reading your code can understand if (success) Commit(). Comments are for why not what.
It looks like you're trying to keep one return statement and that is causing more conditionals with if (success) around the place. I am a big proponent of returning early but I know not everyone agrees. If you only want a single return, you need to at least factor out methods to get rid of the spaghetti.
errorProductNumber = "Error saving invoice"; - That's not a product number. I'm almost sure that you're displaying this string somewhere. Keep the UI concerns out of the infrastructure code.
This seemingly innocuous pattern is a big problem on more established code bases:
invoice.InvoiceStatus = "COMPLETE";
if (!await UpdateInvoiceAsync(invoice))

You have baked in what a complete invoice looks like in an arbitrary place in the code! What if you need to add a CompletedBy or CompletedDate? You have to find all these places and update them. You also have a magic string - it needs to be a constant. Consider instead:
invoice.Complete();
if (!await UpdateInvoiceAsync(invoice))

Now if you add a CompletedBy, you do it in that method and add a parameter. The compiler then ensures you haven't forgotten to update any places. You now have an appropriate place to introduce a constant for the magic string too:
public void Complete()
{
    // You can also add a guard to prevent invalid completion.
    if (!CanCompleteInvoice)
        throw new BusinessConstraintVoilatedException();

    this.InvoiceStatus = COMPLETED_STATUS; // <- name your constants however you like.
}

Overall this method does too much and is too long. You want each method to be scannable, pseudocode:
public async Task<(bool, string)> CompleteInvoice(
    Invoice invoice,
    List<InvoiceLineItem> lineItems)
{
    // Precondition checks omitted

    using (var conn = _InvoiceRepository.DbConnection)
    {
        conn.Open();
        using (var trans = conn.BeginTransaction(System.Data.IsolationLevel.RepeatableRead))
        {
            foreach (var lineItem in lineItems)
            {
                var lineItemResult = UpdateLineItem(lineItem, ...);
                // ... on error, do something
            }
            UpdateInvoiceAsync(invoice);
        }
    }
}


Extracting a subgraph Given a huge graph, I want to traverse it, in the fastest way possible, and extract a subgraph with the following properties:

the starting node is specified
the maximum number of elements and max depth can be specified
if there are more links than the number specified, I want to save the information about the original number of links

To save all information I am using a class:

    class Link:
    def __init__(self, node, sons = None ,weight = None, depth = None):
        # name of the node
        self.node = node
        # number of links in the original graph
        self.weight = weight if weight is not None else 0
        # list of links in the reduced graph
        self.sons = sons if sons is not None else []
        # depth of the node with respect to the starting node
        self.depth = depth if depth is not None else 0

Note that the original graph is a dictionary, which can have loops.

    def reduce_graph(graph, start, max_depth, max_links = None):

        if start not in graph:
            return None

        to_visit = [Link(start, depth = 0)]
        visited = {}
        while to_visit:
            page = to_visit.pop(0)

            if page.depth >= max_depth:
                # we don't need to traverse this node, just save the number of links
                # in the original graph, if any
                try:
                    page.weight = len(graph[page.node])
                except KeyError:
                    page.weight = 0

                # let's make sure we will not visit this node again
                visited[page.node] = page
                continue

            else:
                # we are visiting a new node
                try:
                    # str(l) to ensure compatibility with imported json files
                    # saving at most max_links links
                    links = [Link(str(l), depth = page.depth + 1) for l in graph[page.node][:max_links]]
                    # number of links in the original graph
                    page.weight = len(graph[page.node])
                    page.sons = links
                    visited[page.node] = page
                    to_visit.extend([l for l in links if l.node not in visited])

                except KeyError:
                    # this node in not present in the original graph
                    page.weight = 0
                    visited[page.node] = page

        return visited

You may be visiting nodes more than once, because you only check if l.node not in visited when adding nodes to to_visit. The node may be in to_visit already. The correct place for the check is right after to_visit.pop(0).
to_visit should perhaps be a collections.deque to benefit from the efficient popleft() function
The comment says you use str(l) to ensure compatibility somehow. I'm not sure what you mean. I'm concerned because str(l) becomes page.node which is used to index graph again, which may not work depending on the original type of l.


Serial port write buffer I'm writing to some serial comm port with specific bytes set into buffer. My protocol for write buffer is written below. Platform is Windows and IDE is Visual C++.

Byte 0 will have message type indicator (Byte)
Byte 1-4 will have message number (Uint32)
Byte 5-8 will have unix epoch time (Uint32)
Byte 13-14 will have number of sample packets (Uint16)
Bytes 15+ (upto 60) will have sample data packets (Uint16)
Byte 15+(No of samples*2) will have end of message (Byte)

Now I'm concern about buffer building (raw Bytes) from integer or short values.

void ConvertIntToByte(int iValue, BYTE* ucRawBuffer, int iOffset)
{
  ucRawBuffer[iOffset]   = ((iValue >> 24) & 0xFF);
  ucRawBuffer[iOffset+1] = ((iValue >> 16) & 0xFF);
  ucRawBuffer[iOffset+2] = ((iValue >> 8) & 0xFF);
  ucRawBuffer[iOffset+3] = (iValue & 0xFF);
}

void ConvertShortToByte(short iValue, BYTE* ucRawBuffer, int iOffset)
{
  ucRawBuffer[iOffset]   = (iValue >> 8) & 0xFF;
  ucRawBuffer[iOffset+1] = iValue & 0xFF;
}

void DataThreadFunction()
{
  while(!m_bClosed)
  {
    DWORD dwRet = WaitForSingleObject(m_hDataSendEvent, INFINITE);
    if(dwRet == WAIT_OBJECT_0)
    {
      ResetEvent(m_hDataSendEvent);

      int iUTCTime = GetEpochTime();
      m_iMsgSequence++;
      ASSERT(m_iMaxSamples != 0);

      BYTE ucXmtdat[60] = {0};
      ucXmtdat[0] = 0x0B;
      ConvertIntToByte(iUTCTime, ucXmtdat, 1);
      ConvertIntToByte(m_iMsgSequence, ucXmtdat, 5);
      ConvertIntToByte(m_iMaxSamples, ucXmtdat, 13);
      int iOffset = 15;
      // m_data - member vector populating from some other method
      for(unsigned int i = 0; i < m_data.size(); i++)
      {
        short iCurrData = m_data[i];
        ConvertShortToByte(iCurrData, ucXmtdat, iOffset);
        iOffset += sizeof(unsigned short);
      }

      m_data.clear();
      iOffset = 15+(m_iMaxSamples*2);
      ucXmtdat[iOffset] = 0xCA;

      bool bSuccess = m_objComm.WriteCommPort(ucXmtdat, 60);
    }
  }
}

I see a number of things that may help you improve your program.

Don't use Hungarian notation

Read this for an explantion of why you shouldn't use Hungarian notation.

Use unsigned types where appropriate

It's unlikely that the values you are passing to ConvertShortToByte or ConvertIntToByte are intended to be signed values.  Signed values are treated differently than unsigned values when shifted because of sign extension.  In this case, that's almost certainly not what is needed.

Return something useful from functions

The conversion functions would be more useful if they returned a pointer just past the current value.  Also rather than a pointer and index, why not just pass a pointer?  Rewritten that would look like this:

BYTE *ConvertShortToByte(unsigned short value, BYTE* buffer)
{
  *buffer++ = value >> 8;
  *buffer++ = value;
  return buffer;
}

Eliminate "magic numbers"

This code has a number of "magic numbers," that is, unnamed constants such as 0x0B, 0xCA, 60 etc.  Generally it's better to avoid that and give such constants meaningful names.  That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "60" and then trying to determine if this particular 60 is relevant to the desired change or if it is some other constant that happens to have the same value.

Use an object

A packet is a natural for an object, so why not make a Packet class?  Here's one way to do it:

class Packet {
public:
    Packet (unsigned seq, unsigned time, const std::vector<unsigned short>& data)
    {
      BYTE *buff = buffer;
      *buff++ = MSG_TYPE;
      buff = ConvertIntToByte(time, buff);
      buff = ConvertIntToByte(seq, buff);
      buff = ConvertIntToByte(0, buff); // filler
      buff = ConvertShortToByte(data.size(), buff);
      for (const auto item : data) {
          buff = ConvertShortToByte(item, buff);
      }
      *buff++ = MSG_END;
    }
    size_t size() const { return 16 + (((buffer[13] << 8) + buffer[14]) << 1); }
    static constexpr BYTE MSG_TYPE = 0x0B;  // start of packet
    static constexpr BYTE MSG_END = 0xCA;  // end of packet
    static constexpr size_t BUFF_SIZE = 60;
private:
    BYTE buffer[BUFF_SIZE];
};

If you need access to the underlying buffer, there are a few ways to do it.  First, you could simply make the buffer a public data member (or alternatively make Packet a struct).  Second, you could provide a handle that would return a const * reference to the internal buffer.  Third, you could eliminate the need by providing, for example a write member function.  Fourth, you could declare the buffer in place and then use "placement new" which is a common idiom for communications protocols in which a packet is built in a fixed location buffer.  Since many people seem unfamiliar with the use of "placement new", I'll elaborate.

Use "placement new"

If a buffer is already allocated and you want to create a Packet in place, it can be done using "placement new":

BYTE ucXmtdat[60];
Packet *pkt = new (ucXmtdat) Packet(m_iMsgSequence, iUTCTime, m_data);

This creates a Packet at the location specified by ucXmtdat without allocating further memory.  Note that when this form is used, it is entirely up to you to make sure that the memory area is really big enough and it's entirely up to you to destroy the object when finished.  In this case, there's not really anything to do since ucXmtdat (which is not a very nice name, by the way) is local and will simply disappear when it goes out of scope.


Implement bubble sort in Python I have written the following code for bubble sort. How can it be optimised?

def bubble_sort(details):
    """compares two consecutive integers in a list and shifts the smaller one to left """
    for i in range(len(details)-1):
        try:
            if details[i] > details[i+1]:
                temp = details[i]
                details[i]= details[i+1]
                details[i+1] = temp
                bubble_sort(details)
        except IndexError:
            return
    return details

sort_me = [11,127,56,2,1,5,7,9,11,65,12,24,76,87,123,65,8,32,86,123,67,1,67,92,72,39,49,12, 98,52,45,19,37,22,1,66,943,415,21,785,12,698,26,36,18,97,0,63,25,85,24,94,1501]
print sort_me
print bubble_sort(sort_me)

thanks for sharing your code.

There are a few things I wanted to talk about.

You shouldn't try / except an IndexError as a form of control flow.

An IndexError here would be the programmer's mistake, you should make sure that your code can't even raise an IndexError in the first place by using correct conditions in your loops. You can also removed the try/except sections completely and your code still behaves the same way.

Recursion

BubbleSort is an inefficient sorting algorithm when dealing with large inputs. The sheer number of comparisons done with larger inputs will cause some serious problems if you recursively call the function.

With the example input you gave, your function gets called 568 times. I tried sorting a list of double the size and got this

RecursionError: maximum recursion depth exceeded in comparison

Using a more straightforward iterative approach would be much more efficient.

def bubble_sort(details):
    """compares two consecutive integers in a list and shifts the smaller one to left """
    for i in range(len(details) - 1):
        swapped = False
        for j in range(len(details) - 1):
            if details[j] > details[j+1]:
                temp = details[j]
                details[j] = details[j+1]
                details[j+1] = temp
                swapped = True

        if not swapped:
            break

    return details

sort_me = [11,127,56,2,1,5,7,9,11] * 100 # give it a large input
print sort_me
print bubble_sort(sort_me)

A few additional things,

Consider implementing a version that doesn't modify the original list, think sorted(list) vs list.sort().
Your current implementation sorts the method in place but also returns the same list. If your method is mutating the list, consider not having a return value (or returning None instead).

Hopefully this was useful for you!


Diagonal Movement Code for Python Chess Engine I'm currently in the process of building my own Chess game engine and could really use some suggestions on how to make this segment of code for calculating diagonal moves more efficient. (This is obviously only for diagonals going Up-Right.)

As of now I'm using "Try-Except" to iterate by 1, and then my return statement filters out any off-board values. However this seems like a very bulky way of doing things.

import argparse, json

chessBoard = [[1, 1, 1, 1, 1, 1, 1, 1] for i in range(8)]

chess_map_from_alpha_to_index = {
    "a" : 0,
    "b" : 1,
    "c" : 2,
    "d" : 3,
    "e" : 4,
    "f" : 5,
    "g" : 6,
    "h" : 7
}

chess_map_from_index_to_alpha = {
    0: "a",
    1: "b",
    2: "c",
    3: "d",
    4: "e",
    5: "f",
    6: "g",
    7: "h"
}

def getBishopMoves(pos, chessBoard):
    column, row = list(pos.strip().lower())
    row = int(row) - 1
    column = chess_map_from_alpha_to_index[column]
    i,j = row, column
    solutionMoves = []

#Up-Right Diagonal
    try:
        temp = chessBoard[i + 1][j + 1]
        solutionMoves.append([i + 1, j + 1])
    except:
        pass
    try:
        temp = chessBoard[i + 2][j + 2]
        solutionMoves.append([i + 2, j + 2])
    except:
        pass
    try:
        temp = chessBoard[i + 3][j + 3]
        solutionMoves.append([i + 3, j + 3])
    except:
        pass
    try:
        temp = chessBoard[i + 4][j + 4]
        solutionMoves.append([i + 4, j + 4])
    except:
        pass
    try:
        temp = chessBoard[i + 5][j + 5]
        solutionMoves.append([i + 5, j + 5])
    except:
        pass
    try:
        temp = chessBoard[i + 6][j + 6]
        solutionMoves.append([i + 6, j + 6])
    except:
        pass
    try:
        temp = chessBoard[i + 7][j + 7]
        solutionMoves.append([i + 7, j + 7])
    except:
        pass
    try:
        temp = chessBoard[i + 7][j + 7]
        solutionMoves.append([i + 7, j + 7])
    except:
        pass

    temp = [i for i in solutionMoves if i[0] >=0 and i[1] >=0]
    solutionMoves = ["".join([chess_map_from_index_to_alpha[i[1]], str(i[0] + 1)]) for i in temp]
    solutionMoves.sort()
    return solutionMoves

Go through the PEP-8 style guide. You have inconsistent naming
  convention. A mixed case of camelCase and snake_case throws off
  the developer. Follow snake_case for variables and camelCase for
  class etc.

The first thing that comes to mind is a loop:

for pos in range(1, 8):
    try:
        temp = chessBoard[i + pos][j + pos]
        solutionMoves.append([i + pos, j + pos])
    except:
        break

which about covers the whole of your try-except blocks.

However, with chess; I'd suggest using coordinate system to move around the board.

Design a class Point which takes \$ (x, y) \$ position and define the __add__, __sub__, __neg__ etc. as follows (rough, modify as per your needs):

class Point(tuple):
    def __add__(self, other):
        return Point(v + w for v, w in zip(self, other))

    def __radd__(self, other):
        return Point(w + v for v, w in zip(self, other))

    def __sub__(self, other):
        return Point(v - w for v, w in zip(self, other))

    def __neg__(self):
        return -1 * self

    def __mul__(self, s):
        return Vector(v * s for v in self)

    def __rmul__(self, s):
        return Vector(v * s for v in self)

Now, define your movement direction as a point vector:

DIR_TOP_RIGHT = Point(1, 1)

when you want to move inside the board, just add a multiple of direction to the current point:

current = Point(i, j)
new = current + (distance * DIR_TOP_RIGHT)

Next, define a method (inside a class ChessBoard which checks whether a point lies inside the board or not. This should be quite easy. This ChessBoard class should also be the one responsible for converting your Point object from \$ (1, 1) \$ format to B1 etc.


Haskell sentence segregation I am trying to implement sentence segregation using Haskell, I have achieved a decent bulk of it using the NLP.FullStop library, but this doesn't seem to account for sentences with full stops at the end of quotes like this." or like this.', or at the end of bracketed sentences like this.) I also want to deal with the character ” much in the same way as ", as a lot of the content I am dealing with uses this character. I've been unable to get a successful regex match on this character, so have resorted to replacing it with " before the regex...

import qualified Data.ByteString.Char8 as BC
import           Data.List.Split
import qualified NLP.FullStop as FS

splitter :: String -> [String]
splitter = concatMap FS.segment . splitPunc
  where splitPunc = map unwords . split puncSplitter . words
        puncSplitter = keepDelimsR $ whenElt (\word -> BC.pack (splitPrep word) =~ puncExpr :: Bool)
        splitPrep = replace_ '”' '"'
        puncExpr = "\\.[)'\"][^w]?$" :: String

replace_ :: Eq b => b -> b -> [b] -> [b]
replace_ a b = map (\x -> if (a == x) then b else x)

While your code works and uses type signatures, it's missing documentation. It's not clear from your description or your code what splitter's intended result will be on a given input. Documentation and tests are therefore highly welcome.

Also, it's not clear why you've added an underscore to replace_. And your code is missing at least one include for =~. I assume that you just forgot to include that import line in your question and it is in your actual code.

That being said, the fullstop library is—according to its own documentation—a placeholder library:

  Note that this package is mostly a placeholder. I hope the Haskell/NLP
  communities will run with it and upload a more sophisticated (family
  of) segmenter(s) in its place. Patches (and new maintainers) would be
  greeted with delight!

Your quarrel about the line endings also comes from segment, since it hard-codes the allowed punctuations:

-- https://hackage.haskell.org/package/fullstop-0.1.4/docs/src/NLP-FullStop.html#stopPunctuation
stopPunctuation :: [Char]
stopPunctuation = [ '.', '?', '!' ] -- <<<<

Unfortunately, you cannot expand stopPunctuation, since content in parentheses (like this) does not lead to a new sentence. Note that .) and ." aren't valid in some languages, though, they require ). and "., so it's not clear what you try to achieve there (see comment above documentation above).

So all in all, well written, but without additional explanation or documentation there is no way to check whether the function actually does what you want. I also suggest you to add some tests.


Fast alternative for String.split by whitespace After profiling of one project I've found a bottleneck in the code, specifically the line:

String columns[] = line.split("\\s+");

I've tried to find a workaround, but all ready made solutions, such as StringTokenizer and Scanner seem to be less efficient (for example, look at the answer on SO). So I decided to write a custom function.

private List<String> fastSplitWS(final String text)
{
  final List<String> result = new ArrayList<String>();

  if(text != null)
  {
    final int n = text.length();
    if(n > 0)
    {
      int index1 = 0;
      char x = 0;
      boolean y = false; // "preceding char is separator" flag
      for(int i = 0; i < n; i++)
      {
        x = text.charAt(i);

        if(x == '\t' || x == ' ') // if((x >= 9 && x <= 13) || x == ' ')
        {
          if(!y)
          {
            String token = text.substring(index1, i);
            result.add(token);
          }
          index1 = i + 1;
          y = true;
        }
        else
        {
          y = false;
        }
      }

      if(index1 < n - 1)
      {
        result.add(text.substring(index1));
      }
    }
  }

  return result;
}

The problem is that the code performs even worse than String.split() although I don't see how to improve it. According to profiler, most problematic parts are:

self (lines of the plain code in this method) - 58%
String.charAt - 21%
String.subString - 12%
ArrayList.add - 7%

Could someone suggest how to optimize the self code?

Bug #1

You have an off-by-one error here:

  if(index1 < n - 1)
  {
    result.add(text.substring(index1));
  }

The consequence of this is that "a b c" will be split to ["a", "b"], poor "c" is left out.

Bug #2

I would expect the same output for these calls:

fastSplitWS("");
fastSplitWS("   ");

But that's not the case. The first returns an empty list, the second a list with a single empty string in it.
This behavior is also inconsistent with String.split.

This is caused by this:

if(!y)

One way to fix:

if(!y && i > 0)

Another way is to initialize y to true.
(Notice that with this variable name,
it's hard to understand the logical meaning of this.)

Readability

@TopinFrassi was too kind.
I find this code extremely hard to read,
mostly due to the poor naming throughout.
The unconventional indenting and brace placement don't help.

Performance

It's worth nothing the trade-offs of using .charAt and .toCharArray.
Take a look at those links, of the implementations in OpenJDK 8:

toCharArray does an arraycopy, using double the space
charAt does a boundary check on the index parameter

Which one is more efficient? I've no idea...

Improvement ideas

The indentation levels can be reduced here:

  if(text != null)
  {
    final int n = text.length();
    if(n > 0)
    {

The first one can be reduced using an early return:

if(text == null) return result;
final int n = text.length();
if(n > 0)
{

The second can be reduced by eliminating the if(n > 0) completely,
as the loop logic takes care of the n == 0 case automatically.

Suggested implementation

Applying the above suggestions and bugfixes, the implementation becomes:

private List<String> fastSplitWS(final String text) {
    if (text == null) {
        throw new IllegalArgumentException("the text to split should not be null");
    }

    final List<String> result = new ArrayList<String>();

    final int len = text.length();
    int tokenStart = 0;
    boolean prevCharIsSeparator = true;  // "preceding char is separator" flag

    char[] chars = text.toCharArray();

    for (int pos = 0; pos < len; ++pos) {
        char c = chars[pos];

        if (c == '\t' || c == ' ') {
            if (!prevCharIsSeparator) {
                result.add(text.substring(tokenStart, pos));
                prevCharIsSeparator = true;
            }
            tokenStart = pos + 1;
        } else {
            prevCharIsSeparator = false;
        }
    }

    if (tokenStart < len) {
        result.add(text.substring(tokenStart));
    }

    return result;
}


Decorator to convert a function's result into the desired units I tried to collect some of my formulas inside classes so that I didn't need to browser the names of the functions anymore but only determine which Quantity I want to calculate and then check which methods are avaiable for determining it. This is based on the astropy.units module. Just to make a small example:

import astropy.units as u

class Length(object):
    unit = u.m

    @classmethod
    #@quantitydecorator # I'll explain this later
    def from_velocity_time(cls, velocity, time):
        """
        Calculate length from a given velocity and time.

        Parameters
        ----------
        velocity: `astropy.units.Quantity`
            The velocity of the object.
        time: `astropy.units.Quantity`
            The time the object moved at this velocity

        Returns
        -------
        length: `astropy.units.Quantity`
            The determined length based on velocity * time
        """
        return velocity * time

This works great, I tried:

Length.from_velocity_time(100*u.km/u.h, 100*u.h)
# gives 10000 km

but has some downsides when the units are equivalent but different:

Length.from_velocity_time(100*u.km/u.s, 100*u.h)
# 10000 h*km/s

so I decided to create a general decorator that converts the results unit to the unit defined in the class or have an additional parameter result_unit for every method call:

def quantitydecorator(f):
    """
    Allows for an extra argument "result_unit" for all methods
    and converts the result to the unit specified there or if not
    given to the unit specified in the class.
    """
    @wraps(f)
    def wrapper(*args, **kwds):
        # Check if result_unit is given otherwise use the class default
        # (args[0] is the class)
        if 'result_unit' in kwds:
            result_unit = u.Unit(kwds.pop('result_unit'))
        else:
            result_unit = args[0].unit

        # Calculate the result
        result = f(*args, **kwds)

        # Convert to result unit if the result has a unit
        if not hasattr(result, 'unit'):
            # No unit attribute so we have a plain numpy array
            # or number, only let it pass when target unit is
            # dimensionless
            if args[0].unit != u.dimensionless_unscaled:
                raise ValueError('Got dimensionless quantity but needed'
                        ' quantity with unit: {0}'.format(args[0].unit))
        # Result has a different unit than wanted, convert it to wanted unit
        elif result.unit != result_unit:
            result = result.to(result_unit)
        return result
    return wrapper

This works fine (this time the decorator must be uncommented in the class):

Length.from_velocity_time(100*u.km/u.s, 100*u.h)
# 3.6e10m

Length.from_velocity_time(100*u.km/u.s, 100*u.h, result_unit=u.AU)
# 0.24064514AU

but I can't help suspecting that this is rather awkward. I'm using approximately 300 functions but there are only around 30-40 parameters (like Length in the example but more sophisticated like "free-fall-time", "half-light-radius". "redshift", etc. everything that I at some point needed for any calculation and there are typically 2-10 ways to calculate such a Quantity) so it would be a great gain to collect them into classes and don't have duplicate logic with the result_unit. Would you consider this "good coding" style or should I dismiss this attempt and choose a different approach (if so, any idea which)?

Your proposal…

Length.from_velocity_time(100*u.km/u.s, 100*u.h, result_unit=u.AU)

… is less conventional and not any better than

Length.from_velocity_time(100*u.km/u.s, 100*u.h).to(u.AU)

In addition, monkey-patching the from_velocity_time() function to support a special result_unit named parameter feels slimy.

The only drawback with .to(…) is that pure numbers wouldn't support such a method.  But that would mean that the calculation is complete nonsense anyway.


Multiline Input into Python Arrays I've knocked together a few lines of python to read in stats for a service (haproxy), and store them in an array (to do some analysis on later). It basically takes a multiline output, and splits it into multiple subarrays. This is how I've done it - can anyone offer improvements for me?

def build_array():
        services=[]
        for line in data.split('\n'):           # split out each line of raw input
                holding=[]                      # start a temp array
                for var in line.split(','):     # for each value append it to the temp array
                        holding.append(var)
                services.append(holding)        # append the temp array to the services array
        return services

The raw data is in the format:

data="""web,FRONTEND,,,0,0,4096,0,0,0,0,0,0,,,,,OPEN,,,,,,,,,1,2,0,,,,0,0,0,0,,,,0,0,0,0,0,0,,0,0,0,,,
    mysql,FRONTEND,,,0,0,4096,0,0,0,0,0,0,,,,,OPEN,,,,,,,,,1,3,0,,,,0,0,0,0,,,,,,,,,,,0,0,0,,,
    web-https,FRONTEND,,,0,0,4096,0,0,0,0,0,0,,,,,OPEN,,,,,,,,,1,4,0,,,,0,0,0,0,,,,,,,,,,,0,0,0,,,
    web,web2-NEW,0,0,0,0,,0,0,0,,0,,0,0,0,0,UP,1,0,1,0,0,19,0,,1,5,1,,0,,2,0,,0,L4OK,,0,0,0,0,0,0,0,0,,,,0,0,
    web,web1-OLD,0,0,0,0,,0,0,0,,0,,0,0,0,0,UP,1,1,0,0,0,19,0,,1,5,2,,0,,2,0,,0,L4OK,,0,0,0,0,0,0,0,0,,,,0,0,
    web,BACKEND,0,0,0,0,0,0,0,0,0,0,,0,0,0,0,UP,1,1,1,,0,19,0,,1,5,0,,0,,1,0,,0,,,,0,0,0,0,0,0,,,,,0,0,"""

This looks like CSV. You are probably better off using the python csv module.

            holding=[]                      # start a temp array
            for var in line.split(','):     # for each value append it to the temp array
                    holding.append(var)
            services.append(holding)        # append the temp array to the services arra

Can be written as

services.append( line.split(',') )

line.split() returns a list already, there is no need to copy the elements into another list.


Benchmark using floating point math I wrote some code in Python, to benchmark mathematical operations. It does not do anything in particular besides just calculating a lot of floating point numbers.

To clarify: I need 128-bits precision floating point numbers.

from __future__ import division
from numpy import arctan, sin, absolute, log10
from mpmath import *
import time
imax = 1000001
x = mpf(0)
y = mpf(0)
z = mpf(0)
t = mpf(0)
u = mpf(0)
i = 1
mp.prec = 128
start_time = time.time()
while i < 1000001:
    i += 1
    x = mpf(x + 1)
    y = mpf(y + x * x)
    z = mpf(z + sin(y))
    t = mpf(t + absolute(z))
    u = mpf(u + log10(t))

print("--- %s seconds ---" % (time.time() - start_time))
print x
print y
print z
print t
print u

This took approximately 87 secs to print results. How can I improve this code? My friend in fortran wrote similar program and it took only 3.14 secs for him to  print results in 128-bit precision.

Also in general I want to improve speed of my code. Instead 87 secs I want to reach times much closer to fortran.

Disclaimer: I maintain the gmpy2 library which also provides arbitrary-precision arithmetic.

gmpy2 includes fast integer arithmetic based on the GMP library. If gmpy2 is not installed, mpmath uses Python's native integer type(s) for computation. If gmpy2 is installed, mpmath uses the faster gmpy2.mpz integer type. You didn't mention if you have gmpy2 (or gmpy, an older version) installed, so I ran one test with your original code, mpmath, and without gmpy2. The running time was ~91 seconds. With gmpy2 installed, the running time was ~63 seconds.

The rest of the examples assume gmpy2 is installed.

Your code include superfluous calls to mpf(). Since x is already an mpf, the result of x+1 will also be an mpf so the mpf() call is not needed. If I remove those calls, the running time drops to ~56 seconds.

You import from both numpy and mpmath. mpmath.sin and mpmath.log10 replace the functions imported from numpy. But you are still using numpy.absolute. Numpy can be very fast but only when uses types it natively supports. If I remove numpy and change absolute to abs, the running time drops to ~48 seconds.

Here is the code with all the changes listed from above:

from __future__ import division
from mpmath import *
import time
imax = 1000001
x = mpf(0)
y = mpf(0)
z = mpf(0)
t = mpf(0)
u = mpf(0)
i = 1
mp.prec = 128
start_time = time.time()
while i < 1000001:
    i += 1
    x = x + 1
    y = y + x * x
    z = z + sin(y)
    t = t + abs(z)
    u = u + log10(t)

print("--- %s seconds ---" % (time.time() - start_time))
print x
print y
print z
print t
print u

I don't see any other significant improvements for an mpmath based approach.

gmpy2 also provides arbitrary-precision floating point based on the MPFR library. If I use gmpy2 instead, the running time is reduced to ~18 seconds.

Here is the code for a gmpy2 based solution:

from __future__ import division
import gmpy2
from gmpy2 import mpfr, sin, log10
import time
imax = 1000001
x = mpfr(0)
y = mpfr(0)
z = mpfr(0)
t = mpfr(0)
u = mpfr(0)
i = 1
gmpy2.get_context().precision = 128
start_time = time.time()
while i < 1000001:
    i += 1
    x = x + 1
    y = y + x * x
    z = z + sin(y)
    t = t + abs(z)
    u = u + log10(t)

print("--- %s seconds ---" % (time.time() - start_time))
print x
print y
print z
print t
print u


Time converter in Java Can this time converter be written more efficiently? I'm new to Java, and I would like to know.

main

import javax.swing.JFrame;

public class main {
        public static void main(String[] args) {
    window wn = new window();
        wn.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        wn.setSize(275, 170);
        wn.setVisible(true);
    }
}

time

public class time {
    private int hour;
    private int minute;
    private String ampm;

    public void setTime(int h, int m, String t) {
        hour = ((h>=0 && h<=24) ? h : 0);
        minute = ((m>=0 && m<=60) ? m : 0);
        ampm = t;
    }

    public String to24h() {
        if (ampm == "PM") {
            hour += 12;
        }
        return String.format("%02d:%02d", hour, minute);
    }
}

window

import java.awt.FlowLayout;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JTextField;
import java.awt.event.ActionListener;
import java.awt.event.ActionEvent;
import javax.swing.JButton;
import javax.swing.JComboBox;

@SuppressWarnings("serial")
public class window extends JFrame {

    private JLabel label;
    private JTextField hours;
    private JTextField minutes;
    private JTextField output;
    private JButton twelve_button;
    public time timeConvert = new time();
    int tHours;
    int tMinutes;
    private String[] times = {"AM", "PM"};
    private JComboBox combo;
    String tAMPM;

    public window() {
        super("Time Converter");
        setLayout(new FlowLayout());

        label = new JLabel("Time");
        add(label);

        hours = new JTextField(2);
        add(hours);

        label = new JLabel(":");
        add(label);

        minutes = new JTextField(2);
        add(minutes);

        combo = new JComboBox(times);
        add(combo);

        twelve_button = new JButton("Convert to 24 hours");
        add(twelve_button);

        output = new JTextField(10);
        output.setEditable(false);
        add(output);

        label = new JLabel("It's my 'hello world' of java development!");
        add(label);

        label = new JLabel("by jackwilsdon");
        add(label);

        theHandler handler = new theHandler();

        hours.addActionListener(handler);
        minutes.addActionListener(handler);
        twelve_button.addActionListener(handler);
    }

    private class theHandler implements ActionListener {
        public void actionPerformed(ActionEvent event) {
            if (event.getSource() == twelve_button) {
                if (hours.getText() != null && minutes.getText() != null) {
                    try {
                        tHours = Integer.parseInt(hours.getText());
                        tMinutes = Integer.parseInt(minutes.getText());
                        tAMPM = (String)combo.getSelectedItem();
                        timeConvert.setTime(tHours, tMinutes, tAMPM);
                        if (tHours > 12) {
                            output.setText("Please enter a 12 hour number!");
                        } else if (tMinutes > 60) {
                            output.setText("Please enter a 12 hour number!");
                        } else {
                            output.setText(timeConvert.to24h());
                        }
                    }
                    catch(NumberFormatException nFE) {
                        output.setText("Please enter a number!");
                    }
                }
            }
        }
    }
}

You asked about efficiency, which I am not an expert in. However, if you are going to share your code here, it would be nice if you could adapt it to the Java code style conventions.
In short: you should use PascalCase for your class names, so...

Main.java instead of main.java
Time.java instead of time.java
Window.java instead of window.java

Also, using single-letter names for your parameters is discouraged (borderline in the case of the fairly obvious int h and int m, bad in the case of String t). Why not hours, minutes, and halfOfDaySuffix? I'm sure you could find an even better term for the latter with a little research.

If you haven't already, make sure you organize your code into one or more package(s), e.g. put package com.jackwilsdon.TimeConversion; at the beginning of your files. This helps avoid name clashing problems (for instance with java.sql.Time).

In regard to to24h(): Never compare two Strings with the == (reference equals) operator in Java. This will only work in a narrow range of scenarios, read the full explanation on StackOverflow. You need to use the String.equals method instead.

But while we are at it, you shouldn't be using a String to represent AM/PM anyway - it will stop working if you pass in the wrong string, for instance pm instead of PM by mistake. Instead, restrict the possible values by creating an Enum:

public enum DayHalf {
    AM,
    PM
}

Then (in Window), you can change tAMPM's type from String to DayHalf - the following line will stop working:

tAMPM = (String)combo.getSelectedItem();

So you need to use combo = new JComboBox(DayHalf.values()); instead of combo = new JComboBox(times); and change the above line to

tAMPM = (DayHalf)combo.getSelectedItem();

If you are passing in AM or PM, hours above 12 are not acceptable - neither are minutes above 59! Hence, your conditions are erroneous; also, silently failing by setting the fields to 0 isn't a robust solution. You should throw an IllegalArgumentException if the values are out of range.

Your updated setTime method would look something like this:

public void setTime(int hours, int minutes, DayHalf halfOfDaySuffix) {
    if (hours < 0 || hours > 12 || minutes < 0 || minutes > 59)
        throw new IllegalArgumentException("hours or minutes out of allowed range!");
    hour = hours;
    minute = minutes;
    dayHalf = halfOfDaySuffix;
}

Regarding your JFrame (Window.java, should probably be called TimeConversionWindow.java), you should adjust the obviously copied-and-pasted error message:

else if (tMinutes > 60) {
    output.setText("Please enter a 12 hour number!");
}

to "Please enter less than 60 minutes!" or similar. Note that your condition (tMinutes > 60) is incorrect once again and should be changed to > 59.

A couple of other adjustments will be required, but they are easy to figure out. I'd argue that performance is nearly irrelevant in this scenario, read up on bottlenecks.


Converting from MySQLi to PDO account activation I had been working on a project for some time, and then it went on the way back burner after my daughter was born.  I'm back to it, and now I discover that I'm best off using PDO over MySQLi.  So, I'm working on the conversion, and I'd like to get some confirmation that I'm handling things well.  Essentially: Does this code look secure and sound?  [It's just my account activation php.]

<?php
require_once ('common.php');

$x = $y = FALSE;
if (isset($_GET['x']) && filter_var($_GET['x'], FILTER_VALIDATE_EMAIL)) {
    $x = $_GET['x'];
}
if (isset($_GET['y']) && (strlen($_GET['y']) == 32 )) {
    $y = $_GET['y'];
}

if ($x && $y) {
    $query = "SELECT 1 FROM users WHERE (email=:email AND confirm IS NULL)";
    $query_params = array( ':email' => $x );
    try {
        $stmt = $db->prepare($query);
        $result = $stmt->execute($query_params);
    }
    catch(PDOException $ex) {
        die("Failed to run query: " . $ex->getMessage());
    }

    $count = $stmt->rowCount();
        if($count == 1) {
            echo "
Your account is already activated. No need to activate again. You may now log in.
";
            exit();
        } else { // Not an active user
            $query = "UPDATE users SET confirm=NULL WHERE (email=:email AND confirm=:confirm) LIMIT 1";
            $query_params = array(
                ':email' => $x,
                ':confirm' => $y
            );

            try {
                $stmt = $db->prepare($query);
                $result = $stmt->execute($query_params);
            }
            catch(PDOException $ex) {
                die("Failed to run query: " . $ex->getMessage());
            }

            $count = $stmt->rowCount();
                if($count == 1) {
                    echo "
Your account is now active. You may now log in.
";
                    exit();
                } else {
                    echo '
Your account could not be activated. Please re-check the link or contact the system administrator.
';
                    exit();
                }
        }

} else {
    $url = BASE_URL . 'index.html';
    ob_end_clean();
    header("Location: $url");
    exit();
}

?>

I am aware that the $ex->getMessage() needs to go before production.

The Good

You use bound parameters. You handle errors.

The Bad

You filter the email address but then pass the unfiltered value to the select and update queries. What are you trying to achieve with the check of the filtered email?

You are not checking the validity of the 'y' param (which I assume is an activation token). Isn't the point of this kind of activation to ensure that the real person at that email is doing the activation? The way you have it, an attacker could register an account using any email address. Here is how:

1) Attacker goes to your registration page and enters someone else's email address (i.e. notme@example.com)

2) Attacker crafts and visits the url: yoursite.com/activation.php?x=notme@example.com&y=12345678901234567890123456789012 (where the y value is some random string, 32 chars in length)

Attacker now has an activated account using an email they do not own, having never received the activation email.

The Ugly

Your logic and presentation are mixed together. Perhaps for this relatively simple script, it's not that big a deal. But, over time, these things have a habit of growing and getting very messy. Look into mvc.

One try catch block should be all you need.

Example Rewrite

try {
    //initialize and filter $x and $y
    if (!$x || !$y) {
        throw new Exception("$x or $y invalid");
    }

    //run select query. It's where clause should be something like:
    $where = 'email = :email AND '
           . 'confirm = :token AND '
           . 'confirmExpiration <= :expiration AND '
           . 'activated = false';
    $params = array(
        ':email' => $x,
        ':token' => $y,
        ':expiration' => date('Ymd His'),
    );

    //check results of select query
    if (/* select was not successful */) {
        throw new Exception("Invalid request. x: $x y: $y");
    }

    //Update the account so that it is now active

    //redirect to a success page
} catch (Exception $e) {
    //if production, log it and redirect to a friendly error page
    //if development, redirect to some debug page
}


Average readouts from multiple IR sensors with Arduino I've written an Arduino sketch to continuously read values from multiple analog IR distance sensors, average the last 7 values and output in millimeters. Being pretty new to both Arduino and embedded programming, I'm sure there's a lot of room for improvement. Any critique would be highly appreciated, including idioms and built-in functions I could have used.

My plan is to extend this program to run LED strips based on the sensor values.

#define LOOP_PERIOD_MS 10 // run loop every 10 ms
#define IR_WIN_LENGTH 7 // IR sensor moving-average window
#define IR_NUM_SENSORS 4 // Number of IR sensors, must be 1-6 and connected to the first analog pins

// Declare IR sensor functions
void ir_read_new_vals();
uint16_t ir_get_cur_mm( uint8_t sensor_i );
uint16_t ir_get_avg_mm( uint8_t sensor_i );

// Variables
unsigned long loop_start_ms = 0; // When the last loop iteration started

// Setup
void setup() {
  // Initialize serial communication
  Serial.begin(115200);
}

// Main loop: Read and print IR sensor data continuously
void loop() {
  // Limit loop frequency
  while ( millis() - loop_start_ms < LOOP_PERIOD_MS )
    delayMicroseconds(10);
  loop_start_ms = millis();
  // Print timestamp
  Serial.print( loop_start_ms );
  // Read and print IR sensor values
  ir_read_new_vals();
  for ( uint8_t sensor_i = 0; sensor_i < IR_NUM_SENSORS; sensor_i++ ) {
    Serial.print(",");
    Serial.print(ir_get_avg_mm(sensor_i));
  }
  // print new line
  Serial.println("");
}

// IR sensor functions

uint8_t ir_i = IR_WIN_LENGTH; // array index for the last read value
uint16_t ir_vals_mm[IR_NUM_SENSORS][IR_WIN_LENGTH]; // "rolling" stored values for sensors
bool ir_avg_is_valid = false; // whether moving-average window is filled

// Read, scale and store values from analog sensors
// The four sensors are connected to analog pins 0, 1, 2, 3
void ir_read_new_vals() {

  // Increment "rolling array" index
  ir_i = ( ir_i + 1 ) % IR_WIN_LENGTH;

  // Read each sensor
  for ( uint8_t sensor_i = 0; sensor_i < IR_NUM_SENSORS; sensor_i++ ) {
    // Dummy sensor read (suggested when switching ADC pin)
    analogRead(sensor_i);
    // Read sensor value (values 0..1023 corresponds to 0..5V)
    uint32_t raw = analogRead(sensor_i);
    // Clamp values to 5-80cm
    if ( raw > 600 )
      raw = 600;
    if ( raw < 80 )
      raw = 80;
    // Rescale value to mm according to datasheet curve and store
    ir_vals_mm[sensor_i][ir_i] = (uint16_t)( 67870 / ( raw - 3 ) - 40 );
  }

  // Check if the moving-average window was filled
  if ( ! ir_avg_is_valid && ir_i == IR_WIN_LENGTH - 1 )
    ir_avg_is_valid = true;
}

// Return last read sensor value in mm
uint16_t ir_get_cur_mm( uint8_t sensor_i ) {
  return ir_vals_mm[sensor_i][ir_i];
}

// Return moving average of sensor values
uint16_t ir_get_avg_mm( uint8_t sensor_i ) {
  // Return 0 if not enough values
  if ( ! ir_avg_is_valid )
    return 0;
  // Sum sensor values
  uint32_t accum_mm = 0;
  for ( uint8_t i = 0; i < IR_WIN_LENGTH; i++ )
    accum_mm += ir_vals_mm[sensor_i][i];
  // Divide and return
  return accum_mm / IR_WIN_LENGTH;
}

The code feels overcommented. The only comment I see having some value is

// Dummy sensor read (suggested when switching ADC pin)

but even that shall refer to the proper place in the datasheet.

Everything else could (and should) be taken care of by using proper names. Don't be shy factoring blocks of code into functions, even just for the sake of giving them names.
The code does not know (or care about) the nature of the sensor connected to the pins. Does it matter that it is IR? Would anything be changed if it were RF, or sonar, or anything else? So the ir_ prefix seems like a pure noise. I strongly advise of dropping it.
The code does care on the signal postprocessing (clamping and rescaling). This operation is highly specific on the part used. If by any chance you'd opt for another part, you'd need to fix the heart of the program. I strongly advise to factor this functionality into the function, to be called via a pointer.
I do not endorse variables like ir_avg_is_valid. It is toggled once, but consumes cycles for the entire program lifetime. Consider filling up first IR_WIN_LENGTH samples outside the loop.

All that said, the code should look along the lines of

    void read_new_values(uint32_t (*postproc)(uint32_t raw))
    {
        for (uint8_t sensor_i = 0; sensor_i < IR_NUM_SENSORS; sensor_i++ ) {
            uint32_t raw = read_sensor(sensor_i);
            uint32_t cooked = postproc(raw);
            values[sensor_i][win_ix] = cooked;
            win_ix = (win_ix + 1) % WIN_LENGTH;
       }
    }

    uint32_t read_sensor(uint8_t pin) {
        analogRead(pin); // Dummy read, see datasheet, chapter and verse
        return analogRead(pin);
    }

    uint32_t whatever_part_number_postproc(uint32_t value) {
        // See datasheet, chapter and verse
        clamp
        return rescale
    }

    void prefill_window(uint8_t win_size) {
        for (win_ix = 0; win_ix < win_size; win_ix++) {
            read_new_values(whatever_part_number_postproc);
        }
    }

    void setup() {
        ....
        prefill_window(WIN_LENGTH);
    }

    void loop() {
        ....
        read_new_values(whatever_part_number_postproc);
        ....
    }


Generate 10 random 3-letter strings This is a toy project and I am a beginner in Python.

I need to create 10 random 3-letter strings, where characters are a - z.

This is my implementation:

import random

def random_char():
    return random.randint(97, 122)

three_letter_words = []
for i in range(0, 10):
    three_letter_words.append(chr(random_char()) + chr(random_char()) + chr(random_char()))

I really do not like the part where I call chr(random_char()) 3 times.

How can I improve this to make it more compact and cleaner?

Python 3.6 introduced random.choices, which allows you to write this even more succinctly than using random.choice:

from random import choices
from string import ascii_lowercase

three_letter_words = ["".join(choices(ascii_lowercase, k=3)) for _ in range(10)]


refactor helper for default state for rails select with enums I've created a helper method that is essentially used to get the default state of an select:

<%= select_tag "question_main_frequency",
          options_for_frequency_state(frequency_segment(question)), { class: 'form-control' } %>

The helper method I've created to get the default state looks like this:

  def frequency_segment(question)
    return false if question.id.nil?
    question = Question.find(question.id)
    if question.weekly?
      'weekly'
    elsif question.odd_weeks? || question.even_weeks?
      'Biweekly'
    elsif question.start_of_month? || question.end_of_month?
      'Monthly'
    elsif question.start_of_quarter? || question.end_of_quarter?
      'Quarterly'
    end
  end

The reason why I need this is because that form populates a new select form with the methods you see in the helper. So if they select 'biweekly' then question.odd_weeks and question.even_weeksappear as two options.

and you don't have to be a genius to see that this isn't exactly beautiful. It's also causing:

Perceived complexity for frequency_segment is too high. [10/7]
Method has too many lines. [11/10]
Cyclomatic complexity for frequency_segment is too high.[9/6]

on rubocop. The thing is I'm not entirely sure how to refactor this. The Question methods are enum, set up like this:

class Question < ActiveRecord::Base
  enum frequency: { weekly: 0, odd_weeks: 1, even_weeks: 2,
                    start_of_month: 3, end_of_month: 4,
                    start_of_quarter: 5, end_of_quarter: 6 }

any ideas on how I can refactor this?

Don't know if there's a good way to do this, since your translation isn't 1:1. E.g. your enum differentiates between odd weeks and even weeks, but your helper doesn't.

So you have to do some sort of work to collapse 7 states into 4 options.

If you're already using I18n to do translation, you might be able to put the strings in there, e.g.:

question:
  frequency:
    - weekly
    - biweekly
    - biweekly
    - monthly
    - monthly
    - quarterly
    - quarterly

and the use the standard Rails I18n helpers to fetch the right string. But that's not too pretty either, and really only hides the problem from Rubocop. It's also a more implicit connection, and more error-prone, so, yeah, you probably shouldn't go down this road.

Frankly, I wouldn't bother too much about Rubocop's opinion about method length or cyclomatic complexity in this case. But...

But there's weird stuff going on too:

You're passing in a question and checking its id. But it's not a real Question record? Because you immediately go and fetch one from the database. So what are you passing in that's called question, has an id, but isn't actually a Question?
You're also overwriting the argument variable with the record you fetch, which is not a great idea. Now you have a question that's apparently not really a Question, but then maybe it becomes a question that is actually a Question. Confused? Me too.
Oh, and are you sure you're going to find something when you call find(id)? If not, you'll get a 404 RecordNotFound error. It might be completely intentional to let that happen, but to have the page break due to a helper method is perhaps a little icky.
Why return false? If anything, you should return nil. Ideally, you return a string, but the "opposite" of a string isn't false - at best it's nil. And actually, your method could conceivably return nil already, since you don't have an else branch in your logic. If, for whatever reason, none of the if/elsif conditions are true, your method will return nil.

Anyway, without worrying about the question vs question thing and possible exceptions being thrown, I'd suggest at least using a case statement instead of a lot of elsifs:

case
when question.weekly?
  'Weekly'
when question.odd_weeks?, question.even_weeks?
  'Biweekly'
when question.start_of_month?, question.end_of_month?
  'Monthly'
when question.start_of_quarter?, question.end_of_quarter?
  'Quarterly'
else
  nil
end

Oh, and I capitalized "Weekly" to match the other strings.