Extracting text fields from  tags in an HTML message What i'm doing

I have a string with html information like this:

 Some text this is a test

My goal in the method is to create a dictionary with this value:

**key**     **value**
field-4    Some text

This is the code that i'm using to accomplish my task:

public static Dictionary<int,String> getFields(String mensaje)
    {
        Dictionary<int,String> fields = new Dictionary<int,string>();
        Match m = Regex.Match(mensaje, @"^(.*?(.*?).*?)+$", RegexOptions.Singleline);
        for (int i = 0; i < m.Groups[2].Captures.Count; i++)
        {
            String value = m.Groups[1].Captures[i].Value;
            Match m2 = Regex.Match(value, "^(.*?fieldId=.*?\"(.*?)\">.*?)+$", RegexOptions.Singleline);
            String fieldId = m2.Groups[2].Captures[0].Value;
            fieldId = fieldId.Replace("field-", String.Empty);
            fields.Add(int.Parse(fieldId),m.Groups[2].Captures[i].Value);
        }

        return fields;
    }

How can i improve my code?

I know this is Code Review not Rewrite My Code, however I would suggest using a third-party Html parser (like the Html Agility Pack for example) over regular expressions if that's an option.

I realize you're doing very trivial parsing here, but from my personal experiences regular expressions grow to unmaintainable status quicker than anything in software development.

If you were to use a Html parser, you could do something like this:

string htmlToParse = "
Some text this is a test

Some more text this is another test
";
const string ElementToParse = "span";
const string IdField = "FieldId";

HtmlDocument htmlDocument = new HtmlDocument();
htmlDocument.LoadHtml(htmlToParse);

int fieldId = default( int );

Dictionary<int,string> fieldValuesTable =
(
    from
        htmlNode in htmlDocument.DocumentNode.DescendantNodes()
    where
        htmlNode.Name.Equals( ElementToParse, StringComparison.InvariantCultureIgnoreCase )
        &&
        htmlNode.Attributes.Contains( IdField )
    let
        id = htmlNode.Attributes[ IdField ].Value
    where
        Int32.TryParse( id.Substring( id.IndexOf( "-" ) + 1 ), out fieldId ) // this is stil not ideal,
    select
        new { Id = fieldId, Text = htmlNode.InnerText }
).ToDictionary( f => f.Id, f => f.Text );

You get the output:

4 : Some text
5 : Some more text

IMHO, it's much cleaner and maintainable.


Pascal Triangle implementation in C# This is my attempt to implement the Pascal Triangle in C#. It is meant to calculate and return a pascal triangle of size n (which it takes is through the parameter rows).

static List<List<int>> GetPascalTriangle(int rows)
{
    List<List<int>> result = new List<List<int>>();
    for (int x = 0; x <= rows - 1; x++)
    {
        if (x == 0 || x == 1)
        {
            result.Add(Enumerable.Repeat(1, x + 1).ToList());
        }
        else
        {
            List<int> prevRow = result[x - 1];
            List<int> semiResult = new List<int>();
            semiResult.Add(1);
            for (int x1 = 0; x1 <= prevRow.Count - 2; x1++)
            {
                semiResult.Add(prevRow[x1] + prevRow[x1 + 1]);
            }
            semiResult.Add(1);
            result.Add(semiResult);
        }
    }
    return result;
}

I feel this code is a little messy. Thus I seek some opinion on it and ways to improve it.

First: The Pascal Triangle will grow very rapidly. And rows=100, your maximum value is 1.27388061294904e+28. Yes, that's right it will overflow int, uint, long, and ulong within 100 rows. Check it out on interactive pascal triangle site
The only suggestion is Numerics.BigInteger where there is no upperbound.

Second: Why are you using List<List<int>>? You can use a jagged array, which is much shorter: int[][], and effectively does the same. You just need to initialize them ahead with their size

var triangle = new int[rows][];
for(int i = 0; i<rows;i++){triangle[i] = new int[i+1];}

Third: if you know the first and last element is always a 1, then the if-statement for the first 2 rows is redundant

// This part is redundant, you already check if the first/last element is 1
if (x == 0 || x == 1)
{
    result.Add(Enumerable.Repeat(1, x + 1).ToList());
}

So with all the improvements, you code would be shorter and could handle much more rows. As a bonus I removed the dependency on system.linq. It could look like this:

public static BigInteger[][] GetPascalTriangleImproved(int rows)
    {
        BigInteger[][] result = new BigInteger[rows+1][];
        for (int x = 0; x < rows; x++)
        {
            result[x] = new BigInteger[x + 1];
            result[x][0] = 1; // first element is ALWAYS 1

            for (int x1 = 1; x1 <= x; x1++)
            {
                // last element is always 1 (just like the first)
                if (x1 == x) { result[x][x] = 1; continue; }

                // in all other cases, just add the 2 digits in the upper row
                result[x][x1] = result[x - 1][x1 - 1] + result[x - 1][x1];
            }
        }
        return result;
    }


How I can make file multiple of `segmentSize` I multiply the original file and cut a piece of final file with intention to do it a multiple of segmentSize times with minimum loss of data.

Example: I have a file size of 4 bits, and segment size of 3 bits;
I do extend my original in 3 times or 6 and so far and so forth.

or another example:
I have file size of 48 000 000 and segment (1024*1024*6*2)
and if I multiply the original file by 5 (make file that was contain 5 original files in row) (48 000 000 * 5)/(1024*1024*6*2) = 19.0734...1 -- I will have only 0.7% loss.
And then I make my extended file (five times of original) by copying original file into Temp.iqbin 5 times, and after read the segment (1024*1024*6*2) from Temp.iqbin and write it into my final.iqbin (I do this 19 times in the last cycle) and I get Final file that is multiple of segmentSize (because I trimmed some data by not copying it from Temp.iqbin size).

    double criteria = 0;
    int arraySize = 0;

    double box = 1;
    int box_i = 0;
    double coeff;
    int sizeFactor;
    if (size < 1300000)
        sizeFactor = 8;
    else
        sizeFactor = 2;

    //if original file too big, reduce the extend coeff
    if (size < segmentSize) {
        coeff = segmentSize / size;
        coeff = (int)coeff;
    }
    else
        coeff = 0;

    //optimization by criteria of minimum data loss in final file
    for (int i = 1; i < 4*sizeFactor; i++) {
        criteria = (size*(i+coeff)) / segmentSize;
        criteria = criteria - int(criteria);
        box = box < criteria ? box : criteria;
        box_i = box < criteria ? box_i : i+coeff;
    }

    std::vector<char> vector(segmentSize);

    //Remove old Temp file
    std::remove("Temp.iqbin");
    std::fstream ofs1("Temp.iqbin", std::ios::binary | std::ios::out | std::ios::app | std::ios::in);
    fs.seekg(0, std::ios::beg);

    //i do the file that is multiple of original file in box_i (the size
    //which guarantee minimum data loss after trim) times
    for (int i = 0; i < box_i; i++) {
        ofs1 << fs.rdbuf();
        fs.seekg(0, std::ios::beg);
    }

    //back to beginning of file
    ofs1.seekg(0, std::ios::beg);
    //remove old final.iqbin
    std::remove("final.iqbin");
    std::ofstream ofs("final.iqbin", std::ios::binary | std::ios::out |std::ios::app);
    //Write Segments one by one to final.iqbin file
    //i read segments(have size i want new file was multiple of) and write
    //it to the newfile
    coeff = ((size*box_i) / segmentSize);
    for (int i = 0; i < (int)coeff; i++) {
        //Read Segment from Temp file
        ofs1.read(&vector[0], segmentSize);
        //Write Segment to final.iqbin file
        ofs.write(&vector[0], segmentSize);
    }
    ofs1.close();
    //remove Temp file
    std::remove("Temp.iqbin");

This code is working and it even works fast for files that I feed to it, but I think it's not the optimal solution.

As far as I can understand (or better say: guess) what you mean, the optimum solution is a file size equal to the least common multiple of the given data size and the segment size: multiply your file N times:

\$ N = {segmentSize} / \operatorname{gcd}({dataSize}, \ {segmentSize}) \$

so you get a file of size

\$S = \operatorname{lcm}({dataSize}, \ {segmentSize})\$

which is \$M\$ full segments:

\$M = {dataSize} / \operatorname{gcd}({dataSize}, \ {segmentSize}) \$

For data size 48 million and segment size 12 Meg the greatest common divisor is 3,072 = 3*1024, so their least common multiple is:
\$196,608,000,000 = 2^{12} \times datasize = 5^6 \times segmentsize.\$


A global Enum for comparison operators This approach was born out of wanting to pass a logical expression (E.G. ">=10000") as a parameter to a function.

So, I built an Enum and a function to use it to evaluate logical expressions.

Is this a good, maintainable, scalable approach?

Public Enum comparisonOperator
    '/ Used [[ ]] because the outer set escape the inner brackets for the Enum list, which will then escape the operator when chosen as a value. E.G. they will appear as "Operator = [<]" in code.
    [[<]] = 1
    [[<=]] = 2
    [[=]] = 3
    [[>=]] = 4
    [[>]] = 5
End Enum

Public Function ComparisonIsTrue(ByVal sourceValue As Variant, ByVal operator As comparisonOperator, ByVal comparisonValue As Variant) As Boolean

    Dim isTrue As Boolean

    Select Case operator

        Case Is = 1 '/ <
        isTrue = (sourceValue < comparisonValue)

        Case Is = 2 '/ <=
        isTrue = (sourceValue <= comparisonValue)

        Case Is = 3 '/ =
        isTrue = (sourceValue = comparisonValue)

        Case Is = 4 '/ >=
        isTrue = (sourceValue >= comparisonValue)

        Case Is = 5 '/ >
        isTrue = (sourceValue > comparisonValue)

        Case Else
        '/ Error Handling
        Stop

    End Select

    ComparisonIsTrue = isTrue

End Function

Public Enum comparisonOperator
    '/ Used [[ ]] because the outer set escape the inner brackets for the Enum list, which will then escape the operator when chosen as a value. E.G. they will appear as "Operator = [<]" in code.
    [[<]] = 1
    [[<=]] = 2
    [[=]] = 3
    [[>=]] = 4
    [[>]] = 5
End Enum

Why not just name them and comply with the language's rules for user identifiers? Also, Enum constants should be enumerated: let the compiler assign the underlying values:

Public Enum ComparisonOperator
    EqualTo = 0
    LessThan
    LessThanOrEqualTo
    GreaterThanOrEqualTo
    GreaterThan
End Enum

There should be a sensible default (0) value; I think I'd make EqualTo that default.

The operators in Case Is = ... are redundant, but the magic numbers in the Case expressions should be the enum named constants. The underlying values should be abstracted away behind the enum - the underlying values should be meaningless.

Consider:

    Case ComparisonOperator.LessThan
    isTrue = sourceValue < comparisonValue

    Case ComparisonOperator.LessThanOrEqualTo
    isTrue = sourceValue <= comparisonValue

    Case ComparisonOperator.EqualTo
    isTrue = sourceValue = comparisonValue

    Case ComparisonOperator.GreaterThanOrEqualTo
    isTrue = sourceValue >= comparisonValue

    Case ComparisonOperator.GreaterThan
    isTrue = sourceValue > comparisonValue

That way nothing breaks if the enum members are reordered.


Should I add the "fetch_group" method to my user class or should I create a separate class for group I have a table of users, where each user has a group (or multiple groups) he/she is part of.

Currently I'm handling the users group via a Users class:

<?php
/**
 *
 *  User (class) Model: Handles users (not system users).
 *
 */
class User{

/*=================================
=            Variables            =
=================================*/

    # Database instance
    private $db;

    # Users ID
    public $user_id;

    # User Realm
    public $realm;

    # User Name
    public $user_name;

    # User display name
    public $display_name;

    # User creation
    public $created;

    # User last seen
    public $last_seen;

    # Is user deleted?
    public $deleted;

    # Is user ignored?
    public $ignored;

    # Is user blacklisted
    public $blacklisted;

    # Is user whitelisted
    public $whitelisted;

    # Is user blocked
    public $blocked;

    # Is user filtered
    public $filtered;

    # User Group
    public $group = array();

/*===============================
=            Methods            =
================================*/

    /**
     *
     *  Constructor
     *  @param      $user_id    Init        User id.
     *  @throws                 Object      User object of a single user.
     *
     */
    public function __construct($user_id) {

        # Get database instance
        $this->db           = Database::getInstance();

        # If user_id isn't passed
        if ( $user_id ) {

            # Get this user by id
            $this->get_user_by_id($user_id);
        }
    }

    /**
     *
     *  Get user by id
     *  @param      $id     Init        User id
     *  @throws             Object      Returns the object with data applied.
     *
     */
    private function get_user_by_id($id)
    {
        if ($id) {

            # Search for the user in the Database 'users' table.
            $data   = $this->db->row("SELECT user_id, realm, user_name, display_name, created, last_seen, deleted, ignored, blacklisted, whitelisted, blocked, filtered FROM users WHERE user_id = :id", array('id' => $id));

            # If there is a result
            if ( $data ) {

                # Get group/s
                // -- Get Group method  --

                # Set data in this user object
                $this->data($data);

                return $this;

            } else { return false; }

        } else {
            return false;
        }
    }

    /**
     *
     *  Insert Data to this object
     *  @param      $data       Array       Gets a result array of user data
     *
     */
    private function data($data)
    {
        # Set data for this user object
        $this->user_id      = $data['user_id'];
        $this->realm        = $data['realm'];
        $this->user_name    = $data['user_name'];
        $this->display_name = $data['display_name'];
        $this->created      = $data['created'];
        $this->last_login   = $data['last_seen'];
        $this->deleted      = $data['deleted'];
        $this->ignored      = $data['ignored'];
        $this->blacklisted  = $data['blacklisted'];
        $this->whitelisted  = $data['whitelisted'];
        $this->blocked      = $data['blocked'];
        $this->filtered     = $data['filtered'];
        // $this->group         = $data['group'];
    }

I have just added the group variable, and not sure if it's "good practice" to add a fetch_group_by_user_id() method (separate table in my DB), or create a new group object, and use it inside my User class.

My user class is pasted ^

Please review my User class code and update me if you think it can be written better, and please tell me what is best to fetch the group.

Logical inconsistencies

You create a User object expecting a user_id to be passed to setup the object, id object to this design but im sure you've got it covered, but then later on you ask for a user_id to run a query?

I would leave it in the constructor and use type hiniting (php 7.0 +) to make sure it passed on creation

public function __construct(int $userId) {
    $this->userId = $userId;
    $this->get_user_by_id();
}

But the type hint should make sure an exception is thrown if it isn't provided correctly!

PSR2

The PSR2 is a great coding style guide for php it says you should use camelCase for variable names & function names you should look into it.

Returning early

You should return early where you can this makes your code easier to read and less indentation levels

private function get_user_by_id($id)
{
    if ($id) {
        return false;
    }

    $data   = $this->db->row("SELECT user_id, realm, user_name, display_name, created, last_seen, deleted, ignored, blacklisted, whitelisted, blocked, filtered FROM users WHERE user_id = :id", array('id' => $id));

    if ( !$data ) {
        return false;
    }

    $this->data($data);

    return $this;
}

Adding more functions to setup a user

I would create a Service class for your user as your current class is doing both Model work (connecting to the database) and being a "Object".

<?php

namespace SomeNamespace\Services;

use SomeNamespace\Models\Users\GetUserDetails;
use SomeNamespace\Models\Users\GetUserGroupDetails;

class User
{
    // Properties list

    public function __construct(
        GetUserDetails $getUserDetails,
        GetUserGroupDetails $getUserGroupDetails
    ) {
        $this->getUserDetails = $getUserDetails;
        $this->getUserGroupDetails = $getUserGroupDetails;
    }

    public function populateData(int $userId)
    {
        $userDetails = $this->getUserDetails->get($userId);

        if(empty($userDetails)){
            throw new \Exception("Missing user for id $userId", 1);
        }

        $groupDetails = $this->getUserGroupDetails->get($userId);

        $this->setupUserDetails($userDetails);
        $this->setupUserGroupDetails($groupDetails);

        return $this;
    }

    private function setupUserDetails($userDetails)
    {
        $this->someUserDetails = $userDetails["something"];
    }

    private function setupUserGroupDetails($groupDetails)
    {
        $this->someGroupDetail = $groupDetails["something"];
    }
}

// GetUserDetails.php

namespace SomeNamespace\Models\Users;

class GetUserDetails
{
    public function __construct()
    {
        $this->db = some::db::factory();
    }

    // Lots of sql queries
}

// GetGroupDetails.php

namespace SomeNamespace\Models\Users\Groups;

class GetUserGroupDetails
{
    public function __construct()
    {
        $this->db = some::db::factory();
    }

    // Lots of sql queries
}

I may be wrong as you seem to be using factories to construct objects instead of dependency injection, but im sure you see where im going with it!


jQuery looping on fields after validation I have the following code:

$(":input, textarea, select").on('blur', function(ev) {

var data = $('#campaign-form').serialize();

$.ajax({
    url: "/campaign/validate",
    type: "POST",
    data: data,
    cache: false,
    dataType: 'json',
    success: function (data) {
        if (data) {

            /*Assign error classes*/
            $.each(data, function (field, message) {
                jQuery("#" + field).addClass('input-error');
                jQuery("#" + field + "-error").text(message);
            })
        }
    }
});

This code adds a class 'input-error' to fields which have an error (validated on the PHP side).

The issue is that if I fix the error, and send validation again, the input-error class is not removed. Also I need to add class input-valid to the correct fields.

I have tried a solution, but it does not look optimal (it does make the work though).

$(":input, textarea, select").on('blur', function(ev) {

var data = $('#campaign-form').serialize();

$.ajax({
    url: "/campaign/validate",
    type: "POST",
    data: data,
    cache: false,
    dataType: 'json',
    success: function (data) {
        if(data) {

            /*Remove classes for all fields in the form*/
            clearFields = $("#campaign-form :input.gg-field, #campaign-form textarea.gg-textarea, #campaign-form select.gfp-field").each(function(index) {
                var input = $(this);
                var fieldId = input.attr('id');
                jQuery("#"+fieldId).removeClass('input-valid');
                jQuery("#"+fieldId).removeClass('input-error');
                jQuery("#"+fieldId+"-error").text("");

            });

            /*Assign error classes*/
            $.each(data, function(field, message) {
                jQuery("#"+field).addClass('input-error');
                jQuery("#"+field+"-error").text(message);
            })

            /*To the other fields assign valid classes*/
            validFields = $("#campaign-form :input.gg-field, #campaign-form textarea.gg-textarea, #campaign-form select.gfp-field").not( ".input-error" ).each(
                function(index) {
                    var input = $(this);
                    var fieldId = input.attr('id');
                    jQuery("#"+fieldId).addClass('input-valid');
                    jQuery("#"+fieldId+"-error").text("");
                });

        }
    }
});

I would like to verify which fields are coming on the data JSON object in order to assign the error or valid class in that process, avoiding the need to loop through the fields again.

Data object example:

{ "title":"* The Campaign Title field is required.",
  "description":"* The Description field is required.",
  "campaign_category_id":"* The Campaign Category field is required."
}

Why not just iterate over the inputs, and if the values of the id attribute exists in data then add the error class and show the affiliated error message, otherwise add the valid class and clear the error message?

When the DOM is ready, the input values can be stored in a variable:

$(function() { //DOM-ready
    //cache the inputs from the DOM so as to minimize DOM queries.
    const inputs = $(":input");

Note that texetarea, select were removed from the selector because the jQuery :input selector includes those.

Then inputs can be used for registering the blur callback:

inputs.on('blur', function(ev) {

And then in the success callback of the AJAX request, inputs can be used again with $.each():

success: function(data) {
    if (data) {
      $.each(inputs, function(index, input) {

Then to determine if the field is invalid, check if the id
 attribute exists as a property of data. jQuery(input).prop() can be used to get the id property. Then data.hasOwnProperty() can be used to determine if the id property is set on data.

const id = jQuery(input).prop('id');
if (data.hasOwnProperty(id)) {
  jQuery(input).addClass('input-error')
      .removeClass('input-valid'); //chain calls for efficiency
  jQuery("#" + id + "-error").text(data[id]);
}
else {
  jQuery(input).removeClass('input-error')
      .addClass('input-valid');
  jQuery("#" + id + "-error").text("");
}

For a demonstration of this, see this plunker. Note that the AJAX response is not dynamic.

Actually, that last block could be re-written to utilize jQuery's toggleClass() method.

let message = '';
if (data.hasOwnProperty(id)) {
    message = data[id];
}
jQuery(input).toggleClass('input-error', message !== '')
    .toggleClass('input-valid', message === '');
jQuery("#" + id + "-error").text(message);

See a demonstration of that in this plunker.


Gathers pair of names (first + last) and separate them into two files I'm a beginner in Python programming. Currently, I learn how to work on the files...
I've created a program, which excludes the names (first & last) from each row, that contains both of them. When the pair of names does not include for example the Last Name part, it places n/a in the last_names.txt file.

Like from the file names.txt:

Paul Morgan
James Kowalski
Thomas

sort to two files

firstnames.txt:

Paul
James
Thomas

lastnames.txt:

Morgan
Kowalski
n/a

"""
Program separating first names and last names gathered in pairs from one file and saving them into two files.
"""

"Global Variables:"
allNamesFromFile = []

def names_separator(fileName):
    with open(fileName, "r+", encoding="utf-8-sig") as namesFromFile:
        for names in namesFromFile:
            allNamesFromFile.append(tuple(names.replace("\n", "").split(" ")))
        open("last_names.txt", "w").close() #cleaning the last_names.txt file
        open("first_names.txt", "w").close() #cleaning the first_names.txt file
        lastNamesFile = open("last_names.txt", "a+", encoding="utf-8-sig")
        firstNamesFile = open("first_names.txt", "a+", encoding="utf-8-sig")
        for namesTuples in allNamesFromFile:
            try:
                lastNamesFile.write(namesTuples[1] + "\n")
            except:
                lastNamesFile.write("n/a\n")
            try:
                firstNamesFile.write(namesTuples[0] + "\n")
            except:
                firstNamesFile.write("n/a\n")
    lastNamesFile.close()
    firstNamesFile.close()

print("welcome in the names separator program...")
while True:
    fileName = str(input("Please provide the file name with its extension: "))
    try:
        names_separator(fileName)
        print("operation completed")
        break
    except:
        print("You have provided an incorrect file name...")
        continue

I wonder if there is anything I could improve in this code... I wanted to use list + tuples.
The program works correctly, but I want to review it and maybe improve if it's possible. Maybe I could write it shorter/better, or just its functionality can be optimized?

Thanks in advance.

The functionality of your program seems slightly weird. You read multiple files, and each one gets split, but in doing so you (explicitly) delete the old data, is this right?

One thing I notice is this piece of code here:

open("last_names.txt", "w").close() #cleaning the last_names.txt file
open("first_names.txt", "w").close() #cleaning the first_names.txt file
lastNamesFile = open("last_names.txt", "a+", encoding="utf-8-sig")
firstNamesFile = open("first_names.txt", "a+", encoding="utf-8-sig")

This could be replaced by:

lastNamesFile = open("last_names.txt", "w", encoding="utf-8-sig")
firstNamesFile = open("first_names.txt", "w", encoding="utf-8-sig")

I'd also move it to the start of your function:

with open(fileName, "r", encoding="utf-8-sig") as namesFromFile,
     open("last_names.txt", "w", encoding="utf-8-sig") as lastNamesFile,
     open("first_names.txt", "w", encoding="utf-8-sig") as firstNamesFile:

So you can remove the close statements at the end. I'm not sure what the encoding= does, but it might not be necessary.

You can also reduce this:

    for names in namesFromFile:
        allNamesFromFile.append(tuple(names.replace("\n", "").split(" ")))

    for namesTuples in allNamesFromFile:
        try:
            lastNamesFile.write(namesTuples[1] + "\n")
        except:
            lastNamesFile.write("n/a\n")
        try:
            firstNamesFile.write(namesTuples[0] + "\n")
        except:
            firstNamesFile.write("n/a\n")

To this:

for names in namesFromFile:
    names_split = tuple(names.replace("\n", "").split(" "))
    if len(names_split) >= 1:
        firstNamesFile.write(names_split[0] + "\n")
    else:
        firstNamesFile.write("n/a\n")

    if len(names_split) >= 2:
        lastNamesFile.write(namesTuples[1] + "\n")
    else:
        lastNamesFile.write("n/a\n")

Allowing a for loop less and two less try ... except blocks as well as a global list which can now be left out:

"Global Variables:"
allNamesFromFile = []

I don't think this is necessary:

fileName = str(input("Please provide the file name with its extension: "))

Instead use:

fileName = input("Please provide the file name with its extension: ")

I'd also look at this part:

try:
    names_separator(fileName)
    print("operation completed")
    break
except:
    print("You have provided an incorrect file name...")
    continue

You are catching a lot of exceptions, try to be more precise. Instead try:

except FileNotFoundError:

Lastly, you might want to check naming conventions, variables should be in snake_case and classes in CamelCase.

So, to summarize:

check if your program does what you want it to do
open files using with
limit the amount of global variables to 0
limit the amount of try ... except blocks
be explicit in the exceptions you're catching
check pep8 style guide for naming of variables


Is this a good user control layout for WPF? Below is a code snippet of how I have been laying out my WPF control XAML. I do it mainly because I get intellisense in the editor and I can change the implementation of the viewmodel and push the layout into a resource file.

However, I am not sure whether you gurus have a better insight into why I should not do this?

I'm just after a critique really.

<UserControl.Resources>
<ResourceDictionary>
    <ResourceDictionary.MergedDictionaries>
        <ResourceDictionary Source="../../Styles/BaseStyles.xaml"/>
    </ResourceDictionary.MergedDictionaries>

    <DataTemplate x:Key="LayoutRoot"
                  DataType="{x:Type ns:IViewModel}">

        <Grid Height="200" Width="300">
            <Grid.RowDefinitions>
                <RowDefinition Height="Auto" />
                <RowDefinition Height="Auto" />
                <RowDefinition Height="*"/>
                <RowDefinition Height="Auto" />
            </Grid.RowDefinitions>

            <Grid.ColumnDefinitions>
                <ColumnDefinition Width="Auto"/>
                <ColumnDefinition Width="*"/>
            </Grid.ColumnDefinitions>

            <Label Content="Some Label" Style="{StaticResource BoldLabel}" Margin="5, 0"
                   Target="{Binding ElementName=Lcb1}"/>

            <endorsements:LimitComboBox Grid.Row="0" Grid.Column="1" x:Name="Lcb1"
                            Limit="{Binding SomeData}" />

            <Label Grid.Row="1" Grid.Column="0" Content="Some Other Label"
                   Style="{StaticResource BoldLabel}" Margin="5, 0" Target={Binding ElementName=Lcb2/>

            <endorsements:LimitComboBox Grid.Row="0" Grid.Column="1" x:Name="Lcb2"
                            Limit="{Binding SomeOtherData}" />

            <StackPanel Grid.Row="3" Grid.Column="0" Orientation="Horizontal" HorizontalAlignment="Right" Grid.ColumnSpan="2">
                <Button Content="OK" Command="{Binding ApplyCommand}" IsDefault="True"/>
                <Button Content="Cancel" IsCancel="True"/>
            </StackPanel>
        </Grid>
    </DataTemplate>
</ResourceDictionary>

There are 2 things that I could critisize on your code with the information you gave us:

Don't load style resources inside the usercontrol. Let the application hold control over how to style a usercontrol
Try not to use a grid in this context, because it prevents the control from adopting itself to new sizes of the window. StackPanels and Dockpanel are much better for this


OOP and connecting to multiple databases using PDO I'm just starting to learn OOP and I've setup a class to handle the database connections and wanted to check if I'm going about this the right way.

My website is already using PDO for prepared statements but it's procedural code otherwise. The site connects to multiple databases and one of the databases has two connections for separate privileges.

I have created a Dsn class which contains a method for each connection. My config file instantiates the class and opens all the connections using the methods and passing params where necessary. I'll put the code below but just wanted to check if connecting to multiple databases in this way through a single class follows best practices for OOP.

config.php

$dbh = new Dsn;
$pdo = $dbh->connnect1Read(NAME1, USER2, PASS2);
$pdo2 = $dbh->connnect2Write(NAME1, USER1, PASS1);
$pdo3 = $dbh->connnect3(USER2, PASS2);
$pdo4 = $dbh->connnect4(USER3, PASS3);
$pdo5 = $dbh->connnect5(USER4, PASS4);

dsn.php

    class Dsn
{

 private $host = 'localhost';
 private $charset = 'utf8mb4';

 private $dbName1;
 private $userName1;
 private $userPass1;

 private $dbName2 = 'db2';
 private $userName2;
 private $userPass2;

 private $dbName3 = 'db3';
 private $userName3;
 private $userPass3;

 private $dbName4 = 'db4';
 private $userName4;
 private $userPass4;

 private $opt = [
                            PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
                            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
                            PDO::ATTR_EMULATE_PREPARES   => false,
                      ];

  public function connnect1Read($dbName, $userName, $userPass)
  {
    $this->dbName1 = $dbName;
    $this->userName2 = $userName;
    $this->userPass2 = $userPass;

    $dsn = "mysql:host=" . $this->host . ";dbname=" . $this->dbName1 . ";charset=" . $this->charset . "";
    $pdo1 = new PDO($dsn, $this->userName2, $this->userPass2, $this->opt);

    return $pdo1;
  }

  public function connnect2Write($dbName1, $userName, $userPass)
  {
    $this->dbName1 = $dbName1;
    $this->userName1 = $userName;
    $this->userPass1 = $userPass;

    $dsn = "mysql:host=" . $this->host . ";dbname=" . $this->dbName1 . ";charset=" . $this->charset . "";
    $pdo2 = new PDO($dsn, $this->userName1, $this->userPass1, $this->opt);

    return $pdo2;
  }

  public function connnect3($userName, $userPass)
  {
    $this->userName2 = $userName;
    $this->userPass2 = $userPass;

    $dsn = "mysql:host=" . $this->host . ";dbname=" . $this->dbName2 . ";charset=" . $this->charset . "";
    $pdo4 = new PDO($dsn, $this->userName2, $this->userPass2, $this->opt);

    return $pdo3;
  }

  public function connnect4($userName, $userPass)
  {
    $this->userName3 = $userName;
    $this->userPass3 = $userPass;

    $dsn = "mysql:host=" . $this->host . ";dbname=" . $this->dbName3 . ";charset=" . $this->charset . "";
    $pdo5 = new PDO($dsn, $this->userName3, $this->userPass3, $this->opt);

    return $pdo4;
  }

  public function connnect5($userName, $userPass)
  {
    $this->userName4 = $userName;
    $this->userPass4 = $userPass;

    $dsn = "mysql:host=" . $this->host . ";dbname=" . $this->dbName4 . ";charset=" . $this->charset . "";
    $pdo3 = new PDO($dsn, $this->userName4, $this->userPass4, $this->opt);

    return $pdo5;
  }
}

This is not how things are done in the programming world. You don't write a dozen functions doing the same thing. You write just one function.

Speaking of OOP, there is no use for it here. Given your class consists of just one method, just make it a function:

function connnect($dbName, $userName, $userPass)
{
    $host = 'localhost';
    $charset = 'utf8mb4';
    $opt = [
                        PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
                        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
                        PDO::ATTR_EMULATE_PREPARES   => false,
                  ];
    $dsn = "mysql:host=$host;dbname=$dbName;charset=$charset";
    return new PDO($dsn, $userName, $userPass, $opt);
}

and then create your connections:

$dbRead = connnect(NAME1, USER2, PASS2);
$dbWrte = connnect(NAME1, USER1, PASS1);
$db3 = connnect(NAME3, USER2, PASS2);
$db4 = connnect(NAME4, USER3, PASS3);
$db5 = connnect(NAME5, USER4, PASS4);


Renaming picture files with date I am wondering if this is a safe and/or sensible way of going about doing the following task:

My problem is that I have a folder with loads of directories with names of form YYYY-MM-DD. They are from my camera. Each directory contains picture files. The problem is that the picture files are designated by just 4 digits like C4018.jpeg. Hence after a few days of shooting I will find myself with duplicate file names! My objective is then to just add the date of the containing folder to each file, which solves my problem.

Here is my current solution. I tested it so that it also doesn't keep adding the date each time I run the code - once the files have been renamed they are not renamed in the future.

Am I safe running this code? By safe, I mean I don't end up with a renaming catastrophe. Also, please feel free to comment on the efficiency and style of my code.

 import os
 import re
 rootdir = '/Volumes/Misc/Pictures/test/' #Location of video folders to rename

  # Regexp for YYYY-MM-DD or YYYY/MM/DD
  date_re = re.compile('\d{4}[-/]\d{2}[-/]\d{2}')
  subdirs = [subdir[0] for subdir in os.walk(rootdir)]

  for subdir in subdirs:
      match = date_re.findall(subdir)
      if match:
          os.chdir(subdir)
          for (_,_,files) in os.walk(os.getcwd()):
              for f in files:
                  match2 = date_re.findall(f)
                  if not match2:
                      new_name = match[0] + '-' + f
                      os.rename(f, new_name)
          os.chdir('..')

Why don't you use the exif timestamps of your images and name them accordingly? I use the following code to rename my images. It also extends the timestamp to feature not only the date but also the time:

import os
import glob
from PIL import Image
from PIL.ExifTags import TAGS
import time

def get_exif(fn):
    ret = {}
    i = Image.open(fn)
    info = i._getexif()
    for tag, value in info.items():
        decoded = TAGS.get(tag, tag)
        ret[decoded] = value
    return ret

os.chdir("/path/to/images")

files = glob.glob("*.JPG")

for file in files:
    print(file)
    time = get_exif(file)["DateTimeOriginal"]

    time = time.replace(":", "")
    time = time.replace(" ", "_")
    number = 0
    new_name = time+"_additional_information.jpg"
    if new_name == file:
        print(new_name, "already ok")
        continue
    while os.path.exists(new_name):
        number += 1
        new_name = time+"_"+str(number)+"_additional_information.jpg"
    os.rename(file, new_name)

If you want to use your own code, why don't you just test it on a copy of the data?


Simple inheritance with functions in subclasses It seems like there are many ways implementing inheritance in JavaScript. Which version do you prefer or would you suggest another solution?

The code should run in IE8+ so my first version would not fit…

function Animal(name) {
    this.name = name
}
Animal.prototype = {
    eat: function() {
        console.log(this.name + ': *eat*')
    }
}

function Cat(name) {
    Animal.call(this, name);
}
Cat.prototype = Object.create(Animal.prototype, {
    miaou: {
        value: function() {
            console.log(this.name + ': *miaou*')
        }
    }
})

var garfield = new Cat("Garfield");
garfield.eat();     // Garfield: *eat*
garfield.miaou();   // Garfield: *miaou*

// ------------
//
// possible alternative:

// […] see http://ejohn.org/blog/simple-javascript-inheritance/ for definition
// of Class

var Animal2 = Class.extend({
    init: function(name) {
        this.name = name
    },
    eat: function() {
        console.log(this.name + ': *eat*')
    }
});

var Cat2 = Animal2.extend({
    init: function(name) {
        this._super(name)
    },
    miaou: function() {
        console.log(this.name + ': *miaou*')
    }
});

var garfield2 = new Cat("Garfield2");
garfield2.eat();        // Garfield: *eat*
garfield2.miaou();      // Garfield: *miaou*

As Joseph mentioned, opinion questions is not what CodeReview is about.

I tend to review every OO approach with these 4 questions:

Approach 1

Does instanceof still work ? -> Yes, good
Do you introduce new properties, causing potential naming clashes -> No, good
Can parameters easily be provided to the constructors -> Yes, good
Can I easily have properties in .prototype -> Yes, good

Approach 2

Does instanceof still work ? -> Yes, good
Do you introduce new properties, causing potential naming clashes -> Yes, not so good
Can parameters easily be provided to the constructors -> Yes, good
Can I easily have properties in .prototype -> Yes, good

The second approach uses _super, which is not perfect but definitely acceptable, and the framework was written by John Resig..

In the end, you should use the right tool for the job, and since you provided example code, we can't really tell you which approach is best.


Delete unmatched rows in Excel I'm looking to optimize my for loop to search faster. I've turned off ScreenUpdating, EnableEvents, Calculation, and DisplayStatusBar to speed it up a litte. I have about 10,000 rows to search for in the FAR tab. It works by reading the FAR tab rows and if it matches the master account list ("Account_Range") located in the Input tab, it won't delete the row. It will then update the rows (not shown above). I ran this and it took exactly an hour to run.

Sub DeleteRowsWithArray()

Dim dontDelete() As Variant
Dim actRange As range
Set actRange = Sheets("Input").range("Account_Range")

dontDelete = actRange

Dim i As Long, j As Long

Dim isThere As Boolean

Sheets("FAR").Select

For i = Cells(Rows.Count, "B").End(xlUp).Row To 3 Step -1
    For j = LBound(dontDelete) To UBound(dontDelete)
        If StrComp(range("B" & i), dontDelete(j, 1), vbTextCompare) = 0 Then
            isThere = True
        End If
    Next j
    If Not isThere Then
        range("B" & i).Delete shift:=xlUp
    End If
        isThere = False
Next i

End Sub

In your code there are several slow items:

1.

StrComp(Range("B" & i), dontDelete(j, 1), vbTextCompare) = 0
I would use Range("B" & i).Value2 = dontDelete(j, 1).Value2

2.

Like PeterT suggested deleting rows is quite slow, specially when you do it one at the time
I would hide the rows to keep, and at the end delete all visible rows in one operation

3.

Another significant performance issue is caused by interacting with the sheet
You could copy both ranges into 2 separate variants and loop them the same way as ranges

Bellow I'm providing a different approach:

create a new column with a formula that will find all matches from Account_Range
use AutoFilter to hide the rows you want to keep
delete all visible rows in one operation

Option Explicit

Public Sub DeleteRowsFilter()
  Const FAR_ROW1 As Long = 3

  Dim lrInput As Long, lrFAR As Long, lcFAR As Long, lastCell As Range
  Dim srcRng As String, fCell As Range, formulaRng As Range, t As Double

  FastWB
  t = Timer

  With ThisWorkbook.Worksheets("FAR")
    Set lastCell = GetMaxCell(.UsedRange)
    lrFAR = lastCell.Row
    lcFAR = lastCell.Column

    'create a new column showing if row should be deleted (value = 0) or not (value > 0)
    Set fCell = .Cells(FAR_ROW1, lcFAR + 1)
    fCell.Formula = "=IFERROR(MATCH(B" & FAR_ROW1 & ",Account_Range,0),0)"

    'fill down the formula to last row
    Set formulaRng = .Range(fCell, .Cells(lrFAR, lcFAR + 1))
    fCell.AutoFill Destination:=formulaRng: .Calculate

    'hide the rows to keep
    formulaRng.AutoFilter Field:=1, Criteria1:="=0"

    'delete visible rows (not matching values in Account_Range)
    fCell.Offset(1).Resize(lrFAR - FAR_ROW1).SpecialCells(xlCellTypeVisible).EntireRow.Delete
    .Columns(lcFAR + 1).EntireColumn.Delete 'delet last column (temp formula)
  End With

  Debug.Print "Duration:" & vbTab & Format(Timer - t, "#,##0.0000") & " sec" & vbCrLf
  FastWB False
End Sub

In my test file I only used 2 columns on each sheet, both with 10,000 rows
This version deleted more than 6,000 rows

Total Duration:   1.9338 sec

I also used 2 helper functions

Turn off visual elements:

Public Sub FastWB(Optional ByVal opt As Boolean = True)
    With Application
        .Calculation = IIf(opt, xlCalculationManual, xlCalculationAutomatic)
        .DisplayAlerts = Not opt
        .DisplayStatusBar = Not opt
        .EnableAnimations = Not opt
        .EnableEvents = Not opt
        .ScreenUpdating = Not opt
    End With
End Sub

Determine the last row and column:

Public Function GetMaxCell(Optional ByRef rng As Range = Nothing) As Range

    'Returns the last cell containing a value, or A1 if Worksheet is empty

    Const NONEMPTY As String = "*"
    Dim lRow As Range, lCol As Range

    If rng Is Nothing Then Set rng = Application.ActiveWorkbook.ActiveSheet.UsedRange
    If WorksheetFunction.CountA(rng) = 0 Then
        Set GetMaxCell = rng.Parent.Cells(1, 1)
    Else
        With rng
            Set lRow = .Cells.Find(What:=NONEMPTY, LookIn:=xlFormulas, _
                                        After:=.Cells(1, 1), _
                                        SearchDirection:=xlPrevious, _
                                        SearchOrder:=xlByRows)
            If Not lRow Is Nothing Then
                Set lCol = .Cells.Find(What:=NONEMPTY, LookIn:=xlFormulas, _
                                            After:=.Cells(1, 1), _
                                            SearchDirection:=xlPrevious, _
                                            SearchOrder:=xlByColumns)

                Set GetMaxCell = .Parent.Cells(lRow.Row, lCol.Column)
            End If
        End With
    End If
End Function


Triggering a callback when user put in the Konami cheat code anywhere in the page I wrote this just for some fun and also to practise my ES6 skills.
When user key in the Konami Cheat Code(⬆⬆⬇⬇⬅➡⬅➡↩) anywhere in the page, it should trigger a callback. In the following example, it should trigger an alert.

What could be improved?

Are there more ES6 features I could use in this code?

const {onKonamiSequenceTriggered, clearKonamiSequanceTriggered} = (() => {
    const konamiSequence = ["ARROWUP", "ARROWUP", "ARROWDOWN", "ARROWDOWN", "ARROWLEFT", "ARROWRIGHT", "ARROWLEFT", "ARROWRIGHT", "B", "A", "ENTER"];
    konamiSequence.reverse();
    let currentSequence = Array.prototype.slice.call(konamiSequence);

    const callbacks = {};

    let eventId = 0;

    const onKonamiSequenceTriggered = callback => {
        callbacks[eventId] = callback;
        return eventId++;
    };

    const clearKonamiSequanceTriggered = id => delete callbacks[id];

    document.addEventListener("keyup", event => {
        if (event.key.toUpperCase() !== currentSequence.pop()) {
            currentSequence = Array.prototype.slice.call(konamiSequence);
        }
        if (!currentSequence.length) {
            Object.values(callbacks).forEach(callback => {
                try {
                    callback();
                } catch (e) {
                    console.error(e);
                }
            });
            currentSequence = Array.prototype.slice.call(konamiSequence);
        }
    });

    return {
        onKonamiSequenceTriggered,
        clearKonamiSequanceTriggered
    };
})();

onKonamiSequenceTriggered(() => alert("You've got it!"));

You clone an array 3 times by using:

currentSequence = Array.prototype.slice.call(konamiSequence);

but with ES6, you can also use the spread operator to clone an array:

currentSequence = [... konamiSequence];

It took me a while to figure out the complex outer construction in your code:

const {onKonamiSequenceTriggered, clearKonamiSequanceTriggered} = (() => {

    .... actual code ...

    return {
        onKonamiSequenceTriggered,
        clearKonamiSequanceTriggered
    };
})();

It is an object destructuring assignment which is assigned two methods by using a self-executing anonymous function. I guess the advantage is that you can use local scope within the anonymous function. Note that you could leave out all of the code, in the excerpt above, and your code would still work, albeit completely in global scope.

I did find code on Stack Overflow which looks similar, but it is obviously not the same.

I'm not an Javascript expert, so that's all I have. I learned something from this, but I still think that Javascript has some awful language constructs (in other words: I'm not used to it).


Calculate the lattice paths in an n*n lattice (Project Euler #15) Project Euler #15 is to find out all the distinct possible ways there are to move from the first point (1,1) to point (n,n) in an n*n lattice.

def lattice_paths_of_n(n):
    list2 = []
    my_list = []
    for i in range(1, n+2):
        list2.append(i)
    for i in range(1, n+2):
        my_list.append(list2)
    for i in range(0,n+1):
        for f in range(0,n+1):
            if f == 0 or i == 0:
                my_list[i][f] = 1
            else:
                my_list[i][f] = my_list[i-1][f]+my_list[i][f-1]
    return my_list[n][n]

print(lattice_paths_of_n(20))

However, this function is extremely inefficient and I would appreciate it if you could give me suggestions to optimize the same. I tried to find a more efficient solution to this problem on this site, but couldn't find one in Python 3.

To move from the top left corner of an \$n\times n\$ grid to the opposite one you have to move \$n\$ times to the right and \$n\$ times down.
So in total you will do \$2n\$ moves.
If you could make those in any order there would be \$(2 n)!\$ ways of doing them.
But you don't have that freedom, because the order within the movements to the right and within the movements down is fixed, e.g. you have to move from row 4 to row 5 before you can move from row 5 to row 6.
So of the \$n!\$ ways the movements to the right can be ordered, only one is valid, and similarly with the movements down.

Summing it all up, the closed form answer to that problem is:

$$ \frac{(2n)!}{n!n!} $$

Unsurprisingly this is the same as \$C_{2n, n}\$, the combinations of \$2n\$ items taken \$n\$ at a time.
You could think of the same problem as having \$2n\$ movements and having to choose \$n\$ of those to make e.g. the movements down, leaving the others for the movements right.

With Python's arbitrary size integers you could simply calculate that as:

import math

def pe15(n):
    n_fact = math.factorial(n)
    return math.factorial(2 * n) / n_fact / n_fact

To give this answer a little more meat, in most other languages you don't have the luxury of not having to worry about integer overflows.
Even in Python it may be a good idea if you want to keep your computations fast for very large numbers.
So you would typically do the same computation as:

def pe15_bis(n):
    ret = 1
    for j in range(1, n+1):
        ret *= n + j
        ret //= j
    return ret

In Python that doesn't seem to pay off (performance wise) until n is in the many hundreds, but I find lovely that, the way that code goes, ret is always exactly divisible by j.
Figuring out why is left as an exercise for the reader...


Explicit song lyrics checker To stay in practice with my Python I've decided to write an explicit song checker. It checks each word in the song against an array of 14 explicit words contained in the text file. I've decided not to include the words that are to be checked against, for I am not sure about the rule of having explicit words in a program's code. Feedback on efficiency and structure is what I'm going for mostly. Style and other critiques are invited as well.

explicit_song_checker.py

explicit_words = [
    #explicit words not shown
]

def isClean(song_path):
    with open(song_path) as song:
        for line in song:
            words = line.split(" ")
            for word in words:
                for explicit_word in explicit_words:
                    if word == explicit_word:
                        return False

        return True

def main():
    song = raw_input("Enter path to song: \n")
    if isClean(song):
        print("CLEAN")
    else:
        print("EXPLICIT")

if __name__ == '__main__':
    main()

I recommend practicing Python 3 rather than Python 2 these days.

According to PEP 8, isClean() should be is_clean().

One or more of your loops could be replaced by some use of the any() function.  Note that this suggests that an is_explicit() function would be a more natural concept than an is_clean() function.

I would expect song lyrics to contain a mixture of uppercase and lowercase.  Words may be delimited by punctuation as well as spaces.  Therefore, words = line.split(" ") is probably too naïve.  Furthermore, I would expect each song to be short enough to fit entirely in memory very comfortably, so processing each file line by line is an unnecessary complication.

I would rewrite the program to use a regular expression instead.

import re

explicit_words = [
    …
]

is_explicit = re.compile(
    r'\b(?:' +
        '|'.join(re.escape(w) for w in explicit_words) +
    r')\b',
    re.IGNORECASE).search

def main():
    with open(raw_input("Enter path to song: ")) as song:
        print("EXPLICIT" if is_explicit(song.read()) else "CLEAN")

if __name__ == '__main__':
    main()

The way I chose to write the is_explicit() function above might show too much Haskell influence for some Python programmers' taste.  Here's a more conventional way of writing it:

def is_explicit(text):
    return re.search(
        r'\b(?:' +
            '|'.join(re.escape(w) for w in explicit_words) +
        r')\b',
        text,
        re.IGNORECASE
    )


Calculating the font size to fit the width of a website I created a function to calculate the font size to fit the width of the website I'm doing. I believe the code is a bit ugly and could use some improvement by creating a more logical function, faster and less heavy.

Can someone look at the code and comment on it for my own improvement?

//Script jquery

$(window).resize(function() {
  newWidthGhost = $('#ghostFontes').width();
  widthFontsResize();
});

$(document).ready(function(){
    $('body').find('*').addClass('wFonte');

$('.wFonte').each(function(i, el){
    fontHeight[i] = $(el).css('font-size');
});

oldWidthGhost = $('#ghostFontes').width(); // TAMANHO WIDTH;

widthFontsReady();

});

function widthFontsResize(){
    $('.wFonte').each(function(i, el){
        nFonte['e'+i] = parseInt((newWidthGhost * nFonte[i]) / oldWidthGhost);
        $(el).css({'font-size': nFonte['e'+i]+'px'});
});

}

function widthFontsReady(){
    $('.wFonte').each(function(i, el){
        nFontHeigth[i] = parseInt(fontHeight[i].substr(0,2));
        nFonte[i] = parseInt((oldWidthGhost * nFontHeigth[i]) / 1586);
        $(el).css({'font-size': nFonte[i]+'px'});
});

}

Use strict-mode

Start your code with "use strict";:
https://stackoverflow.com/questions/1335851/what-does-use-strict-do-in-javascript-and-what-is-the-reasoning-behind-it

Don't pollute the global namespace

When you don't use var x; to declare a variable, it is implicitly declared in the global namespace. If another script does the same, both scripts will be using the same variable.

It is not very easy for this to happen with an odd name like newWidthGhost, but fontHeight sounds like a better candidate for these surprises.

Run the code through JSLint

Run your code through JSlint, and try to understand the errors and suggestions it gives you.

That alone will greatly improve the quality of your code.


Print Longest Decreasing Segment I'm trying to brush up on my Dynamic Programming by doing some example problems. I've got the following...

Given an array of integers, print the longest decreasing segment.

For example {4,3,2,1,5,6,7,6,5,4,3,2,1} would print "7, 6, 5, 4, 3, 2, 1"

The following is my solution.. I think it looks okay and it has a decent O(n) runtime. However, the space complexity is O(n^2) due to the extra "length" array storing state. Is it possible to get rid of it? Any other problems?

int[] segment = {4,3,2,1,5,6,7,6,5,4,3,2,1};
int[] length =  {0,0,0,0,0,0,0,0,0,0,0,0,0};

public String getDecreasingSegment(){

    int maxStart = 0;
    int maxEnd = 0;

    for(int i = 1; i < segment.length; i++){

        if(segment[i] < segment[i-1]){

            length[i] = length[i-1]+1;

            if(length[i] > length[maxEnd]){

                maxEnd = i;
                maxStart = maxEnd - length[maxEnd];

            }

        }else{

            length[i] = 0;

        }

    }

    StringBuffer buff = new StringBuffer();

    for(int i = maxStart; i <= maxEnd; i++){

        buff.append(segment[i] + ", ");

    }

    return buff.toString();
}

As said above adding an extra Array does not change the space complexity to n^2.

Another thing is you do not need that extra Array also. All you need is 3 or 4 variables.
You can store the previous beginIndex and length of and the currentBeginIndex and length and if it crosses the previous maxLength then store the max as the crrentMax else continue with the iteration.

Let me know if you want some code sample.


Parallel download of rpm packages I use zypper to download and install for openSUSE Tumbleweed (much like everyone else of course). Unfortunately it downloads packages serially, and having about a thousand packages to update a week, it gets quite boring. Also, while zypper can download packages one-by-one in advance, it can't be called concurrently. I found the libzypp-bindings project but it is discontinued. I set myself to improve the situation.

Goals:

Download all repositories in parallel (most often different servers);
Download up to MAX_PROC (=6) packages from each repository in parallel;
Save packages where zypper picks them up during system update: /var/cache/zypp/packages;
Alternatively, download to $HOME/.cache/zypp/packages;
Avoid external dependencies, unless necessary.

Outline:

Find the list of packages to update;
Find the list of repositories;
For each repository:

Keep up to $MAX_PROC curl processes downloading packages.

Copy files to default package cache;

#!/bin/bash

MAX_PROC=6

function repos_to_update () {
    zypper list-updates | grep '^v ' | awk -F '|' '{ print $2 }' | sort --unique | tr -d ' '
}

function packages_from_repo () {
    local repo=$1

    zypper list-updates | grep " | $repo " | awk -F '|' '{ print $6, "#", $3, "-", $5, ".", $6, ".rpm" }' | tr -d ' '
}

function repo_uri () {
    local repo=$1

    zypper repos --uri | grep " | $repo " | awk -F '|' '{ print $7 }' | tr -d ' '
}

function repo_alias () {
    local repo=$1

    zypper repos | grep " | $repo " | awk -F '|' '{ print $2 }' | tr -d ' '
}

function download_package () {
    local alias=$1
    local uri=$2
    local line=$3
    IFS=# read arch package_name <<< "$line"

    local package_uri="$uri/$arch/$package_name"
    local local_dir="$HOME/.cache/zypp/packages/$alias/$arch"
    local local_path="$local_dir/$package_name"
    printf -v y %-30s "$repo"
    printf "Repository: $y Package: $package_name\n"
    if [ ! -f "$local_path" ]; then
        mkdir -p $local_dir
        curl --silent --fail -L -o $local_path $package_uri
    fi
}

function download_repo () {
    local repo=$1

    local uri=$(repo_uri $repo)
    local alias=$(repo_alias $repo)
    local pkgs=$(packages_from_repo $repo)
    local max_proc=$MAX_PROC
    while IFS= read -r line; do
        if [ $max_proc -eq 0 ]; then
            wait -n
            ((max_proc++))
        fi
        download_package "$alias" "$uri" "$line" &
        ((max_proc--))
    done <<< "$pkgs"
    wait
}

function download_all () {
    local repos=$(repos_to_update)
    while IFS= read -r line; do
        download_repo $line &
    done <<< "$repos"
    wait
}

download_all
sudo cp -r ~/.cache/zypp/packages/* /var/cache/zypp/packages/

There's 2 or 3 places where grep/tr are subject to issues, but the nature of the data doesn't require much more than that.

I completely changed the approach which resulted in faster downloads, and improved functionality. The overall approach of the initial version works well for a generic solution looking for adding concurrency to independent jobs.

Removed zypper list-updates. This format is easier for machine consumption, but it's not intended for Tumbleweed. Replaced with zypper dup/inr/in --details;
Removed background jobs and waits. Replaced with aria2 which handles the maximum number of concurrent connections;
It pays off learning more about awk capabilities, in order to replace sequences of grep/awk/tr with a single awk;
The main job of the script became building a plain text file with URIs and target directory for each .rpm file

aria2 is really great tool. Superb quality. curl is not reliable in its native concurrent download capabilities.


Parsing different json to one model I have several json models which represent same logic model. e.g:
{"title":"title1","years":"31"} and {"name":"title1","age":31}

at the moment I make 2 different classes which implement same interface

 public class NameModel implements Person {

  @SerializedName("name") private String name;
  @SerializedName("age") private int age;

  public String getName() {
    return name;
  }

  public int getAge() {
    return age;
  }
}
public class TitleModel implements Person {

  @SerializedName("title") private String title;
  @SerializedName("years") private int years;

  public String getName() {
    return title;
  }

  public int getAge() {
    return years;
  }
}
public interface Person {
  String getName();
  int getAge();
}

Is this a correct approach to unify different models to one if they are essentially same, but with different field names?

in Jackson, you have the mixin feature. This allows to add or override annotations at run time.

so we have one target POJO

public class Person
{
    private String name;
    private int age;

    public void setName(String name) {
        this.name = name;
    }
    public void setAge(int age) {
        this.age = age;
    }
}

and the mixin classes can be interfaces (they never get initialized) with matching setter method signatures and different annotations

public interface NameModel
{
    @JsonProperty("name")
    void setName(String name);
    @JsonProperty("age")
    void setAge(int age);
}

public interface TitleModel
{
    @JsonProperty("title")
    void setName(String name);
    @JsonProperty("years")
    void setAge(int age);
}

at run-time you can associate one mixin to the target class.  Jackson matches properties and methods by name and takes the annotations from the mixin. overriding any from the target:

public static void main(String[] args) {
    String nameModelJson = "{\"name\":\"title1\",\"age\":31}";
    try (InputStream is = new ByteArrayInputStream(nameModelJson.getBytes())) {
        ObjectMapper mapper = new ObjectMapper();
        mapper.addMixIn(Person.class, NameModel.class);
        Person p = mapper.readValue(is, Person.class);
        System.out.println(p.name + " " + p.age);
    } catch (Exception e) {
        e.printStackTrace();
    }

    String titleModelJson = "{\"title\":\"title1\",\"years\":\"31\"}";
    try (InputStream is = new ByteArrayInputStream(titleModelJson.getBytes())) {
        ObjectMapper mapper = new ObjectMapper();
        mapper.addMixIn(Person.class, TitleModel.class);
        Person p = mapper.readValue(is, Person.class);
        System.out.println(p.name + " " + p.age);
    } catch (Exception e) {
        e.printStackTrace();
    }
}


Pull, Tag, Save, Delete Multiple Docker Containers First time posting and first real Python program that I use fairly frequently. I needed a way to pull a large amount of containers, re-tag them, save them (docker save) and clean up the recently pulled containers.

I did this pretty easily with bash, though depending on the container and depending how many containers I quickly did not like how slow it was and wanted a way to do it faster.

I am looking to make any improvements possible, it works, and it works well (at least I think). I tend to always come across more Pythonic ways to do things and would love the additional feedback.

Couple things that I am currently tracking:

I need docstrings (I know, small program, but I want to learn to do it
right)
I tossed in the if __name__ == "__main__":, and if I am being honest, I am not confident I did that right.
I know, I am terrible at naming functions and variables :(

#!/usr/bin/env python3

import sys
import os
from os import path
import concurrent.futures  # https://docs.python.org/3/library/concurrent.futures.html
import docker  # https://docker-py.readthedocs.io/en/stable/

cli = docker.APIClient(base_url="unix://var/run/docker.sock")
current_dir = os.getcwd()
repository = sys.argv[2]
tar_dir = os.path.join(current_dir, "move")

if path.exists(tar_dir) is not True:
    os.mkdir(tar_dir)

def the_whole_shebang(image):
    img_t = image.split(":")
    img = img_t[0].strip()
    t = img_t[1].strip()
    image = f"{img}:{t}"

    print(f"Pulling, retagging, saving and rmi'ing: {image}")
    # Pulls the container
    cli.pull(image)
    # Tags the container with the new tag
    cli.tag(image, f"{repository}/{img}", t)

    new_image_name = f"{img}{t}.tar"
    im = cli.get_image(image)
    with open(os.path.join(tar_dir, new_image_name), "wb+") as f:
        for chunk in im:
            f.write(chunk)

    # Deletes all downloaded images
    cli.remove_image(image)
    cli.remove_image(f"{repository}/{image}")

if __name__ == "__main__":
    with concurrent.futures.ProcessPoolExecutor() as executor:
        f = open(sys.argv[1], "r")
        lines = f.readlines()
        executor.map(the_whole_shebang, lines)

Anyways, I assume there are many things that I can do better, I would love to have any input so that I can improve and learn.

Thank you!

Global variables

These:

cli = docker.APIClient(base_url="unix://var/run/docker.sock")
current_dir = os.getcwd()
repository = sys.argv[2]
tar_dir = os.path.join(current_dir, "move")

probably shouldn't live in the global namespace. Move them to a function, and feed them into other functions or class instances via arguments.

Command-line arguments

Use the built-in argparse library, if not something fancier like Click, rather than direct use of sys.argv. It will allow nicer help generation, etc.

Boolean comparison

if path.exists(tar_dir) is not True:

should just be

if not path.exists(tar_dir):

Pathlib

Use it; it's nice! For instance, if tar_dir is an instance of Path, then you can replace the above with if not tar_dir.exists().

It should also be preferred over manual path concatenation like f"{repository}/{img}"; i.e.

Path(repository) / img

there are other instances that should also use pathlib; for instance rather than manually appending .tar, use with_suffix.

Split unpack

img_t = image.split(":")
img = img_t[0].strip()
t = img_t[1].strip()
image = f"{img}:{t}"

can be

image, t = image.split(':')
image_filename = f'{image.strip()}:{t.strip()}'


n-queens puzzle in Python I want to increase the efficiency and reduce the time complexity for the n-queen problem in n*n matrix chess. I am able to run only still (11*11) in normal time otherwise for the big number it is taking much more time. How can I improve performance?

import time

start_time = time.time()

def queensproblem(rows, columns):
    solutions = [[]]
    for row in range(rows):
        solutions = add_one_queen(row, columns, solutions)
    return solutions

def add_one_queen(new_row, columns, prev_solutions):
    return [solution + [new_column]
            for solution in prev_solutions
            for new_column in range(columns)
            if no_conflict(new_row, new_column, solution)]

def no_conflict(new_row, new_column, solution):
    return all(solution[row]       != new_column           and
               solution[row] + row != new_column + new_row and
               solution[row] - row != new_column - new_row
               for row in range(new_row))

count = 0
r = input("Enter number of ROWS for Chess/Matrix: ")
c = input("Enter number of COLUMNS for Chess/Matrix: ")

for solution in queensproblem(int(r), int(c)):
    count += 1
    print(solution)

print("\n Total number of solution is: ", count)

print("--- Time: %s seconds ---" % (time.time() - start_time))

Output

For a 4x4 matrix

Enter number of ROWS for Chess/Matrix: 4
Enter number of COLUMNS for Chess/Matrix: 4
[1, 3, 0, 2]
[2, 0, 3, 1]

 Total number of solution is:  2
--- Time: 2.3161020278930664 seconds ---

For a 12x12 matrix

Enter number of ROWS for Chess/Matrix: 12
Enter number of COLUMNS for Chess/Matrix: 12
[0, 2, 4, 7, 9, 11, 5, 10, 1, 6, 8, 3]
[0, 2, 4, 9, 7, 10, 1, 11, 5, 8, 6, 3]
<more output>

 Total number of solution is:  14200
--- Time: 29.522358894348145 seconds ---

To my surprise, rewriting no_conflict as

def no_conflict(new_row, new_col, solution):
    for row, col in enumerate(solution):
        if col == new_col or row + col == new_row + new_col or row - col == new_row - new_col:
            return False
    return True

resulted in a factor of 2 speedup. I have no explanation.

Other tweaks I tried gave very marginal performance changes.

As for the general review, the code is quite straightforward and readable. The only recommendation is to compute start_time after taking inputs. As written, you are timing not only the algorithm but also user's typing skills.


Chromium Nightly Build Downloader After realizing that I was spending a lot of time getting the latest version of Chromium, I figured I would throw together a quick script. Over time, I wanted to make the script more efficient, cleaner, and pythonic. Here is what I came up with:

from zipfile import ZipFile
from os import rename, remove, path
from shutil import rmtree, copytree
from httplib2 import Http
from sys import argv, exit
from easygui import indexbox

h = Http('.cache')
saveDir = 'C:\\Program Files\\Chromium\\'

def getLatestVersion():
    verSite = ('http://build.chromium.org/f/chromium/snapshots/' +
            'Win/LATEST')
    re, ver = h.request(verSite)
    ver = str(ver, encoding='utf8')
    return ver

def delCurrent():
    try:
        rmtree(path.join(saveDir, 'Current'))
    except Exception:
        print('Chromium could not be removed.')

def delBackup():
    try:
        rmtree(path.join(saveDir, 'Backup'))
    except Exception:
        print('Backup could not be removed.')

def downloadChromium(ver):
    site = ('http://build.chromium.org/buildbot/snapshots/Win/'
            + ver + '/chrome-win32.zip')
    re, chrome = h.request(site)
    file = open(path.join(saveDir, 'latest.zip'), 'wb')
    file.write(chrome)
    file.close()

def unzip():
    zip = ZipFile(saveDir + 'latest.zip', 'r')
    zip.extractall(saveDir)
    rename(path.join(saveDir, 'chrome-win32'), path.join(saveDir, 'Current'))
    zip.close()
    remove(path.join(saveDir, 'latest.zip'))

def revert():
    delCurrent()
    copytree(path.join(saveDir, 'Backup'), path.join(saveDir, 'Current'))

def backup():
    delBackup()
    copytree(path.join(saveDir, 'Current'), path.join(saveDir, 'Backup'))

def gui():
    ver = getLatestVersion()
    choices = ['Download version %s' % ver, 'Backup', 'Revert', 'Exit']
    choice = indexbox('What do you want to do?',
            'Chromium Downloader', choices)
    if choice == 0:
        delCurrent()
        downloadChromium(ver)
        unzip()
    elif choice == 1:
        backup()
    elif choice == 2:
        revert()
    elif choice == 3:
        exit()
    gui()

def usage():
    print('-h         Display help text\n' +
          '-g         Launches the GUI Program\n' +
          '-v         Only gets the version\n' +
          '-r         Reverts to last backup\n' +
          '-b         Saves a new backup\n' +
          '-o         Specify version to download\n')

def main():
    if '-g' in argv:
        gui()
        exit()
    elif '-h' in argv:
        usage()
        exit(2)
    elif '-r' in argv:
        revert()
        exit()

    if '-o' in argv:
        ver = argv.index('-o') + 1
    else:
        ver = getLatestVersion()
        print('Latest Version: ', ver)
    if '-v' in argv:
        exit()

    delCurrent()
    downloadChromium(ver)
    unzip()

    if '-b' in argv:
        backup()

if __name__ == "__main__":
    main()

Are there any major things that should be changed to make this code better?

I assume you are aware of Chrome Canary, which I gather is able to use binary deltas? I don't think it's pure Chromium though.

Your code is pretty clean. You could be using argparse for parsing the command line, currently you are ignoring flags that don't match. You could wrap the exit call around main, like this: exit(main()), and use return / return 2 statements instead. You could replace "C:\\Program Files" with %PROGRAMFILES%, or pick up some defaults from the registry, but that's not necessary unless you plan for a larger audience. You could also reduce the window Chromium is unavailable by unzipping somewhere else, then moving the directories, then clearing the previous one (but I don't think you can make it atomic without something like symlinks or hardlinks).


Anagram checking implementation Given two strings, check whether two given strings are anagram of each
  other or not. An anagram of a string is another string that contains
  same characters, only the order of characters can be different. For
  example, “act” and “tac” are anagram of each other.

  Input:

  The first line of input contains an integer T denoting the number of
  test cases. Each test case consist of two strings in 'lowercase' only,
  in a separate line.

  Output:

  Print "YES" without quotes if the two strings are anagram else print
  "NO".

  Constraints:

  1 ≤ T ≤ 30

  1 ≤ |s| ≤ 100

  Example:

  Input:

  2

  geeksforgeeks

  forgeeksgeeks

  allergy

  allergic

  Output:

  YES

  NO

My approach:

/*package whatever //do not write package name here */

import java.util.Scanner;
import java.io.IOException;
import java.util.HashMap;

class GFG {

    private static String isAnagram (String str1, String str2)
        {
            HashMap <Character, Integer>occurs = new HashMap<>();

            if (str1.length() != str2.length())
                {
                    return "NO";
                }

            for (char ch: str1.toCharArray())
                {
                    if (!(occurs.containsKey(ch)))
                        {
                            occurs.put(ch,1);
                        }
                    else
                        {
                            int count = occurs.get(ch);
                            occurs.put(ch,count + 1);
                        }
                }

            for (char ch: str2.toCharArray())
                {
                    if (!(occurs.containsKey(ch)))
                        {
                            return "NO";
                        }
                    else
                        {
                            int count = occurs.get(ch);
                            count  = count - 1;
                            if (count < 0)
                                {
                                    return "NO";
                                }
                            else
                                {
                                    occurs.put(ch,count);
                                }
                        }
                }

        return "YES";
        }

    public static void main (String[] args) throws IOException {
        Scanner sc = new Scanner (System.in);
        int numTests = sc.nextInt();

        for (int i = 0; i < numTests; i++)
            {
                String str1 = sc.next();
                String str2 = sc.next();
                System.out.println(isAnagram(str1, str2));
            }
    }
}

I have the following questions with regards to the above code:

1) How can I further improve my approach?

2) Is there a better way to solve this question?

3) Are there any grave code violations that I have committed?

4) Can space and time complexity be further improved?

Reference

Your use of the counting system using a HashMap is a functional approach to solving the problem, and algorithmically it's relatively good (has time complexity of \$O(N)\$). But, for a prolem like this, it's easier to normalize each input value in to a sorted array or string, than in to a counting HashMap.

Consider the following function:

public static final String normalize(value) {
    if (value == null) {
        return null;
    }
    char[] chars = value.toCharArray();
    Arrays.sort(chars);
    return new String(chars);
}

Now, all values that are anagrams will have the same normalize result (they have the same characters in the same sorted order).

Your code would simply become:

private static String isAnagram (String str1, String str2) {
    return Objects.equals(normalize(str1), normalize(str2));
}

Note that in this case, Objects is java.util.Objects.

Further, by putting the normalization in to a separate function it reduces the amount of logic duplication in your code.

Now, you ask if there are any grave violations... and I have to answer "yes". In Java it is common practice to put { braces on the same line as the code block definition. For example your code:

        for (char ch: str1.toCharArray())
            {
                if (!(occurs.containsKey(ch)))
                    {
                        occurs.put(ch,1);
                    }
                else
                    {
                        int count = occurs.get(ch);
                        occurs.put(ch,count + 1);
                    }
            }

should be:

        for (char ch: str1.toCharArray()) {
            if (!(occurs.containsKey(ch))) {
                occurs.put(ch,1);
            } else {
                int count = occurs.get(ch);
                occurs.put(ch,count + 1);
            }
        }


python collectd prometheus scraper I've been tinkering with python for quite a while but have never taken on a challenge like this - I'm trying to write a collectd plugin to scrape metrics from prometheus endpoints.
My goals are to make it as stable as possible - i'd like to put this into production when i am happy with it. There are a couple of items that i haven't figured out yet (summary and histogram familytypes), so if anyone has any suggestions about how to do that, i would be very appreciative. As well as this, i am trying to be a pythonic as possible, but i am 100% self taught, so my code is probably all over the place.
I'm interested in any form of feedback, even if it means i need to go learn/un-learm some more stuff :)
import platform
import collectd
from prometheus_client.parser import text_string_to_metric_families
import requests

def config_func(config):
    endpoint_set = False
    endpoint_set = False
    interval = 60

    for node in config.children:
        key = node.key.lower()
        val = node.values[0]

        if key.lower() == 'endpoint':
            endpoint = val
            endpoint_set = True
        elif key.lower() == 'endpoint_name':
            endpoint_name = val
            endpoint_name_set = True
        elif key.lower() == 'interval':
            interval = val
        else:
            collectd.info(f'prometheus_scraper: Unknown config key "{key}"')

    if endpoint_set and endpoint_name_set:
        collectd.info(f'prometheus_scraper: python version {platform.python_version()}')
        collectd.info(f'prometheus_scraper: Using {endpoint} for {endpoint_name} on ')

        global ENDPOINT
        ENDPOINT = endpoint
        global ENDPOINT_NAME
        ENDPOINT_NAME = endpoint_name
        global INTERVAL
        INTERVAL = interval

def parse_func(metrics):

    for family in text_string_to_metric_families(metrics):
        for sample in family.samples:

            if family.type == 'summary':
                #print(family.type)
                pass
            elif family.type == 'histogram':
                #print(family.type)
                pass
            else:
                val = collectd.Values()
                val.plugin = ENDPOINT_NAME
                val.interval = INTERVAL
                val.type = family.type

                if len(sample.labels) > 0:
                    joined = ''.join(key + '_' + str(val) for key, val in sample.labels.items())
                    val.type_instance = sample.name + '.' + joined
                else:
                    val.type_instance = sample.name

                #print(f'type_instance: {val.type_instance}')

                val.values = [sample.value]
                val.dispatch()

def read_func():

    try:
        metrics = requests.get(url=ENDPOINT)
        metrics.raise_for_status()
        parse_func(metrics.text)
    except requests.exceptions.RequestException as e:
        collectd.error(f'failed to retrive metrics: {e}')

collectd.register_config(config_func)
collectd.register_read(read_func, INTERVAL)

https://github.com/mark-vandenbos/collectd_prometheus_scraper/

I can't test this code, so I will only make a few general remarks for the time being.
I was curious, since I actually started working on the Prometheus API yesterday, because I was asked to design a monitoring dashboard.

This code is un-Pythonic:
    global ENDPOINT
    ENDPOINT = endpoint
    global ENDPOINT_NAME
    ENDPOINT_NAME = endpoint_name
    global INTERVAL
    INTERVAL = interval

Assigning a variable to another variable like this: ENDPOINT = endpoint has no added value and is even confusing. All this is redundant.
Global variables are frowned upon, because they are seldom necessary in fact. There usually is a better way. Just pass the variables along to your functions and retrieve resulting values as necessary.
If you have constants, define them at the top of your code (if you have just one file), or put them in a separate config file.
I suggest that you create a config.py file, and move all your constants there. Then you import the file:
import config

And you can refer to your constants like this all across your code:
config.ENDPOINT

Source and there are more useful tips too. This is recommended reading for beginners and more experienced programmers as well.

You have this code:
        if family.type == 'summary':
            #print(family.type)
            pass
        elif family.type == 'histogram':
            #print(family.type)
            pass
        else:

pass is no operation (NOP). So you have code that does strictly nothing and should be removed. What you want is probably something like this:
if family.type not in ('summary', 'histogram'):

You have 3 functions: config_func, parse_func, read_func. It's obvious they are functions, so the _func suffix is superfluous. But the names are not descriptive enough. It doesn't tell me what you are reading or parsing. So I think your naming conventions should be revisited.
In a way I think the order of things is logical: configuration, reading results, parsing results. Gotta make it more obvious.

Something that is lacking badly: comments. They are not just for external reviewers but for your benefit too. After a while you will forget stuff and you will have to re-analyze your own code to figure out the flow and what kind of data is returned by a given function. I would recommended that you document each function with a docstring. 2-3 lines will do.
Document your functions, and describe what kind of data you are manipulating. When you have some ifs and are testing for a specific condition, it does not hurt to explain why in a comment, unless the purpose of the test is very obvious.
Since you put your code on Github there is an expectation that somebody else might want to download it, use it, possibly fork it. But then it is important to provide enough documentation. Lack of documentation can be forgiven for software that works straight out of the box without any configuration, but this is rare.

Quality control: in config_func (at the top of your code) you have repeated code:
endpoint_set = False
endpoint_set = False

A good IDE would help you spot those mistakes and also enforce PEP8 rules. In a job interview this would qualify as lack of attention to detail, just by proof-reading your code carefully you should have noticed :)

In a Python program it is customary to have a main method and use __main__ like this:
if __name__ == '__main__':
    collectd.register_config(config_func)
    collectd.register_read(read_func, INTERVAL)

Or better yet:
if __name__ == '__main__':
    main()

And put your launcher code in the main function. The benefit of doing so is that you can import your package (to reuse some functions) without actually launching it.

Now that I am reading what I wrote so far I realize that it's not very helpful. I only gave you general suggestions but I think they are still useful though. But I am kinda frustrated because you have not described what the finished product does look like. After reading your code I can't really figure it out. I don't have a Prometheus box handy so I can't run your code and pull out some data.
The code is quite short (< 100 lines) and yet it is obscure to me. I don't think it's only because I am Prometheus newbie. The code does not speak for itself. Comments would help make more sense of the overall approach.
I am not familiar with collectd so I can't comment much on that. I understand you are scraping metrics but what is the ultimate goal, how will the data be used, that's what is interesting.


Bitmap implementation I recently started cleaning up my projects and decided that I want to create a personal repository of useful bits of code. I found this bitmap code between them and heavily edited it and I'd appreciate if someone could review it, because I'm not that used to bitwise operations and such. Is the code portable enough, or there should be changes?

bitmap.h

#ifndef BITMAP__
#define BITMAP__

#define BIT (8*sizeof(byte))
#define BITMAP_NOTFOUND -1

typedef enum{false=0, true} bool;
typedef unsigned char byte;

bool bitmapGet   (byte *, int);
void bitmapSet   (byte *, int);
void bitmapReset (byte *, int);
int  bitmapSearch(byte *, bool, int, int);

#endif

bitmap.c

#include "bitmap.h"

static bool get  (byte,   byte);
static void set  (byte *, byte);
static void reset(byte *, byte);

/* CAREFUL WITH pos AND BITMAP SIZE! */

bool bitmapGet(byte *bitmap, int pos) {
/* gets the value of the bit at pos */
    return get(bitmap[pos/BIT], pos%BIT);
}

void bitmapSet(byte *bitmap, int pos) {
/* sets bit at pos to 1 */
    set(&bitmap[pos/BIT], pos%BIT);
}

void bitmapReset(byte *bitmap, int pos) {
/* sets bit at pos to 0 */
    reset(&bitmap[pos/BIT], pos%BIT);
}

int bitmapSearch(byte *bitmap, bool n, int size, int start) {
/* Finds the first n value in bitmap after start */
/* size is the Bitmap size in bytes */
    int i;
    /* size is now the Bitmap size in bits */
    for(i = start+1, size *= BIT; i < size; i++)
        if(bitmapGet(bitmap,i) == n)
            return i;
    return BITMAP_NOTFOUND;
}

static bool get(byte a, byte pos) {
/* pos is something from 0 to 7*/
    return (a >> pos) & 1;
}

static void set(byte *a, byte pos) {
/* pos is something from 0 to 7*/
/* sets bit to 1 */
    *a |= 1 << pos;
}

static void reset(byte *a, byte pos) {
/* pos is something from 0 to 7*/
/* sets bit to 0 */
    *a &= ~(1 << pos);
}

If you expect your code will ever be used from C++ (which a lot of C code is) then  you should not use any identifiers with a double underscore.

#ifndef BITMAP__
    //        ^^   May cause problems in C++

As this is reserved for the implementation in C++. Also the term BITMAP is very generic and you are very likely to clash with other libraries try and make it more unique to your project.

You are assuming that a char is 8 bits.

#define BIT (8*sizeof(byte))

The standard does not say this (it is at least 8 bits) but it is actually defined via the macro CHAR_BITS so you should use:

#define BIT (CHAR_BITS*sizeof(byte))

Your header file is how most people will first see your code and try and understand how to use it. Thus putting the parameter names into the header file is probably a good idea as it helps document their usage.

bool bitmapGet   (byte *, int);
void bitmapSet   (byte *, int);
void bitmapReset (byte *, int);
int  bitmapSearch(byte *, bool, int, int);

Additional the action of bitmapSearch is not obvious thus a comment on its usage in the header would probably be a good idea. Actually a small blurb about how the first parameter can be an array would probably be a good idea (as it is not obvious without reading the code).

Be careful with comments:

/* pos is something from 0 to 7*/

This is only true if you make assumptions (A: that sizeof(byte) == 1 B: you multiply this by 8). The comment should have been:

/* pos is a value between [0, BIT) */

Note the use of '[' on the left and ')' on the right. It is mathematical notation indicating that 0 is inclusive and BIT is not included in the range (it is common in both C/C++ documentation).


Check for palindrome in singly linked list Looking for code review, optimizations and best practices.

Node.java

 class Node {
    Node next;
    int data;

    public Node(int value) {
        data = value;
    }
}

Palindrome.java

 public class Palindrome {

        static Node reverse(Node head) {

            Node prev = null;
            Node current = head;
            Node next;
            while (current != null) {
                next = current.next;
                current.next = prev;
                prev = current;
                current = next;
            }
            head = prev;
            return head;
        }

        static public boolean isPalindrome(Node head) {

            Node current = head;
            int length = 0;

            // Get the length of the LinkedList
            while (current != null) {
                current = current.next;
                length++;
            }
            // Find position of next to mid element
            int mid = length % 2 == 0 ? length / 2 + 1 : length / 2 + 2;

            int idx = mid;
            current = head;

            // Traverse to next to mid element
            while (idx > 1) {
                current = current.next;
                idx--;
            }
            // Reverse right side of the List and return head of the reversed list
            Node head1 = reverse(current);

            boolean isPalindrome = true;
            idx = length / 2;
            current = head;

            // Check reversed list(second half) matches with first half
            while (idx > 0) {

                if (current.data != head1.data) {
                    isPalindrome = false;
                    break;
                }
                current = current.next;
                head1 = head1.next;
                idx--;
            }

            return isPalindrome;
        }

        public static void main(String[] args) {

            Node head = new Node(1);
            head.next = new Node(2);
            head.next.next = new Node(3);
            head.next.next.next = new Node(2);
            head.next.next.next.next = new Node(1);

            System.out.println(isPalindrome(head));
        }
    }

Your inconsistent and unconventional indentation makes your code harder to read than it should be.

Although Java accepts multiple top-level classes in the same file as long as no more than one is public, it is poor form.  Every top-level class, interface, and enum should have its own file, whether public or not.  Among other things, this makes it much easier to find all the classes, both for you and for the compiler.

It is a bit questionable that you're willing to destroy the list in order to test whether it's palindromic, but inasmuch as you are willing to do so, you could perform your palindrome test more efficiently.  You would use two primary Node references, one that advances one position for every two that the other advances.  Using the trailing reference, you would reverse the front portion of the list as you go, so that when the lead reference finds the end of the list, the trailing reference is at the midpoint, with a reference to the reversed first half already in hand.  I'm sure you see how to proceed from there.

You could also consider using an auxiliary data structure to build the reverse half-list or an equivalent; in particular, pushing the nodes one by one onto a stack would have a comparable effect.  This could be done in a way that does not destroy the original list.

Or on the third hand, if you were willing to change to a doubly-linked list, and especially if you were willing to represent the list itself with a separate data structure that contained a reference to the tail in addition to the reference to the head, then the palindrome test could be performed much more simply.  You would just start at both ends and work towards the middle.  That doesn't modify the list, either.


AJAX communication with pure JS and PHP I'm looking for a good approach to AJAX communication using only pure JS and PHP. I would like it to be as safe as possible and compatible with most modern browsers and IE 10+. I did some research and found some code examples for writing a vanilla JS function to handle AJAX communication, but I was not sure how safe and up to date those were. I started coding my own solution based on what I've found. I need to receive and send data using AJAX. There will be a login system and also a text posting system.

index.html

<input type="checkbox" id="agreement"/> I agree with terms.

<input type="text" id="user"/> User

<input type="password" id="password"/> Password

<textarea id="text"></textarea> Text

<button id="submit" onclick="submit_form()">Submit</button>

ajax.php

if($_GET['action'] == 'submit_form')
{
    $agreement = $_POST['agreement'];
    $user = $_POST['user'];
    $password = $_POST['password'];
    $text = $_POST['text'];

    echo 'Data received:
';
    echo 'Agreement: '.$agreement.'
';
    echo 'User: '.$user.'
';
    echo 'Password: '.$password.'
';
    echo 'Text: '.$text.'
';
}

scrip.js

function submit_form()
{
    var ajax = new XMLHttpRequest();

    ajax.open('POST', 'ajax.php?action=submit_form', true);

    ajax.setRequestHeader('Content-type', 'application/x-www-form-urlencoded');

    var content = '';

    content = content + 'agreement=' + document.querySelector('#agreement').checked;
    content = content + '&user=' + document.querySelector('#user').value;
    content = content + '&password=' + document.querySelector('#password').value;
    content = content + '&text=' + document.querySelector('#text').value;

    ajax.send(content);

    ajax.onreadystatechange = function()
    {
        if (ajax.readyState == 4 && ajax.status == 200)
        {
            document.querySelector('#result').innerHTML = ajax.responseText;
        }
    }
}

Can you spot any problems with this approach? Are there better, simpler or more cross-browser compatible ways for doing the same? Are there any special concerns I should have when sending and receiving passwords?

var ajax = new XMLHttpRequest();

ajax.open('POST', 'ajax.php?action=submit_form', true);

You can replace XHR with fetch. While XHR is widely supported, fetch is easier to work with. It uses promises instead of callbacks, which means you can readily use async/await with it. It's inspired from jQuery's $.ajax(). So if you're familiar with that, fetch will just click.

var content = '';

content = content + 'agreement=' + document.querySelector('#agreement').checked;
content = content + '&user=' + document.querySelector('#user').value;
content = content + '&password=' + document.querySelector('#password').value;
content = content + '&text=' + document.querySelector('#text').value;

Do not build query strings manually. The issue is that in query strings, certain characters must be escaped correctly for it to be valid. Otherwise, I could stick characters in there which can make the query string invalid. Use URLSearchParams() to construct the query string.

<button id="submit" onclick="submit_form()">Submit</button>

Instead of using onclick on the button, use onsubmit on the form instead. This way, you can also catch someone submitting the form by pressing enter. Also, instead of inline event properties, use element.addEventListener(). This way, you can, among other things, add more than just one handler and avoid defining a global function for your event handler.

if($_GET['action'] == 'submit_form')
{
    $agreement = $_POST['agreement'];
    $user = $_POST['user'];
    $password = $_POST['password'];
    $text = $_POST['text'];

    echo 'Data received:
';
    echo 'Agreement: '.$agreement.'
';
    echo 'User: '.$user.'
';
    echo 'Password: '.$password.'
';
    echo 'Text: '.$text.'
';
}

Not exactly sure why you'd put action in a GET parameter while everything else is in POST. Use POST all the way if you intend to POST. Also, if I recall correctly, it's up to the server to parse GET parameters in a non-GET operation. The server may choose to ignore it. This might work in PHP, but it might not work on another platform.

Also, sanitize/escape the data before printing it on the page. This looks like debugging to me, so that's fine. But never trust user input, never print arbitrary values. This can easily be used in a reflected XSS, a security issue.


Beta Reducer in Haskell This is my first Haskell application, so any non-name-calling tips will be taken extremely well! It is just a beta reducer:

data Term = Var Int | Lam Term | App Term Term | Num Int | Add Term Term
            deriving (Show)

subs :: Int -> Term -> Term -> Term
subs depth value (Lam body) = Lam (subs (depth + 1) value body)
subs depth value (App abstraction body) = App (subs depth value abstraction) (subs depth value body)
subs depth value (Var bind)
    | bind == depth = inc depth value
    | otherwise     = Var (bind + depth)

inc :: Int -> Term -> Term
inc depth (Var a) = Var (a + depth)
inc depth (App a b) = App (inc depth a) (inc depth b)
inc depth (Lam a) = Lam (inc depth a)

red :: Term -> Term
red (Var a) = Var a
red (Lam a) = Lam (red a)
red (App (Lam body) value) = red (subs 0 value body)
red (App a b) = (App (red a) (red b))

val = red (App (Lam (Lam (Lam (App (Var 2) (Var 1))))) (Lam (Var 77)))

main = print val

I want to know what I could improve on the style or how I could make the code cleaner. I have mostly problems with indentation, I never know where I can (and should) make a newline without confusing the parser.

Comment: making this was ridiculously easier than I thought. The type system actually helped me a lot.

This looks reasonable and idiomatic to me.

I don't recognise the reduction strategy, though. Specifically, the line

red (App a b) = App (red a) (red b)

looks like you're not reducing to a normal form, and you might be reducing, say, one redex in the function and one in the argument. This may be intentional on your part, but if it isn't, maybe take a look here for a brief overview of reduction strategies, or here for more detail.

I haven't looked at de Bruijn indices in a while, so I wouldn't be able to promise that your implementation is correct. Nothing untoward comes to mind, though.

About the style–and this is really nitpicking–in the subs definition you might use simply a instead of the cumbersome abstraction. And I'd have indented the | one more space. (See? Nitpicking.)