Connection pool in Java Please review this connection pool. I tested it and it works but can we improve it from design or performance perspective?

public class ConnectionPool {

    private static final int MAX_SIZE=10;
    private static final BlockingQueue<Connection> bq;

    static{
        bq= new ArrayBlockingQueue<Connection>(MAX_SIZE);
        for(int i=0;i<MAX_SIZE;i++)
        {
            try {
                bq.add(makeConnection());
            } catch (SQLException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }

        System.out.println("total size:" + bq.size());
    }

    public static Connection getConnection() throws InterruptedException
    {
        System.out.println("size before getting connection"+ bq.size());
        Connection con=bq.take();
        System.out.println("size after getting connection"+ bq.size());
        return (con);
    }

    public static boolean releaseConnection(Connection con) throws InterruptedException
    {
        System.out.println("size before releasing connection"+ bq.size());
        boolean bool =bq.add(con);
        System.out.println("size after releasing connection"+ bq.size());
        return (bool);
    }

    public static Connection makeConnection() throws SQLException {
        Connection conn = null;
        Properties connectionProps = new Properties();
        connectionProps.put("user", "root");
        connectionProps.put("password", "java33");
        try {
            Class.forName("com.mysql.jdbc.Driver");
        } catch (ClassNotFoundException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
        conn = DriverManager.getConnection("jdbc:" + "mysql" + "://"
                + "localhost" + ":" + "3306" + "/test", connectionProps);

        System.out.println("Connected to database");
        return conn;
    }
}

Overall:

Are you sure that you want to keep a static number of connections? Why not keep a minimum set of connections and create new ones when needed?
ConnectionPool shouldn't have static fields as signalpillar said.
You don't check if the connections have timed out after a period of wait
Loggers are nice -> you don't necessary have to remove all useful debug output when going to production
You might want to have some sort of a timeout for the blocking queue depending on the application. Then you would poll rather than take from the queue.

Static initializer:

Is it sensible to catch exceptions per makeConnection - There's probably something wrong if any of them fail.
You don't check if there really are 10 connections in the pool after initialization.
What tt34 said.

Method getConnection:

return is not a method, parenthesis are not needed
do you really want to throw an InterruptedException from your ConnectionPool?

Method releaseConnection:

The same as getConnection

Method makeConnection:

this could be perhaps renamed as connect or openConnection
It would be better that the connection parameters are given as parameters rather hard-coding - you'll probably just testing at the moment but still
You should not just ignore the ClassNotFoundException in Class.forName(...): The program will fail again on the next instruction. Rethrow the exception or wrap it as a run-time exception if you do not want to show the ClassNotFoundException in the method signature
Rather than concatenating multiple strings, consider using String.format


Implement a string in reverse using pointers and recursion I'm trying to reverse a string using pointers and recursion, and I send it a char array. Is this the best way, or is there a better one?

int display_string_reversep(char* astring)
{
  char* achar = astring;
  if((*achar) != '\0')
    {
      char* x= achar;
      x++;
      display_string_reversep(x);
      cout<<(*achar);
    }
    return 0;
}

I'm also trying to do the same using smart pointers and that is proving to be more of a problem.

int display_string_reverseup(shared_ptr<char> astring)
{
  cout<<"\nEntering revup";
  shared_ptr<char> achar(astring);
  cout<<"\nAssigned achar";
  //if((*achar) != '\0')
  if(achar != nullptr)
    {
      cout<<"Forward String ";
      cout<<achar<<endl;
      if(*(achar.get()+1) != '\0')
    {
      shared_ptr<char> x(achar.get()+1);
      //x++;
      cout<<"Forward + 1 String ";
      cout<<x<<endl;
      //if ((*x) != '\0')
      display_string_reverseup(x);
    }
      cout<<(achar);
    }
    return 0;
}

General comments

This review might come across a little harsh, so I want to start out by saying that the code is overall pretty good. In particular, the first implementation is a completely reasonable implementation of a recursive approach to string reversal, and it's easy to read and follow.

Now, rather than comparing the two approaches (short version: the first one is vastly superior), I'm going to go over some design and style items.

Standard implementation

I assume this is a toy function. If not, you really should leverage the standard library instead:

std::string str("some string to be reversed");
std::copy(str.crbegin(), str.crend(), std::ostream_iterator<char>(std::cout));

Or if you want to use a C-style string:

const char str[] = "Some string to be reversed";
std::copy(std::reverse_iterator<const char*>(str + sizeof(str)),
          std::reverse_iterator<const char*>(str),
          std::ostream_iterator<char>(std::cout));

Side note: If you're importing individual names from std, great. If not, be very careful. using namespace std can be a dangerous option. Inside of implementations (i.e. source files) it's questionable; inside of headers, it's dangerous and harmful.

Bare pointer implementation

It seems you're always returning 0, and I can't think of a situation in which this function could fail (at least gracefully), so I wouldn't bother returning anything.

astring is a bit noisy (particularly the 'a'). I would just go with str. (string might be a questionable choice since it could shadow std::string in certain circumstances.)

achar is redundant (and like astring, I find the name a bit distracting -- instead, the typical option would be ch or chr, at least if it were actually a char and not a pointer to a char).

char* x and it's manipulation is unnecessary.

    display_string_reversep(str + 1);

I feel like the outputting could use a little more room to breathe:

std::cout << *str;

This is essentially opinion though.

Since you're not modifying the input, you should be taking a const char*.

It's typically good practice to keep manipulations and output separate. In other words, it would be more reusable if you had a function for reversing strings and then just output the reversed copy. Or, if you wanted to compromise, you could have it take an arbitrary std::ostream instead of being stuck on std::cout.

void display_string_reverse(const char* str, std::ostream& os);

(Note that the standard library version both keeps concerns separate and it avoids making a copy of the string -- iterators are a powerful thing.)

This can act as a decent intro-to-recursion thing, but otherwise, I would go with iteration. Recursion avoids an extra linear scan (if length isn't already known), so that's a bit nice, but the overhead is likely not worth it.

I'm not sure I get the name display_string_reversep. I would drop the p.

Recursive example

void display_string_reverse(const char* str, std::ostream& os) {
    if (str == nullptr || str[0] == '\0') { return; }
    display_string_reverse(str + 1);
    os << str[0];
}

(I tend to prefer str[0] over *str in situations like this -- either one is equally valid. str[0] just clicks a bit better with my brain.)

Iterative example

void display_string_reverse(const char* str, std::size_t len, std::ostream& os) {
    for (std::size_t idx = 1; idx <= len; ++idx) {
        os << str[len - idx];
    }
}

void display_string_reverse(const char* str, std::ostream& os) {
    display_string_revserse(str, std::strlen(str), os);
}

Typically when you operate on C style strings (or strings that don't carry around their own length in general), you want to allow the user to provide the length, and then for usability purposes, you can offer a strlen fallback (which I would probably not do in a real application -- I'd rather force the caller to know what he's doing). This allows for avoid an unnecessary linear scan when the length is already known.

Note that you could generalize this to use two pointers instead of a pointer and a length. Then you could extract out that pointer concept (via templating) and you'd find yourself getting eerily close to the mentality of the standard library based implementation :).

Note: you could also use for (char* chr = beg + len - 1; chr != str; --chr) { os << chr[0]; }.
Note: for (std::size_t idx = str + len - 1; idx >= 0; --idx) will underflow std::size_t and likely result in an ininite loop. If you were to go this route, care would have to be taken, or a signed type used (or use idx = str + len and then use idx - 1 inside of the loop instead of idx).

shared_ptr implementation

Correctness

I haven't tried to run it, but I'm going to guess that this segfaults. When you create a shared_ptr around a raw pointer, you're creating an ownership relation. When all of the shared pointers referencing the object go away, the object is cleaned up automatically. This is incredibly useful in certain situations, but you must be aware of the implications.

Your code is essentially doing this:

std::shared_ptr<char> ptr0(str);
std::shared_ptr<char> ptr1(str + 1);
std::shared_ptr<char> ptr2(str + 2);
// ...
std::shared_ptr<char> ptrn(str + n); // (str + strlen(str) - 1)

So what happens when all these shared_ptr go out of scope? Bad, bad things. In essence, str will be destructed n times. The problem is that the string isn't separate allocations of characters strung together. Instead it's either a C-string literal (so an automatically managed array of characters -- char[]), or it's a dynamically allocated array of characters. In the first situation, it should not be freed at all. In the second situation, it should be freed once and only once.

If you actually wanted to use smart pointers for this (and you shouldn't), you would have to use a single smart_ptr wrapping a dynamically allocated array of characters, and you would have to use either bare pointers or the smart pointer plus an offset.

In short, smart pointers are a powerful tool when they're required, but they are not only not required here, but, in my opinion, actively harmful. They convulute ownership semantics, introduce performance problems, and at the end of the day, a highly specialized smart pointer already exists for strings: std::string.

shared_ptr performance and semantics

Before you start passing around smart pointers, please go read this.

In short, you should only pass a shared_ptr by value if you're intending to share ownership with whatever you are passing it to. Since display_string_reversep has nothing to do with owning it's parameter, accepting a shared_ptr by value is incorrect both semantically and from a performance point of view. To make things worse a second shared_ptr is created.

shared_ptr is incredibly powerful for situations that truly require shared ownership semantics, but be mindful that its utility comes at a cost. Doing pretty much anything other than reading a shared pointer is very expensive compared to a bare pointer or a unique_ptr.

In this case, since there's not conditional logic of taking ownership of the shared pointer, you would still pass by a bare pointer.


Convert string array to object with true/false flags Currently a filter parameter of a GET request is used to designate items' categories that will be requested from database and displayed.

The parameter contains comma delimited string - each string id for each category: filter=complete,inwork,cancelled,ycancelled.

I want to get a Ruby object with corresponding attributes that will be true or false dependent on string id presence in filter parameter. Current implementation is as follows:

filter = params[:filter].split(',') rescue []
fopt = OpenStruct.new(Hash[%w(complete inwork cancelled ycancelled).map {|fo| [fo.to_sym, filter.include?(fo)]}])

Could this be rewritten in shorter way?

Code looks great to me. Here is a Rubocop-clean version with one change:

def fopt
  filter = (params[:filter] || '').split(',')
  OpenStruct.new(Hash[
    %w(complete inwork cancelled ycancelled).map do |fo|
      [fo.to_sym, filter.include?(fo)]
    end
  ])
end

Note the Rubocop rule to avoid rescue in its modifier form.


Javascript. Reverse order of words in string, but not special characters Hello I am hoping someone can provide me feedback on my solution to this prob. If there are additional edge cases I should consider etc. Thank you!
The question: "Given a sentence with a set of delimiters, reverse the order of the words in the sentence and keep the delimiters in place."
let isAlpha = (char) => {
  let pat = /[a-zA-z]/igm;     // match any alphabetic character
  if(char.match(pat)){
      return true
  } else {
      return false
  }
}

function reverse(s){

  let specialArr = []

  for(ltr of s){
    if(!isAlpha(ltr)){                        // pull out all special characters in order
      specialArr.push(ltr)
    }
  }

  let regCharArr = s.split(/[^a-z]/ig).reverse()     // use split to pull out all words separated by non alpha characters. then reverse

  let result = []                                    // init result array

  if(isAlpha(s[0])){
    result = regCharArr.flatMap((val, idx) => [val, specialArr[idx]])   //use flatmap to combine the two arrays...if origin array starts with letter, combine special array into reg array
  }

  if(!isAlpha(s[0])){
    result = specialArr.flatMap((val, idx) => [val, regCharArr[idx]])    // vice versa
  }

  for(let i = 0; i < result.length; i++){         // remove any undefineds
    if(result[i] === undefined){
      result.splice(i, 1)
    }
  }
  return result.join('')
}

// let sentence = "one|twotwo/three:four"  // four|three/twotwo:one
let sentence = "|brian/steve:chris|jon"     // |jon/chris:steve|brian

console.log(reverse(sentence))

```

The current implementation of isAlpha ...
let isAlpha = (char) => {
  // match any alphabetic character.
  let pat = /[a-zA-z]/igm;

  if(char.match(pat)){
    return true;
  } else {
    return false;
  }
};

... can be refactored into something as short as this ...
function isAlpha(char) {
  return (/[a-zA-z]/).test(char);
};

... or maybe even that ...
function isAlpha(char) {
  // \w matches any word character
  // and is equal to [a-zA-Z0-9_]
  return (/\w/).test(char);
};

Why is that?
isAlpha gets invoked from within reverse always by providing a single character. Thus the regex can be simplified from /[a-zA-z]/igm to /[a-zA-z]/. Since isAlpha is supposed to return a boolean value do not use match and do no not return either true or false separately but use RegExp.prototype.test instead which already provides the correct return value.
Thats all so far but I will continue looking through the code, since as of now I don't understand why one wants to test a string stepwise character by character.
There is an answer of mine, regarding your question at StackOverflow. Maybe you get some inspiration from an alternative approach.


Parsing and formatting var_export() string I have this script to format var_export's result string. The information in the output is good, but I need know if the code performance is good or if it can be improved.

https://wtools.io/php-sandbox/biDk

<?php
#Variable:

$Test                            = [];
$Test['Check1']                  = null;
$Test['Check2']                  = [];
$Test['Check2']['int']           = 20;
$Test['Check2']['float']         = 20.35;
$Test['Check2']['string']        = 'Hello World';
$Test['Check2']['bolean']        = true;
$Test['Check2']['array']         = [];
$Test['Check2']['array']['data'] = 'Array Text';
class Example {
    function foo_function() {
        return "Hello World! Object";
    }
}
$var_object                         = new Example;
$Test['Check2']['array']['object']  = $var_object;
$Test['Check2']['array']['object2'] = $var_object->foo_function();

#Script Type:
function myGetType($var) {
    if (is_null($var) OR $var == 'null' OR $var == 'NULL') {
        return "(NULL)";
    }

    if (is_array($var)) {
        return "array";
    }

    if (in_array($var, array("true", "false"), true)) {
        return "boolean";
    }

    if ((int) $var == $var && is_numeric($var)) {
        return "integer" . '(' . strlen($var) . ')';
    }

    if ((float) $var == $var && is_numeric($var)) {
        return "float" . '(' . strlen($var) . ')';
    }

    if (is_object($var)) {
        return "object";
    }

    if (is_resource($var)) {
        return "resource";
    }

    if (is_string($var)) {
        return "string" . '(' . strlen($var) . ')';
    }

    return "unknown";
}

#Script Analisis:
function VarExportFormat($Var) {
    $textvar    = '';
    $textvar    = var_export($Var, true);
    $textvar    = preg_replace("/^([ ]*)(.*)/m", '$1$1$2', $textvar);
    $textvarArr = preg_split("/\r\n|\n|\r/", $textvar);
    # Analisis del tipo.
    foreach ($textvarArr as $key => $value) {
        preg_match('~=>\\s(.*?),~', $value, $newvalue);
        if (!empty($newvalue)) {
            $newvalue[1]      = str_replace("'", "", $newvalue[1]);
            $typeval          = myGetType($newvalue[1]);
            $value            = str_replace("=> ", "=> " . $typeval . ': ', $value);
            $textvarArr[$key] = $value;
        }
    }
    $textvarArr = preg_replace(["/\s*array\s\($/", "/\)(,)?$/", "/\s=>\s$/"], [NULL, ']$1', ' => array ['], $textvarArr);
    $textvar    = join(PHP_EOL, array_filter(["array ["] + $textvarArr));
    if (substr($textvar, -1) == '[') {
        $textvar = str_replace("[", "[]", $textvar);
    }
    $textvar = str_replace("__set_state", "__set_state(object)", $textvar);

    $textvar = highlight_string("<?php \n#output of Variable:\n" . $textvar . ";\n?>", true);
    return $textvar;
}

echo VarExportFormat($Test);

Your script is like a var_export()-var_dump() hybrid with flare (I've not come across highlight_string() before -- pretty cool).

I ran some more tests with a few samples that I had laying around and noticed that your code is having some difficulties accessing the written "resource" branch.  I tested with fopen($filename, 'r'); the var_export() evaluation of a resource is NULL and var_dump() returns resource(5) of type (stream).  Because your script is founded on var_export(), it will fail to dignify the resource type data, I think you [sh/c]ould remove that from your battery of checks.
Miscellaneous gotchas include:

['a => key' => 'something']

and

['multiline' => 'line1
line2']

These will break your regex-based parsing -- of course, the former is going to be a much less common occurrence versus the latter.
You can safely remove some useless declarations:

$Test = []; and $textvar = '';

As a general rule, I prefer to never use OR or AND in my conditional statements as a matter of consistency and as a means to avoid fringe precedence issues.
preg_replace("/^([ ]*)(.*)/m", '$1$1$2', $textvar); This pattern/function is over-performing.  It doesn't make any sense to pick up the second capture group, just to put it back down unaltered.  In fact, there is no need for capture groups or character classes for this task.  I will also recommend that you use a consistent pattern delimiter for all patterns in your script. A tilde is a good/popular choice because it shouldn't interfere with any of your patterns and will help to prevent any accidental typos and escaping problems elsewhere.  To double the leading whitespace on each line, just use this (there's no use doubling zero whitespaces, so match one-or-more with +):

preg_replace('~^ +~m', '$0$0', $textvar);

$textvarArr = preg_split("/\r\n|\n|\r/", $textvar) is more succinctly written as:

$textvarArr = preg_split("~\R~", $textvar)

Whenever I see preg_match() called inside of a loop so that replacement actions can be performed, this is a classic indicator that preg_replace_callback() should be called.

$textvarArr = preg_split("~\R~", $textvar);
foreach ($textvarArr as $key => $value) {
    preg_match('~=>\\s(.*?),~', $value, $newvalue);
    if (!empty($newvalue)) {
        $newvalue[1]      = str_replace("'", "", $newvalue[1]);
        $typeval          = myGetType($newvalue[1]);
        $value            = str_replace("=> ", "=> " . $typeval . ': ', $value);
        $textvarArr[$key] = $value;
    }
}

can become:

$textvar = preg_replace_callback(
    "~ => \K\V+(?=,)~",
    function ($m) {
        return myGetType(str_replace("'", "", $m[0])) . ": {$m[0]}";
    },
    $textvar
);
// then proceed with something like...
$textvarArr = preg_replace(["/\s*array\s\($/", "/\)(,)?$/", "/\s=>\s$/"], [NULL, ']$1', ' => array ['], preg_split("~\R~", $textvar));

I've got to say that this looks a little dangerous...

if (substr($textvar, -1) == '[') {
    $textvar = str_replace("[", "[]", $textvar);
}

I mean, you are checking the final character, then doing a full text scan and replace operation.  This deserves a bit of double-checking.  Don't you only mean to close the final square brace? Maybe...

if (substr($textvar, -1) == '[') {
    $textvar .= "]";
}

I didn't get a chance to properly review (but it could probably use a little polish/simplifying):

$textvarArr = preg_replace(["/\s*array\s\($/", "/\)(,)?$/", "/\s=>\s$/"], [NULL, ']$1', ' => array ['], preg_split("~\R~", $textvar));
$textvar    = join(PHP_EOL, array_filter(["array ["] + $textvarArr));

I ran our outputs on a diffchecker program to be sure that my script returns the exact same output as yours.

Overall, an interesting exercise, good job.


HackerRank Array left rotation using Swift I solve this problem in Swift. Looking for feedback on my code or a better solution:

import Foundation

// parses line to return array of numbers in string format
func readIntegers() -> [String] {
    return readLine()!.componentsSeparatedByString(" ").map { $0 }
}

// left rotates the array by a given input
func leftRotation() -> String {
    let input = readIntegers()
    let array = readIntegers()
    var newArray = [String](count: Int(input[0])!, repeatedValue: "")
    let (count, rotations) = (Int(input[0])!, Int(input[1])!)

    for index in 0..<count {
        // newIndex after rotation
        let newIndex = index + rotations
        if newIndex < count  { // up until count
            newArray[index] = array[newIndex]
        } else { // loops back to beginning index of array
            newArray[index] = array[newIndex - count]
        }
    }
    // joins the array
    return newArray.joinWithSeparator(" ")
}

print(leftRotation())

I am not a swift programmer so i will not be able to give you exact advice on how to improve the quality of your code but rather the algorithm you used to solve this challenge and nitpicks i have with your solution.

Firstly lets look at the question itself.

It asks you to print out the new array. why are you creating it and turning it back into a string?

That considered, you can simply perform 2 for loops

1 for loop up until the "break" where the array has overflown, and a second for loop for the rest of the untouched array.

therfor we can simply print out the elements at location

d to n

and then the elements from

0 to d

This will shorten your code considerably and make it run a whole lot faster because no extra variables are needed and you arent doing array manipilation, its just a way to iterate over an array


Chat console using Ruby and Rust One of the exercises I like to program when learning a new language is a chat client for the console. I've programmed it in many languages (Perl, Python, C, C++, JavaScript, ...). One of the versions I like most for its terseness is Ruby:

#!/usr/bin/ruby
require 'socket'

s = TCPSocket.new(ARGV[0], ARGV[1].to_i)
nick = ARGV[2]

# send nick to server
s.puts(nick)

# input thread
kbd = Thread.new {
    STDIN.each_line { |line|
        s.puts("\033[31m" + nick + "> " + "\033[0m" + line)
    }
    s.close_write
}

# output thread
con = Thread.new {
    s.each_line { |line|
        STDOUT.puts(line)
    }
}

kbd.join
con.join

The initial Rust equivalent would be something similar to:

use std::os;
use std::io;
use std::io::{TcpStream, BufferedReader};

fn main() {
    let args = os::args();
    let mut s = TcpStream::connect(args[1], from_str(args[2]));

    // send nick to server
    nick = args[3];
    s.write_line(nick);

    // input thread
    spawn(proc() {
        for line in io::stdin().lines() {
            s.write_str(format!("\x1b[31m{}> \x1b[0m{}", nick, line));
        }
        s.close_write();
    });

    // output thread
    spawn(proc() {
        for line in BufferedReader::new(s).lines() {
            io::stdout().write_str(line);
        }
    });
}

which obviously has some compile errors. The corrected version is:

use std::os;
use std::io;
use std::io::{TcpStream, BufferedReader};

fn main() {
    let args = os::args();
    let mut s = TcpStream::connect(args[1].as_slice(),from_str(args[2].as_slice()).unwrap()).unwrap();

    // send nick to server
    s.write_line(args[3].as_slice()).unwrap();
    let nick = args[3].clone();
    let mut sin = s.clone();

    // input
    spawn(proc() {
        for line in io::stdin().lines() {
            sin.write_str(format!("\x1b[31m{}> \x1b[0m{}", nick, line.unwrap().as_slice()).as_slice()).unwrap();
        }
        sin.close_write().unwrap();
    });

    // output
    spawn(proc() {
        for line in BufferedReader::new(s).lines() {
            io::stdout().write_str(line.unwrap().as_slice()).unwrap();
        }
    });
}

That's too verbose, isn't it?  Perhaps it's due to the fact that the compiler is still beta (0.12-nightly)?  Can this terseness be simplified?

The verbosity in your code actually come from a number of different sources.

Error handling

There is a lot of calls to .unwrap in your code.

This is because rust is a low-level language and thus you need to be able to predictably be able to handle all errors your code might run into. Traditionally this have been done using exceptions (as one might in c++) or error values (as in c), but since these often come with their own set of problems, rust have chosen to do it differently. This has the disadvantage of having slightly more verbosity (in some cases), but more often giving rise to the desired solution without bugs.

Your way of handling it by calling .unwrap on every Option or Result you encounter. This is not a particularly smart solution, as it will simply cause the thread to fail on error, but if that is your desired outcome, then I guess it is okay.

The typically used ways of handling these errors are as far as I know either to use the try! macro or pattern matching.

str/String

Again, since rust is a low-level language without forced garbage collection, it needs to keep track of where its values are actually placed and how they are freed. In this case, this is apparent because of your need to do .as_slice in a lot of places.

This is because rust has at least two different types of strings. The first is String, which is a wrapper around a growable heap-vector of bytes. If you have such a value, you own it and is responsible for freeing it.

The second is a &str, which is just a pointer into a string. A function cannot free or alter this string. This is the type of string that most libraries take as arguments (to prevent cloning values all the time).

The extra verbosity comes from the fact that you need to convert the first kind into the second kind. It might be an idea to do this coercion automatically, but I have not heard about this being in the pipeline.

Extra cloning

Finally you needed to add a few calls to .clone. In this case, you send the values to a thread, which would make it unavailable in the original thread.

This is because you cannot easily access the same value from multiple places at the same time as you can in ruby. This is again because rust is a low-level language without forced garbage collection with the need to control error handling and memory allocations.

There are multiple ways of handling this problem, but the easiest (though not necessarily the best) is to simply call .clone before sending the value to a different thread.

Conclusion

While it is possible to make improvements, most of the verbosity actually comes from rusts increased need for control, and this is not going away in the future.


Lazily Load CSS and JS I have written this piece of JS and CSS loading code and I would like some advice on it. Anything some of the JavaScript gurus could possibly point out would be much appreciated. The code works, but I have not done extensive testing, because I am concerned about replacing functions in this manner.

A single JavaScript file containing JQuery as well as the below code will be included on all the pages. We write all the components in house (which means no Require JS) and keep them very modular separated into their own folder with the corresponding JS and CSS. You can imagine starting to use for instance a dropown, dialog and a datepicker on one page would require us to add 6 includes and this quite frankly is annoying, because I want the dependencies to resolve automatically and using JSP includes could possibly make multiple calls to the same resources.

Below is the source to load a single datepicker lazily:

;(function($){
    //All Lazily loaded components go here
    $.fn.datepicker = function(settings){
        console.log("This should only be displayed once");
        loadCSS("/res/component/datepicker/datepicker.css");
        var elem = this;
        return loadJS("/res/component/datepicker/datepicker.js",
                    function(){return elem.datepicker(settings)});//After Load Completion the $.fn.datepicker is replaced
                                                                  //by the proper working implementation, execute it and return it so we maintain the chain
    };
}(jQuery));

function loadCSS(absoluteUrl){
    if(loadCSS[absoluteUrl])
        return;//Css already loaded

    $('<link>')
      .appendTo('head')
      .attr({type : 'text/css', rel : 'stylesheet'})
      .attr('href', absoluteUrl);//Appending entire element doesn't load in IE, but setting the href in this manner does

    loadCSS[absoluteUrl] = true;//Memoize
}

function loadJS(absoluteUrl, onComplete){
    if(loadJS[absoluteUrl])
        return;//Script already loaded

    loadJS[absoluteUrl] = true;//Memoize

    var result;
    jQuery.ajax({
        async : false,//Synchronized because we need to maintain the JQuery chain
         type :'GET',
          url : absoluteUrl,
     dataType :'script',
      success : function(){
                    result = onComplete();
                }
    });

    return result;
}

No JavaScript guru, but I do have some thoughts on this.

As you yourself have point out in comments, this will be a problem when loadJS is called with the same URL a 2nd time before the 1st call has completed:

function loadJS(absoluteUrl, onComplete){
    if(loadJS[absoluteUrl])
        return;//Script already loaded

    loadJS[absoluteUrl] = true;//Memoize
    // ...

As loadJS[absoluteUrl] gets set to true immediately,
even before the URL is really loaded,
subsequent calls to the method will return early.
By moving the loadJS[absoluteUrl] = true; line further down in the method,
somewhere after the onComplete() call,
the problem will be mitigated.
In that case,
the worst thing that can happen is that the URL will be loaded twice,
which is not as bad as the rest of the code happily executing thinking that initialization has successfully completed when in fact it hasn't yet.

Update: ok maybe that's not the worst thing that can happen (as you pointed out in comments).
For example, if the same JS file is loaded by multiple loadJS calls in parallel,
the functions in the JS file will get overwritten.
I don't know how that works, and what happens if the function is being overwritten while it's also being executed at the same time.
To avoid such issues,
you would need to implement locking in loadJS to prevent subsequent calls while a URL is being loaded.

It seems a bad idea to use the same name for objects and functions,
for loadCSS and loadJS, for many reasons:

Can be confusing and lead to nasty bugs (see more on that in my update)
loadCSS and loadJS are not good names for objects that act essentially as a cache of url -> flag, indicating whether a URL was already loaded or not
Since CSS and JS URLs are distinct, a single cache will be enough

So how about renaming the cache, and sharing between CSS and JS:

function loadCSS(absoluteUrl) {
    if (alreadyLoadedUrls[absoluteUrl])
        return;  // Css already loaded

    // ...
}

function loadJS(absoluteUrl, onComplete) {
    if (alreadyLoadedUrls[absoluteUrl])
        return;  // Script already loaded

    // ...
}

Another thing,
loadJS returns different things when the URL was already loaded or not:

If already loaded, returns null
If called for the first time, it returns onComplete()

I think you should make the behavior consistent: either return null always, or onComplete() always.

Update

As you asked, a bit more info why not to use the same name for different objects.

As a general principle, programs should be as clear as possible.
Using the same name for different objects is really just asking for trouble,
for no apparent benefit, at least in this example.
The handling of and behavior of this feature in JavaScript can be confusing,
and may also be browser-dependent.

Take a look at the example snippets in this other answer.
Predicting the output of these examples is far from trivial:

(function (foo) {
  var foo; // doesn't affect the existing `foo` identifier
  return typeof foo;
  function foo () {}
})('string'); // yields "function"

(function (foo) {
  var foo = {};
  return typeof foo;
  function foo () {}
})('string'); // yields "object"!!

Having such elements in the code that force me to think hard would be a really unnecessary obstacle.

Even though in your code there is no such confusion,
that might creep in later as you (or somebody else) add more features later.
It also sets a bad example that others might follow.
Unless there is a really good reason to do this,
and real advantages,
don't call different things by the same name,
don't make me think,
keep things perfectly clear.


Speed up a while loop that conditionally creates a new variable in R I would like to speed up the following code in R. This is a loop to define new individuals (column newID) when the variable change is equal to 1. Any idea on how to improve the loop will be greatly appreciated.

Here is the code:

## Build the data frame
dat <- expand.grid(x = 1:1000, ID = as.character(seq(0, 4000, 1)))
dat$change <- 0
dat[which(dat$x == 1), c("change")] <- 1
dat[which(dat$x == 300), c("change")] <- 1
dat[which(dat$x == 700), c("change")] <- 1
dat[1, c("change")] <- 0

## Add a column "newID"
dat$newID <- NA
index <- c(1, which(dat$change == 1), nrow(dat))
j <- 1
i <- 1

system.time(while (j < length(index)){
  print(paste(j, "/", length(index), sep = " "))
  i <- ifelse((j > 1) && (dat[index[j], c("ID")] != dat[index[j - 1], c("ID")]), 1, i)
  ## print(i)
  if(j == length(index) - 1){
    dat[seq(index[j], index[j + 1], by = 1), c("newID")] <- paste("Ind ", dat[index[j], c("ID")], "|", i, sep="")
  } else{
    dat[seq(index[j], index[j + 1] - 1, by = 1), c("newID")] <- paste("Ind ", dat[index[j], c("ID")], "|", i, sep="")
  }
  j <- j + 1
  i <- i + 1
})
## summary(dat)

Here is an example:

The input data frame has 3 columns. In particular, ID is the ID number of each individual and change takes the value of 1 when the individual is renewed.

    x ID   change
1   1  0      0
2   2  0      0
3   3  0      0
4   4  0      0
5   5  0      1
6   6  0      0
7   7  0      1
8   8  0      0
9   9  0      0
10 10  0      0
11  1  1      1
12  2  1      0
13  3  1      0
14  4  1      0
15  5  1      1
16  6  1      0
17  7  1      1
18  8  1      0
19  9  1      0
20 10  1      0
21  1  2      1
22  2  2      0
23  3  2      0
24  4  2      0
25  5  2      1
26  6  2      0
27  7  2      1
28  8  2      0
29  9  2      0
30 10  2      0

The variable newID is created as follows:
When change is equal to 1, newID takes the old ID number and the increment value. Thus, in the example, the expected result is:

    x  ID  change newID
1   1  0      0 Ind 0|1
2   2  0      0 Ind 0|1
3   3  0      0 Ind 0|1
4   4  0      0 Ind 0|1
5   5  0      1 Ind 0|2
6   6  0      0 Ind 0|2
7   7  0      1 Ind 0|3
8   8  0      0 Ind 0|3
9   9  0      0 Ind 0|3
10 10  0      0 Ind 0|3
11  1  1      1 Ind 1|1
12  2  1      0 Ind 1|1
13  3  1      0 Ind 1|1
14  4  1      0 Ind 1|1
15  5  1      1 Ind 1|2
16  6  1      0 Ind 1|2
17  7  1      1 Ind 1|3
18  8  1      0 Ind 1|3
19  9  1      0 Ind 1|3
20 10  1      0 Ind 1|3
21  1  2      1 Ind 2|1
22  2  2      0 Ind 2|1
23  3  2      0 Ind 2|1
24  4  2      0 Ind 2|1
25  5  2      1 Ind 2|2
26  6  2      0 Ind 2|2
27  7  2      1 Ind 2|3
28  8  2      0 Ind 2|3
29  9  2      0 Ind 2|3
30 10  2      0 Ind 2|3

Do not use explicit loops unless you absolutely have to

Many functions in R are vectorized and your code will be much faster if you leverage this instead of writing your own loops.

For example, you can compute the first part of your newID with a single paste() call:

dat$newID <- paste("Ind", dat$ID)

    x ID change newID
1   1  0      0 Ind 0
2   2  0      0 Ind 0
3   3  0      0 Ind 0
4   4  0      0 Ind 0
5   5  0      1 Ind 0
6   6  0      0 Ind 0
7   7  0      1 Ind 0
8   8  0      0 Ind 0
9   9  0      0 Ind 0
10 10  0      0 Ind 0
11  1  1      1 Ind 1
12  2  1      0 Ind 1
13  3  1      0 Ind 1
14  4  1      0 Ind 1
15  5  1      1 Ind 1
16  6  1      0 Ind 1
17  7  1      1 Ind 1
18  8  1      0 Ind 1
19  9  1      0 Ind 1
...

The second part of your newID is simply the cumulative sum of change

The most tricky part here is to reset the counter each time the ID changes. A way to do this is to use the function by, which will execute a given function on a group of rows depending on the values of a grouping variable (here ID):

by(dat, dat$ID, function(x) {
  cumsum(x$change)
})

dat$ID: 0
 [1] 0 0 0 0 1 1 2 2 2 2
---------------------------------------------------------------------------------------------
dat$ID: 1
 [1] 1 1 1 1 2 2 3 3 3 3
---------------------------------------------------------------------------------------------
dat$ID: 2
 [1] 1 1 1 1 2 2 3 3 3 3

The two issues here are:

by returns a list so we have to use unlist() before trying to put the result in a data.frame column

dat$newID <- unlist(by(dat, dat$ID, function(x) {
  cumsum(x$change)
}))

because change doesn't start with 1, the values for the first ID are shifted by one. We can fix this by changing the first value manually.

Put everything together

dat$change[1] <- 1

dat$newID <- unlist(by(dat, dat$ID, function(x) {
  cumsum(x$change)
}))

dat$newID <- paste0("Ind ", dat$ID, "|", dat$newID)

And you get exactly the output you were asking for, without any explicit for loops!


Counting the number of "on" bits in an int Here is some code I wrote as a solution to Programming Exercise 15.3 in Stephen Prata's C Primer Plus, 6th Edition. It is probably worth pointing out that the title of the chapter is Bit Fiddling. The question asks:

  Write a function that takes an int argument and returns the number
  of "on" bits in the argument. Test the function in a program.

My code seems to work for a variety of test-cases. But bit-fiddling seems finicky, and I was wondering if anyone could see any mistakes here. I would also be very interested to hear about ways to improve this code, or better approaches (e. g., more concise, or more efficient) to the problem.

The code that I have forms a bitmask from an unsigned int with only the bit corresponding to the sign bit of the input int set to "on", and then proceeds to check the bits of the input int against the bitmask, moving the test bit towards the lower order bits at each iteration until the last bit has been tested.

Since first posting this question, I have begun to feel uneasy about the expression:

(b_mask & num)

as b_mask is an unsigned int and num is an int. It usually seems like a bad idea to mix signed and unsigned types in the same expression. Is the result of this operation reliable? Any thoughts on this particular issue would be welcome.

#include <stdio.h>
#include <limits.h>

int on_bits(int num);

int main(void)
{
    int input;

    printf("Enter an int value ('q' to quit): ");
    while (scanf("%d", &input) == 1) {
        printf("%d\n", on_bits(input));
        printf("Enter an int value ('q' to quit): ");
    }

    return 0;
}

int on_bits(int num)
{
    int res = 0;
    unsigned b_mask = 0x1;

    /* Move to sign bit */
    for (int i = 1; i < (sizeof(num) * CHAR_BIT); i++)
        b_mask <<= 1;

    /* Check bits */
    while (b_mask > 0) {
        if (b_mask & num)
            ++res;
        b_mask >>= 1;
    }

    return res;
}

Here is a sample interaction:

Enter an int value ('q' to quit): 255
8
Enter an int value ('q' to quit): -255
25
Enter an int value ('q' to quit): 2048
1
Enter an int value ('q' to quit): 2047
11
Enter an int value ('q' to quit): -2047
22
Enter an int value ('q' to quit): 4095
12
Enter an int value ('q' to quit): -4095
21
Enter an int value ('q' to quit): -1234567
22
Enter an int value ('q' to quit): -987654321
16
Enter an int value ('q' to quit): q

The current implementation

Your implementation is correct, and takes \$O(n)\$ time, where \$n\$ is the number of bits in an int.  One thing you can improve is the initial setting of b_mask.  Currently, you use a loop, but it can be done like this:

unsigned int b_mask = 1u << (sizeof(b_mask)*CHAR_BIT - 1);

Actually, there are two other strategies you can use as well:

Start b_mask at 1 and left shift it instead of right shift it.
Get rid of b_mask and shift the number instead.

For an example of #2:

int on_bits(unsigned int num)
{
    int res = 0;

    while (num != 0) {
        res += (num & 0x1);
        num >>= 1;
    }

    return res;
}

Other implementations

There are a wide variety of ways to count bits.  You should read the wikipedia page on Hamming weight for starters.  I'll briefly mention some here:

Removing one bit per loop

There is a bit trick that removes the lowest set bit.  So instead of looping \$n\$ times (e.g. 32), you can only loop \$m\$ times, where \$m\$ is the number of bits actually set. So if there is only 1 bit set, you only have to loop once.  The code looks like this:

int on_bits(unsigned int num)
{
    int res = 0;

    while (num != 0) {
        num &= num - 1;
        res++;
    }

    return res;
}

Using hardware instruction

Many CPUs have a dedicated instruction for counting bits (e.g. popcnt for X86).  Your compiler probably has a way of accessing this instruction (e.g. __builtin_popcount() for GNU.  This will be the fastest of all implementations but will be hardware and compiler dependent.

int on_bits(unsigned int num)
{
    return __builtin_popcount(num);
}

Using a lookup table

Suppose you created a lookup table with 256 entries that contained the number of bits set in each byte value 0..255.  Then your code would need to loop only 4 times instead of 32:

static const uint8_t bitsInByte[256] = { /* Prefilled in */ };
int on_bits(unsigned int num)
{
    int res = 0;

    while (num != 0) {
        res += bitsInByte[num & 0xff];
        num >>= 8;
    }

    return res;
}

Using parallelism and bit tricks

This is described in the wikipedia article in depth.  Here I give the 32-bit version instead of the 64-bit one:

// Assumes int is 32 bits
int on_bits(unsigned int num)
{
    num -= ((num >> 1) & 0x55555555);
    num  = (num & 0x33333333) + ((num >> 2) & 0x33333333);
    return (((num + (num >> 4)) & 0xF0F0F0F) * 0x1010101) >> 24;
}

One's complement

According to later comments by the OP, being able to handle one's complement is important, and the input argument should be int instead of unsigned int.  You can still use all of the functions above, if you first prepare the input argument by stripping its sign bit, like this:

int on_bits(int num)
{
    int signBit = 1 << (sizeof(int)*CHAR_BIT - 1);
    int res     = (num & signBit) != 0;

    num &= ~signBit;

    // Now, num has its sign bit stripped so it can be right shifted.
    // Res is 1 if the sign bit was set, or 0 if it was not.

    // Do the rest here...
}


Find SuburbName from latlong or location I have create one class and interface for finding SuburbName based on Location or LatLong coordinates.

My interface

public interface IGeoCodeService
{
    string GetSuburbName(double latitude, double longitude);
    string GetSuburbName(string location);
    string GetGoogleStaticMapImageUrl(string location);
}

My Class

public class GeoCodeService : IGeoCodeService
{
    private static readonly string GoogleApiKey = "*****MyAPI KEY*****";
    private static readonly string GoogleStaticMapUrl = "http://maps.googleapis.com/maps/api/staticmap";

    public string GetSuburbName(string location)
    {
        if (!string.IsNullOrEmpty(location))
        {
            using (var clientService = new HttpClientService<GeoCodeResponse>("https://maps.googleapis.com"))
            {
                var param = new Dictionary<string, string>();
                param.Add("address", location);
                param.Add("components", "country:AU");
                param.Add("result_type", "locality");
                param.Add("key", GoogleApiKey);
                var apiResult = clientService.GetAPI("maps/api/geocode/json", param);
                return FindSuburbName(apiResult);
            }
        }
        return string.Empty;
    }

    public string GetSuburbName(double latitude, double longitude)
    {
        using (var clientService = new HttpClientService<GeoCodeResponse>("https://maps.googleapis.com"))
        {
            var param = new Dictionary<string, string>();
            param.Add("latlng", $"{latitude},{longitude}");
            param.Add("result_type", "locality");
            param.Add("key", GoogleApiKey);

            var apiResult = clientService.GetAPI("maps/api/geocode/json", param);
            return FindSuburbName(apiResult);
        }
    }

    private string FindSuburbName(GeoCodeResponse result)
    {
        var response = string.Empty;
        if (result != null || result.results != null && result.results.Count > 0)
        {
            var addressObj = result.results.FirstOrDefault();
            if (addressObj != null)
            {
                var component = addressObj.AddressComponents.Where(x => x.Types.Contains("locality")).FirstOrDefault();
                if (component != null)
                {
                    return component.LongName;
                }
            }
        }
        return response;
    }

    public string GetGoogleStaticMapImageUrl(string location)
    {
        return $"{GoogleStaticMapUrl}?size=350x200&markers={location}";
    }
}

And lastly the GeoCodeResponse class to store response from the google api call.

public class GeoCodeResponse
{
    [JsonProperty("results")]
    public List<Results> results { get; set; }
    [JsonProperty("status")]
    public string status { get; set; }
}

public class Results
{
    public Results()
    {
        AddressComponents = new List<AddressComponent>();
    }
    [JsonProperty("address_components")]
    public List<AddressComponent> AddressComponents { get; set; }
}
public class AddressComponent
{
    [JsonProperty("long_name")]
    public string LongName { get; set; }
    [JsonProperty("short_name")]
    public string ShortName { get; set; }
    [JsonProperty("types")]
    public string[] Types { get; set; }
}

Can you please suggest me if what I have implemented is right or require any change. Your help will improve my code quality.

The HttpClientService class is used for call api

There are lots of nice things in your code for a beginner, congratulations.
Configuration file
Right now, you obfuscated your API key so that we don't have it, that's logical.
But it shouldn't be hardcoded in your code either. This is a bad practice for multiple reasons

What if I want to use your code? I'd be using you API key, this is danngerous. I should be able to set mine without having to recompile
Your API key would be stored in your source control (if you have one) and could be accessible to anyone decompiling your code. If there are costs related to Maps API, things could go wrong very quickly.

An API key should always be in some sort of configuration file. Either an app.config, web.config or some other external configuration manager.
Code repetition
Between the two overrides of GetSuburbName, there are parts of code that are duplicated. You did a good job of extracting FindSuburbName(GeoCodeResponse result) as a private method though.
I would go a little further. The only difference between the two methods is the parameters list so, as an example, I'd change :
if (!string.IsNullOrEmpty(location))
{
    using (var clientService = new HttpClientService<GeoCodeResponse>("https://maps.googleapis.com"))
    {
        var param = new Dictionary<string, string>();
        param.Add("address", location);
        param.Add("components", "country:AU");
        param.Add("result_type", "locality");
        param.Add("key", GoogleApiKey);
        var apiResult = clientService.GetAPI("maps/api/geocode/json", param);
        return FindSuburbName(apiResult);
    }
}
return string.Empty;

To :
if (!string.IsNullOrEmpty(location))
{
    var param = new Dictionary<string, string>();
    param.Add("address", location);
    param.Add("components", "country:AU");
    param.Add("result_type", "locality");
    param.Add("key", GoogleApiKey);

    return FindSuburbName(param);
}
return string.Empty;

Then, you could have your API call in the common method.
C# tips
You can use the Dictionary Initializer if you have a recent version of C#.
As an example :
var param = new Dictionary<string, string>();
param.Add("address", location);

becomes :
var param = new Dictionary<string, string>
{
    {"address", location},
    //etc
}

Regarding this line :
var component = addressObj.AddressComponents.Where(x => x.Types.Contains("locality")).FirstOrDefault();

There is an override for FirstOrDefaut that takes a Predicate (a condition), which means your code could be :
var component = addressObj.AddressComponents.FirstOrDefault(x => x.Types.Contains("locality"));

@FabienH. @FabianH. already pointed out the other points I wanted to cover regarding C#.


Android RecyclerView holding rows of razzles I have a RecyclerView that works and everything, and this is how I coded it:

public class RazzleRecyclerViewAdapter extends RecyclerView.Adapter<RazzleRecyclerViewAdapter.RecyclerViewHolder> {
    private List<Razzle> mRazzleList;
    private Context mContext;
    private SelectRazzleRowListener mSelectRazzleRowListener;

    public RazzleRecyclerViewAdapter(
            Context context,
            List<Razzle> razzleList,
            SelectRazzleRowListener selectRazzleRowListener) {
        mContext = context;
        mRazzleList = razzleList;
        mSelectRazzleRowListener = selectRazzleRowListener;
    }

    @Override
    public RecyclerViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
        return new RecyclerViewHolder(LayoutInflater.from(parent.getContext())
                .inflate(R.layout.item_razzle_row, parent, false));
    }

    @Override
    public void onBindViewHolder(RecyclerViewHolder holder, int position) {
        holder.bindRow(mRazzleList.get(position), position);
    }

    @Override
    public int getItemCount() {
        return mRazzleList.size();
    }

    public class RecyclerViewHolder extends RecyclerView.ViewHolder {
        public int thisPosition;
        public Razzle mThisRazzle;
        public TextView razzleNameTextView;

        public RecyclerViewHolder(View itemView) {
            super(itemView);
            razzleNameTextView = (TextView) itemView.findViewById(R.id.item_razzle_row_textview_razzle_name);

            itemView.setOnClickListener(new View.OnClickListener() {
                @Override
                public void onClick(View v) {
                    mSelectRazzleRowListener.launchRazzleDetailsActivity(mThisRazzle, thisPosition);
                }
            });
        }

        public void bindRow(final Razzle razzle, int position) {
            mThisRazzle = razzle;
            thisPosition = position;
            razzleNameTextView.setText(razzle.getRazzleName());
        }

    }
}

How can I code this better? Can I make it clearer? Am I following good conventions? Can I restructure it so it's easier to work with? In particular, the ViewHolder stuff?

Remove mContext member. You're not using it anywhere.
Your code tells us that onClickListener will not change during life of Adapter object. Please make ViewHolder class static to not have implicit reference to Adapter and pass OnClickListener as a constructor parameter to ViewHolder.
ViewHolder class and it's bindRow method can be private.

Point 2 explanation :

You are passing SelectRazzleRowListener to Adapter class via constructor and don't provide a setter method to change that field during lifetime of Adapter object - so once adapter will be created with that listener he uses it always. And that's ok. But in that situation you can do a little tune to ViewHolder class. For now it's non-static inner class. By default if inner class is not static it contains implicit reference to parent class object (Adapter in this case). It may be risky in terms of memory leaks and it's good to be aware of that relationship (it's easy to test - try to create ViewHolder object somewhere outside adapter class in current state - without static - you won't be able to). It's also a reason why you can access Listener object from adapter inside ViewHolder class.
In my opinion you should make ViewHolder class static (so public static class RecyclerViewHolder .... Then you need to pass SelectRazzleRowListener object to ViewHolder by defining a field in ViewHolder class and constructor parameter. Then Adapter will pass Selection listener to ViewHolder during onCreateViewHolder.


Python3 Stack implementation with List built-in So, inspired by this other code review, I wanted to see if I could implement a very basic stack without the linked-list approach taken by the OP in their implementation.

To that end, I came up with the following Stack class.  I wrote it for Python 3, but it actually works as-is in Python 2 as well:

class Stack:
    _stack = []

    def __init__(self):
        self._stack = []

    @property
    def size(self):
        return len(self._stack)

    @property
    def count(self):
        return self.size

    def __sizeof__(self):
        return self.size

    def pop(self):
        if self.size == 0:
            return "Cannot pop from an empty stack."

        item = self._stack[self.size - 1]
        self._stack.remove(item)
        return item

    def peek(self):
        if self.size == 0:
            return "Cannot peek into an empty stack."

        return self._stack[self.size - 1]

    def push(self, item):
        self._stack.append(item)

    def __iter__(self):
        return iter(self._stack)

I'm probably missing some key parts of what one would want from a Stack, but any improvement is welcome.  Literally any improvement.

Note there were a couple things I did intentionally:

I intentionally initialize the _stack attribute as an empty list both in the class itself and in __init__.  For some reason PyCharm complains if the attribute isn't already part of the class when being worked with in __init__, so this is more or less to get PyCharm to stop yelling at me.
I intentionally have __sizeof__ declared as referring to the size property function.  This lets len(StackObject) work if someone doesn't want to do stack.size.

With these in mind, feel free to tell me what you think, or point out any mistakes I've made.  (I'm not an expert in data structures, so I may have gotten some points messed up a little).

By far the biggest problem this code has is error handling. When popping or peeking from an empty stack this returns a string. this is really broken. For an example of why, suppose I add the string "Cannot peek into an empty stack." to the stack and then try to see what the top element is. I won't know if the answer I got was because the stack was empty, or because that string was in the stack. The solution to this is to raise an IndexError when the stack is empty. I also think that you shouldn't define either size or count, as the pythonic way of getting somethings length is to call len(x). You might also be interested to know that python lists have a pop function already, so you could just use that for your pop


Low-level methods with temperature conversion program This is my first program to convert temperatures. I have a very basic knowledge of Python and I tried using only what I already knew to make the program. What are some improvements I can make, or how can I re-create this in an easier way?

temp_string = raw_input("What is the Temperature: ")

x = int(temp_string)

print x

temp_type = raw_input("Would you like to convert to Celsius or Fahrenheit?: ")

input_type = str(temp_type)

def convert_fahrenheit(x):
    x = (x - 32) * 0.556
    print x

def convert_celsius(x):
    x = (x * 1.8) + 32
    print x

if input_type == str("c"):
    convert_fahrenheit(x)
elif input_type == str("celsius"):
    convert_fahrenheit(x)
elif input_type == str("Celsius"):
    convert_fahrenheit(x)
elif input_type == str("C"):
    convert_celsius(x)
elif input_type == str("f"):
    convert_celsius(x)
elif input_type == str("fahrenheit"):
    convert_celsius(x)
elif input_type == str("Fahrenheit"):
    convert_celsius(x)
elif input_type == str("F"):
    convert_celsius(x)
else:
    print "You have not entered a valid conversion"

Overall I think the most important thing to improve here is code clarity.

You have some code like this:

if input_type == str("c"):

"c" is already a string so doing str("c") is redundant. It's definitely easier and clearer to just write:

if input_type == "c":

There is also a lot of boilerplate to call the required functions with that big if-else block. Instead I would use a dictionary here to do this:

temp_input = int(raw_input("What is the Temperature: "))

input_type = str(raw_input("Would you like to convert to Celsius or Fahrenheit?: "))

conversion_dispatcher = {
    "c": convert_fahrenheit,
    "C": convert_fahrenheit,
    "celsius": convert_fahrenheit,
    "Celsius": convert_fahrenheit,
    "f": convert_celsius,
    "F": convert_celsius,
    "fahrenheit": convert_celsius,
    "Fahrenheit": convert_celsius,
}

try:
    conversion_dispatcher[input_type](temp_input)
except KeyError:
    print("You have not entered a valid conversion")

This way you end up with a lot less boilerplate overall and it's much easier to make changes if you find a problem.

Also I would change the conversion functions to return a value and I would print that value afterwards:

def convert_fahrenheit(x):
    x = (x - 32) * 0.556
    return x

def convert_celsius(x):
    x = (x * 1.8) + 32
    return x

try:
    new_value = conversion_dispatcher[input_type](temp_input)
    print(new_value)
except KeyError:
    print("You have not entered a valid conversion")

The reason for this is because it more clearly separates out the logic from the display code. The makes those conversion functions more reusable as you can now use them in situations where you don't just want to print to the console.

You might also have noticed that I have changed your print syntax to the new print function syntax. This is because the old print statement is going away in newer version of python (python 3.x) and you might as well get used to writing it the other way now. If this causes an error in your version of python you might need to use from __future__ import print_function at the top of your file.

Now that we are done with the Python related stuff it's probably good to improve some other elements of the code. Given the function name isn't completely unambiguous a docstring explaining which way the units conversion is being applied in the conversion functions would help future readers of your code understand what's going on better. This is especially the case because your printout at the beginning suggests different behavior to the code you wrote.

For example:

def convert_to_celsius(x):
    """Converts a temperature in Celsius to Fahrenheit"""
    x = (x * 1.8) + 32
    return x


Get ruby Hash#each to return a built hash I'm golfing some code while waiting for a coworker, and I shortened a function to this:

def get_data(dir)
  newest = get_most_recent_file_numbers(dir)

  data = {}
  $file_prefixes.each do |type, file_prefix|
    file = File.open("#{dir}/#{file_prefix}_#{newest[type]}.csv")
    file.readline
    data[type] = file.readlines
  end

  data
end

I feel like there must be some method to do away with that last line of just data. How can I get $file_prefixes.each to return data?

If it matters, $file_prefixes looks like this:

$file_prefixes = {
  :fixed_deal   => "PC2MMS_CMSS_DEALS_",
  :fixed_offer  => "PC2MMS_CMSS_OFFERS_",
  :mobile_deal  => "PC2MMS_NGBSS_DEALS_",
  :mobile_offer => "PC2MMS_NGBSS_OFFERS_"
}

and newest is pretty much identical.

My target code might look something like this:

def get_data(dir)
  newest = get_most_recent_file_numbers(dir)

  data = {} # might even be able to get rid of this with clever use of Hash.new
  data = $file_prefixes.each do |type, file_prefix|
    file = File.open("#{dir}/#{file_prefix}_#{newest[type]}.csv")
    file.readline
    ???
  end
end

Disclaimer: I'm aware I could golf more by using single letter names, but I'm just trying to get it succinct and still readable

Some notes:

Avoid the pattern init + each + update. Use map or other functional methods.
In Ruby >= 2.1.0 you have Array#to_h. Use Hash[...] in older Rubies.
Use File.join

I'd write:

def get_data(dir)
  newest = get_most_recent_file_numbers(dir)
  $file_prefixes.map do |type, file_prefix|
    path = File.join(dir, file_prefix + "_" + newest[type] + ".csv")
    [type, File.readlines(path)]
  end.to_h
end


Replace sequence of strings in binary file I have searched for method to find and replace strings sequence in binary files without any luck. The main requirement was that method should not load all file in memory but rather use chunks. I am new in c# and code may look not "polished" but it works fine. Maybe someone will have ideas how this code could be improved or does it have any flaws? p.s. Thanks goes to Jon Skeet for idea.

public static void ReplaceTextInFile(string inFile, string find, string replace)
{

    if (find.Length!=replace.Length) throw new ArgumentException("The lenght of find and replace strings must match!");

    const int chunkPrefix = 1024*10;
    var findBytes = GetBytes(find);
    var replaceBytes = GetBytes(replace);
    long chunkSize = findBytes.Length * chunkPrefix;
    var f = new FileInfo(inFile);
    if (f.Length < chunkSize)
        chunkSize = f.Length;

    var readBuffer = new byte[chunkSize];

    using (Stream stream = File.Open(inFile, FileMode.Open))
    {
        int bytesRead;
        while ((bytesRead=stream.Read(readBuffer, 0, readBuffer.Length)) != 0)
        {
            var replacePositions = new List<int>();
            var matches = SearchBytePattern(findBytes, readBuffer, ref replacePositions);
            if (matches != 0)
                foreach (var replacePosition in replacePositions)
                {
                    var originalPosition = stream.Position;
                    stream.Position = originalPosition - bytesRead + replacePosition;
                    stream.Write(replaceBytes, 0, replaceBytes.Length);
                    stream.Position = originalPosition;
                }

            if (stream.Length == stream.Position) break;
            var moveBackByHalf = stream.Position - (bytesRead / 2);
            stream.Position = moveBackByHalf;
        }

    }

}

static public int SearchBytePattern(byte[] pattern, byte[] bytes, ref List<int> position)
{
    int matches = 0;
    for (int i = 0; i < bytes.Length; i++)
    {
        if (pattern[0] == bytes[i] && bytes.Length - i >= pattern.Length)
        {
            bool ismatch = true;
            for (int j = 1; j < pattern.Length && ismatch == true; j++)
            {
                if (bytes[i + j] != pattern[j])
                    ismatch = false;
            }
            if (ismatch)
            {
                position.Add(i);
                matches++;
                i += pattern.Length - 1;
            }
        }
    }
    return matches;
}

public static byte[] GetBytes(string text)
{
    return Encoding.UTF8.GetBytes(text);
}

usage

 ReplaceTextInFile(@"MyFile.bin", "Text to replace", "New Text! Test!");

A few issues with the code:

You're comparing the string length for both, but then replacing the bytes. In UTF-8 encoding, as you're using, it's possible that the two will be different: if find = "aeiou" and replace = "áéíóú" you'll have findBytes.Length == 5, and replaceBytes.Length == 10
You don't need to pass the position parameter by reference to SearchBytePattern, since you're not changing the reference, only calling methods on it.
On SearchBytePattern, you don't need the outermost loop to go all the way to bytes.Length, it only needs to go to bytes.Length - pattern.Length + 1 (and that would simplify the inner "if"
stream.Read doesn't necessarily return the count of bytes you asked for - it can return less than that. Your code should be ready to handle that situation.


Reading and returning the HTML content from a specified URL I wanted to create a simple function that can read and return the HTML content from a specified URL. This is what reading here and there lead me to:

from socket import timeout
from urllib.request import Request, urlopen
from urllib.error import URLError, HTTPError

def get_html_content(url, max_attempt = 3):
    req = Request(url, headers={'User-Agent': 'Mozilla/5.0'})
    content = ""
    attempt = 1
    while True:
        try:
            html_page = urlopen(req, timeout=10)
            content = html_page.read()
        except (HTTPError, URLError, timeout) as e:
            if isinstance(e, HTTPError):
                print("The server couldn\'t fulfill the request....attempt %d/%d" % (attempt, max_attempt))
                print('Error code: ', e.code)
            if isinstance(e, URLError):
                print("We failed to reach a server....attempt %d/%d" % (attempt, max_attempt))
                print('Reason: ', e.reason)
            if isinstance(e, timeout):
                print('timeout...attempt %d/%d' % (attempt, max_attempt))
            attempt += 1
            if attempt > max_attempt:
                break
            continue
        else:
            break

    return content

I would use this function to parse the content of many URLs. For if content = "", I would raise a random exception after writing to some file whatever I had already successfully gathered.

There are a couple of minor technical issues:

The content variable is unnecessary, because you can simply return html_page.read() directly. (And you could as well return urlopen(req, timeout=10).read() directly...) When the max attempts are reached, you could return "" instead of relying on that content was initialized to "". And how about returning None? Then you could simply omit the return statement to the same effect.
In the exception handling, there are multiple if statements with conditions that are mutually exclusive, only one can match at a time. In such situation you should chain them together with elif.
Instead of doing a single except statement with multiple error types and then using conditionals to identify the correct one, it would be better to use multiple except statements each with a single error type.
You could iterate using range for slightly more compact code.

Like this:

def get_html_content(url, max_attempt = 3):
    req = Request(url, headers={'User-Agent': 'Mozilla/5.0'})

    for attempt in range(max_attempt):
        try:
            return urlopen(req, timeout=10).read()
        except HTTPError as e:
            print("The server couldn\'t fulfill the request....attempt {}/{}".format(attempt + 1, max_attempt))
            print('Error code: ', e.code)
        except URLError as e:
            print("We failed to reach a server....attempt {}/{}".format(attempt + 1, max_attempt))
            print('Reason: ', e.reason)
        except timeout as e:
            print('timeout...attempt {}/{}'.format(attempt + 1, max_attempt))


Objects/closures for storing movie information I'm currently in the process of learning JavaScript and what it has to offer. I thought writing a simple web app that would allow me to add/remove/show my favorite movies would be a nice learning experience. I'm curious as to what the best solution would be for an object to house all of the data. It would be triggered by an event such as the clicking of a button.

var Movies = (function() {
    var movieList = [];

    return {
        add : function(newTitle, newRating) {
            movieList.push({title: newTitle, rating: newRating});
        },
        show : function() {
            for(var i = 0; i < movieList.length; i += 1) {
                console.log(movieList[i].title + ": " + movieList[i].rating);
            }
        },
        del : function(titleString) {
            for(var i = 0; i < movieList.length; i += 1) {
                if(movieList[i].title === titleString) {
                    movieList.splice(i, 1);
                    break;
                }
            }
        }
   };
}());

If I'm using an IIFE, or the use of closures incorrectly, please let me know. I want to learn JavaScript properly.

You're using the "module" pattern, which works well for many use cases. Your code for this pattern is correct as it stands. The best pattern to use is more about how you are planning to use it.

Some different patterns and why they might be better depending on your use case:

This module pattern is a little difficult if you ever needed to create two movie lists on the same page. A different pattern would be the constructor pattern. This would allow you to easily initialize a new list on the same page.
One potential issue with this pattern is there is a lot of private variables. While this is sometimes good design, when you want to unit test this code, it might be faster to have these private variables available, so you can manipulate them for the test. You can do this with the revealing module pattern, or the constructor pattern which will expose them.


Calculator console app in Java Here is my source code for my very first app in Java. I am completely new to the language; any help and criticism is welcomed!
package com.company;

import java.util.*;

public class Main {

    public static float multiply(float a, float b)
    {
        return a*b;
    }

    public static float divide(float a, float b)
    {
        return a/b;
    }

    public static float addition(float a, float b)
    {
        return a+b;
    }

    public static float subtraction(float a, float b)
    {
        return a-b;
    }

    public static void main(String[] args)
    {
        Scanner scanner;
        float firstNum, secondNum, result;
        String operation;

        scanner = new Scanner(System.in);
        while(true){
            System.out.println("Select operation (*,/,+,-)");
            operation = scanner.next();
            if (operation.matches("[*/+-]")) {
                break;
            } else {
                System.out.println("Invalid operation");
            }
        }
        System.out.println("Type in first number");
        firstNum = scanner.nextFloat();

        System.out.println("Type in second number");
        secondNum = scanner.nextFloat();

        switch(operation)
        {
            case "*": result = multiply(firstNum, secondNum); break;
            case "/": result = divide(firstNum, secondNum); break;
            case "+": result = addition(firstNum, secondNum); break;
            case "-": result = subtraction(firstNum, secondNum); break;
            default: result = 0;
        }

        System.out.println("Result of your operation is " + result);
    }
}

For a first application in Java, good job!
I have some suggestions for your code.
When using regex, try to uses the java.util.regex.Pattern class instead of the java.lang.String#matches method.
When using the java.lang.String#matches method, the regex pattern is recompiled each time the method is called. This can cause a slowdown in your method execution.
How to uses the matcher
Pattern pattern = Pattern.compile("[*/+-]");
while (true) {
   //[...]
   if (pattern.matcher(operation).matches()) {
      //[...]
   }
   //[...]
}

Extract some of the logic to methods.
Extracting code can make the code shorter and easier to read. Also when you have logic that does the same thing, you can generally move it into a method and reuse it.

You can make a method that asks a question and read the next float; this will allow you to reuse the same method.

public static void main(String[] args) {
   //[...]
   firstNum = askQuestionAndReceiveFloat(scanner, "Type in first number");
   secondNum = askQuestionAndReceiveFloat(scanner, "Type in second number");
   //[...]
}

private static float askQuestionAndReceiveFloat(Scanner scanner, String question) {
   System.out.println(question);
   return scanner.nextFloat();
}

You can extract the logic to parse the operation.

private static String askForOperation(Pattern pattern, Scanner scanner) {
   String operation;
   while (true) {
      System.out.println("Select operation (*,/,+,-)");
      operation = scanner.next();

      if (pattern.matcher(operation).matches()) {
         break;
      } else {
         System.out.println("Invalid operation");
      }
   }
   return operation;
}

If you are using Java 14, you can use the newer version of the switch-case and return the value.

private static float getResult(float firstNum, float secondNum, String operation) {
   return switch (operation) {
      case "*" -> multiply(firstNum, secondNum);
      case "/" -> divide(firstNum, secondNum);
      case "+" -> addition(firstNum, secondNum);
      case "-" -> subtraction(firstNum, secondNum);
      default -> 0;
   };
}

Those's changes will make the main method shorter and easier to read!
Use java.io.PrintStream#printf instead of java.io.PrintStream#println when you have to concatenate
java.io.PrintStream#printf offer you to use patterns to build the string without concatenating it manually. The only downside is you will be forced to add the break line character yourself; in java you can use the %n to break the line (portable between various platforms) or uses the traditional \n / \r\n.
System.out.printf("Result of your operation is %s%n", result);

Refactored code
public static float multiply(float a, float b) {
   return a * b;
}

public static float divide(float a, float b) {
   return a / b;
}

public static float addition(float a, float b) {
   return a + b;
}

public static float subtraction(float a, float b) {
   return a - b;
}

private static float askQuestionAndReceiveFloat(Scanner scanner, String question) {
   System.out.println(question);
   return scanner.nextFloat();
}

private static String askForOperation(Pattern pattern, Scanner scanner) {
   String operation;
   while (true) {
      System.out.println("Select operation (*,/,+,-)");
      operation = scanner.next();

      if (pattern.matcher(operation).matches()) {
         break;
      } else {
         System.out.println("Invalid operation");
      }
   }
   return operation;
}

private static float getResult(float firstNum, float secondNum, String operation) {
   return switch (operation) {
      case "*" -> multiply(firstNum, secondNum);
      case "/" -> divide(firstNum, secondNum);
      case "+" -> addition(firstNum, secondNum);
      case "-" -> subtraction(firstNum, secondNum);
      default -> 0;
   };
}

public static void main(String[] args) {
   Pattern pattern = Pattern.compile("[*/+-]");
   Scanner scanner = new Scanner(System.in);
   float firstNum, secondNum, result;
   String operation = askForOperation(pattern, scanner);

   firstNum = askQuestionAndReceiveFloat(scanner, "Type in first number");
   secondNum = askQuestionAndReceiveFloat(scanner, "Type in second number");
   result = getResult(firstNum, secondNum, operation);

   System.out.printf("Result of your operation is %s%n", result);
}

Also, I suggest to handle the x / 0 case :)


Simple subnet calculator and parser I have started to learn Java for the past 20 days with no prior coding/comp science experience.

My brother gave me an idea to make a subnet calc/ping tool to enhance my knowledge and mentored through this.

Just wanted to know how can I upgrade this code and if I have used proper language idioms in this code. Any ideas to simplify/features for more learning would be appreciated.

import java.util.*;
import java.util.regex.Pattern;
import java.math.*;
import java.text.NumberFormat;
import java.text.ParseException;
import java.net.*;

public class example {

    public static void main(String[] args) {
        Scanner Scan = new Scanner(System.in);
        System.out.print("Enter IP: ");
        String IP;
        IP = Scan.nextLine();
        String Z = IP.split("\\/")[1];
        String Y = IP.split("\\/")[0];
        String[] parts = Y.split("\\.");
        String F1 = parts[0];
        String F2 = parts[1];
        String F3 = parts[2];
        String F4 = parts[3];
        int A = Integer.parseInt(F1);
        int B = Integer.parseInt(F2);
        int C = Integer.parseInt(F3);
        int E = Integer.parseInt(F4);

        {
            System.out.println(F1 + "." + F2 + "." + F3 + "." + F4);
        }
        String X = (IP.substring(IP.lastIndexOf("/") + 1)); {
            System.out.println(X);
        }
        int S = Integer.parseInt(X);
        double Bits = 32;
        double TotalBites = Bits - S;
        double I = Math.pow(2, TotalBites);
        int D = (int)(I - 2); {
            System.out.println(D);
        }

        List < String > Hosts = new ArrayList < String > ();

        if (S >= 24) {
            int val = (int) TotalBites - 0;
            String bitsda = stringMultiply("0", val);
            String bitstoconvert = "1" + bitsda;
            int decimal = Integer.parseInt(bitstoconvert, 2) - 1;
            //String bits = Strings.repeat("0", val);

            for (int IPF4 = E; IPF4 <= decimal; IPF4++) {

                Hosts.add(F1 + "." + F2 + "." + F3 + "." + IPF4);
            }

        } else

        if (S >= 16) {

            int val = (int) TotalBites - 8;
            String bitsda = stringMultiply("0", val);
            String bitstoconvert = "1" + bitsda;
            int decimal = Integer.parseInt(bitstoconvert, 2) - 1;
            //String bits = Strings.repeat("0", val);
            for (int IPF3 = C; IPF3 <= decimal; IPF3++) {
                for (int IPF4 = E; IPF4 <= 255; IPF4++)
                    Hosts.add(F1 + "." + F2 + "." + IPF3 + "." + IPF4);

            }

        } else

        if (S >= 8) {

            int val = (int) TotalBites - 16;
            String bitsda = stringMultiply("0", val);
            String bitstoconvert = "1" + bitsda;
            int decimal = Integer.parseInt(bitstoconvert, 2) - 1;
            //String bits = Strings.repeat("0", val);

            for (int IPF2 = B; IPF2 <= decimal; IPF2++) {
                for (int IPF3 = C; IPF3 <= 255; IPF3++)
                    for (int IPF4 = E; IPF4 <= 255; IPF4++)
                        Hosts.add(F1 + "." + IPF2 + "." + IPF3 + "." + IPF4);

            }
        }

        Hosts.forEach(host -> pingIt(host));

    }

    public static void pingIt(String host) {
        try {
            String ipAddress = host;
            InetAddress inet = InetAddress.getByName(ipAddress);
            System.out.println("Sending Ping Request to " + ipAddress);
            if (inet.isReachable(5000)) {
                System.out.println(ipAddress + " is reachable.");
            } else {
                System.out.println(ipAddress + " NOT reachable.");
            }
        } catch (Exception e) {
            System.out.println("Exception:" + e.getMessage());
        }
    }

    public static String stringMultiply(String s, int n) {
        StringBuilder sb = new StringBuilder();
        for (int i = 0; i < n; i++) {
            sb.append(s);
        }
        return sb.toString();
    }
}

Splitting the input, parsing the parts, figuring the bits and filling the hosts list should all be seperate methods that return the results to main.
Every method should perform a simple task. If you list the steps necessary to perform an action, each step should be a separate method.
Commenting a methods purpose above it will help you and others understand it's inner workings.
Bits and bites are not the same thing.
 double totalbites = Bits - S;
Also variable names should not start with a capital unless they are all capitals.
Variable names like A, B, C, E, X, S are not very helpful. input1, input2,... and num1, num2,... would convey a clearer picture of these variables purpose.
Use comments to explain the actions taking place.


Find the maximum GCD of a number with respect to all the values in an array for competitive programming Here is the Question. Find the largest GCD of the input variable with the values of an array.

Inputs are as follows:- First-line contains two integers, N and Q.

Second-line contains N integers which form the arr[].

Next, Q lines contain an integer M, the time in seconds she wishes to go back.

#include <stdio.h>

int main(){
    int n,q;
    scanf("%d %d", &n,&q);
    int a[n];
    int gcdn[n];
    for (int i = 0;i < n; i++) {
        scanf("%d",&a[i]);      }
    while (q>0){
        q--;
        int x;
        int max = 0;
        scanf("%d",&x);
        max = gcd(a[0],x);
        for(int i=0;i<n;i++) {
            int t = gcd(a[i],x);
            if(t > max)
                max = t;
        }
        printf("%d \n",max);
    }

}

int gcd (int a, int b) {
    if (b==0)
        return a;
    else
        return gcd(b, a%b);
}

How can I make this code better in terms of competitive programming? And what other languages can make this code better and more efficient?

gcdn[] is unused.
if n==0, you access a non-existent array element, and even if it isn't, you do the GCD with a[0] twice.
gcd() is called before it is declared, which is bad style and can mess up optimization.
you might think about whether the M or arr[] elements are likely to be larger.  The order you pass values to gcd() can reduce by one loop if you guess right.
The tail recursion on gcd() should be OK as long as you optimize.


Splitting string by words to new strings Function:

char **strsplit (char source[])
{
    char **strings = calloc(1, sizeof(char *));
    int nStrings = 0, nChars = 0;

    strings[0] = calloc(2, sizeof(char));

    while('\0' != *source)
    {
        if(*source == ' ')
        {
            nStrings++;
            nChars = 0;
            strings = realloc(strings, (1 + nStrings) * sizeof(char *));
            strings[nStrings] = calloc(2, sizeof(char));
            source++;
            continue;
        }

        strings[nStrings][nChars] = *source;
        strings[nStrings][nChars + 1] = '\0';

        nChars++;
        source++;
        strings[nStrings] = realloc(strings[nStrings], (2 + nChars) * sizeof(char));
    }

    return(strings);
}

Example usage:

char *source = "Hello Code Review";
char **words = strsplit(source);

printf("Second word is: %s", words[1]);

Output:

  Code

Add some documentation to function.

While the name strsplit is pretty descriptive in itself, you should still add documentation to at least explain the argument and the return value.

For example, I was a little confused when I saw strsplit because I thought it should've taken another argument that was the delimiter.

Add some more comments to your code!

I had to rubberduck this code a couple times before I understood what was happening and where.

I think at least having a comment for what each variable was doing would help a lot.

Allocating and re-allocating memory is very inefficient. You should set one size of memory that will definitely fit the amount of memory needed.

For example, you could just allocate, for each output string, the size of the input string. However, this eats up a lot of memory for big strings.

You may have to come up with another algorithm for this.

You are sort-of reinventing the wheel here, and I don't think that that is your intent.

<string.h> already has a function char *strtok(char *str, const char *delim). However, this function only grabs tokens one at a time, so you'd have to take these steps:

Grab a token
Put it in the string array
Go to 1

Is roughly what you'd have to do.

I wrote out an implementation in C. If you'd like to see it, then leave a comment.


Mandelbrot Set Fractal I thought I would give this months community challenge a try. This is my first time using Python.
It does take quite a while to run and it's not very colourful but it works.

from PIL import Image, ImageDraw

img = Image.new("RGB", (2400, 2400), "white")
draw = ImageDraw.Draw(img)

max_count = 200
width = img.size[0]
height = img.size[1]

for row in range(height):
    for col in range(width):
        str_output = ""
        c = complex(
            (col - float(width)/2.0)*5.0/float(width),
            (row - float(height)/2.0)*5.0/float(height)
        )
        iteration = 0
        z = 0
        while abs(z) < 2 and iteration < max_count:
            z = z**2 + c
            iteration += 1
        if abs(z) < 2:
            draw.point((col, row), fill="black")
        else:
            draw.point((col, row), fill=(255 - iteration,255 - iteration,255 - iteration))

img.save('mandelbrot.png')

Your constants should be in UPPER_SNAKE_CASE

MAX_COUNT = 200
WIDTH = img.size[0]
HEIGHT = img.size[1]

Instead of nested for loops, you can use itertools.product which basically will perform the nested iteration you need:

from itertools import product

for row, col in product(range(height), range(width))

You don't use str_output, you should clean up unused values.

You should have whitespace either side of your mathematical operators and have a space after each comma in a comma separated list of values:

(col - float(width) / 2.0) * 5.0 / float(width),
draw.point((col, row), fill=(255 - iteration, 255 - iteration, 255 - iteration))

You should also keep lines below 79 characters as the Python style guide dictates. You can split lines quite easily when they're in paretheses:

    draw.point((col, row), fill=(255 - iteration, 255 - iteration,
                                 255 - iteration))

Also your mileage may vary on whether or not this is more readable but you could use a ternary for your final if condition. Like this:

    draw.point((col, row), fill=("black" if abs(z) < 2 else
                                 (255 - iteration, 255 - iteration,
                                  255 - iteration))

The formatting is definitely more awkward, but I personally like these structures because it makes it clear that the only difference in the two results is that one condition.


Fixed-Size Memory Pool with alignment After taking a break and reviewing the comments on my first attempt at creating a fixed-size memory pool, this is my second attempt.

The memory pool should allocate each block correctly aligned in memory, so that when I call placement new on the returned memory, it isn't misaligned.
This is my main goal with this implementation.

The pool is implemented as a free-list, with each block looking something like this in memory:

    -------------------------------------------
   | Next Block Index | Padding | Object Data |
    -------------------------------------------

Here's my .h

#include <cstdint>
class FixedSizeMemoryPool
{
public:
    FixedSizeMemoryPool(const uint32_t objectSize, const uint32_t nObjects);
    ~FixedSizeMemoryPool();

    FixedSizeMemoryPool(const FixedSizeMemoryPool&)=delete;
    FixedSizeMemoryPool&operator=(const FixedSizeMemoryPool&)=delete;
    FixedSizeMemoryPool(FixedSizeMemoryPool&&)=delete;
    FixedSizeMemoryPool&operator=(FixedSizeMemoryPool&&)=delete;

    uint8_t* Alloc();
    void Free(void* data);

    const uint32_t GetCapacity() const;
    const uint32_t GetFreeBlockCount() const;
private:
    const uint32_t GetIndex(void* data) const;
    const uint32_t CalculatePadding(const uint32_t objectSize) const;
    uint32_t m_nFreeBlocks;
    uint32_t m_Capacity;
    uint32_t m_BlockSize;
    uint32_t m_PaddingSize;
    uint32_t m_NextFreeBlock;
    uint8_t* m_pData;
};

And the .cpp:

  FixedSizeMemoryPool::FixedSizeMemoryPool(const uint32_t objectSize,
                                         const uint32_t nObjects)
 : m_pData(nullptr), m_BlockSize(0), m_PaddingSize(0),
   m_Capacity(nObjects), m_nFreeBlocks(nObjects), m_NextFreeBlock(0)
{
    const uint32_t headerSize = sizeof(uint32_t);
    const uint32_t totalObjectSize = headerSize + objectSize;

    m_PaddingSize = CalculatePadding(totalObjectSize);
    m_BlockSize = totalObjectSize + m_PaddingSize;

    m_pData = new uint8_t[m_BlockSize * nObjects];

    //Set up the next free block
    for(unsigned int i = 0; i < nObjects; ++i)
    {
        uint8_t* hPtr = m_pData + (i * m_BlockSize);
        *((uint32_t*)hPtr) = i + 1;
    }
}

FixedSizeMemoryPool::~FixedSizeMemoryPool()
{
    delete[] m_pData;
}

uint8_t *FixedSizeMemoryPool::Alloc()
{
    if(m_NextFreeBlock == m_Capacity)
    {
        return nullptr;
    }

    uint8_t* ptr = m_pData + (m_NextFreeBlock * m_BlockSize);
    m_NextFreeBlock = *((uint32_t*)ptr);

    ptr += sizeof(uint32_t);
    ptr += m_PaddingSize;

    --m_nFreeBlocks;
    return ptr;
}

void FixedSizeMemoryPool::Free(void* data)
{
    uint8_t* pData = (uint8_t*)data - m_PaddingSize - sizeof(uint32_t);
    *((uint32_t*)pData) = m_NextFreeBlock;
    m_NextFreeBlock = GetIndex(data);
    ++m_nFreeBlocks;
}

const uint32_t FixedSizeMemoryPool::GetIndex(void *data) const
{
    return ((uint8_t*)data - m_pData) / m_BlockSize;
}

const uint32_t FixedSizeMemoryPool::CalculatePadding(
    const uint32_t objectSize) const
{
    //Check the object size is a power of 2
    if((objectSize & (objectSize - 1)) != 0)
    {
        //Get next highest power of 2
        uint32_t paddingSize = 0;
        paddingSize  = objectSize - 1;
        paddingSize  |= m_PaddingSize >> 1;
        paddingSize  |= m_PaddingSize >> 2;
        paddingSize  |= m_PaddingSize >> 4;
        paddingSize  |= m_PaddingSize >> 8;
        paddingSize  |= m_PaddingSize >> 16;
        ++paddingSize ;

        paddingSize  = paddingSize  - objectSize;
        return paddingSize;
    }
    return 0;
}

const uint32_t FixedSizeMemoryPool::GetCapacity() const
{
   return m_Capacity;
}

const uint32_t FixedSizeMemoryPool::GetFreeBlockCount() const
{
    return m_nFreeBlocks;
}

first of all you hold Next Block Index in allocated block. but you not use it . Next Block Index (which is implicit pointer to next free block) used only in free block. as result you not need Next Block Index and Padding in allocated block. the Object Data must begin at begin of block. this give serious optimization by memory size and simplify code.

also instead index (implicit pointer) better use explicit pointer to next free block (or 0 if no more free blocks). this yet more simplify code.

next your code is not thread safe and not design to allocate/free in concurrent from multiple threads. however this is very easy can be implemented.

finally CalculatePadding simply wrong. impossible calculate align of object by it size. different objects with same size, can have different align.

first we need implement structure describes an entry in a block linked list:

struct LE
{
    LE* next;

    static void Push(LE* head, LE* entry)
    {
        LE *_next = head->next, *next;

        for( ; ; _next = next)
        {
            entry->next = _next;

            next = (LE*)InterlockedCompareExchangePointerRelease(
                (void**)&head->next, entry, _next);

            if (next == _next)
            {
                return;
            }
        }
    }

    static LE* Pop(LE* head)
    {
        LE* _entry = head->next, *entry;

        for ( ;_entry;_entry = entry)
        {
            entry = (LE*)InterlockedCompareExchangePointerAcquire(
                (void**)&head->next, _entry->next, _entry);

            if (entry == _entry)
            {
                return entry;
            }
        }
        return 0;
    }
};

we put this LE at begin of every free block. this will pointer to next free block (or 0 if this is the last free block). instead of index. with this we can allocate block by LE::Pop and free block by LE::Push.

#define DEBUG_CHECK 1

class FixedSizeMemoryPool
{
    LE m_head;
    void* m_buf;
    void (__stdcall * m_Free)(void*);
#if DEBUG_CHECK
    ULONG m_count;
#endif

public:

    FixedSizeMemoryPool() { m_buf = 0; }

    ~FixedSizeMemoryPool();

    void* Alloc()
    {
        return LE::Pop(&m_head);
    }

    void Free(void* data)
    {
        LE::Push(&m_head, reinterpret_cast<LE*>(data));
    }

    bool Create(ULONG objectSize, ULONG nObjects,
        void* (__stdcall* XxxAllocate)(ULONG),
        void (__stdcall * XxxFree)(void*));
};

FixedSizeMemoryPool::~FixedSizeMemoryPool()
{
    if (m_buf)
    {
#if DEBUG_CHECK
        ULONG count = m_count;

        if (LE* next = m_head.next)
        {
            do
            {
                --count;
            } while (next = next->next);
        }

        if (count)
        {
            __debugbreak();
        }
#endif
        m_Free(m_buf);
    }
}

bool FixedSizeMemoryPool::Create(ULONG objectSize, ULONG nObjects,
                                 void* (__stdcall* XxxAllocate)(ULONG),
                                 void (__stdcall * XxxFree)(void*))
{
    if (!objectSize || !nObjects || objectSize > MAXLONG)
    {
        return false;
    }

    if (objectSize < sizeof(LE))
    {
        // we need space for LE in block
        objectSize = sizeof(LE);
    }

    // must be aligned how minimum as LE
    objectSize = (objectSize + alignof(LE) - 1) & ~(alignof(LE) - 1);

    ULONGLONG size = (ULONGLONG)objectSize * (ULONGLONG)nObjects;

    if (size > MAXULONG)
    {
        return false;
    }

    union {
        void* buf;
        char* pb;
        LE* entry;
    };

    if (buf = XxxAllocate((ULONG)size))
    {
#if DEBUG_CHECK
        m_count = nObjects;
#endif
        m_buf = buf;
        m_Free = XxxFree;
        LE* next = 0;
        do
        {
            entry->next = next, next = entry;
        } while (pb += objectSize, --nObjects);

        m_head.next = next;

        return true;
    }

    return false;
}

note that client of FixedSizeMemoryPool must provide itself routines for allocate/free block of memory, where pool will be implemented. this give ability implement different memory types. also if objects(pool blocks) require special align (more than default allocation align), user implement this in allocate routine. say if we not need extended alignment for object, we can use

void* __stdcall XxxAlocate(ULONG s)
{
    return new char[s];
}

void __stdcall XxxFree(void* data)
{
    delete [] data;
}

if we need extended alignment - say want 64byte align for blocks in pool:

void* __stdcall XxxAlocate(ULONG s)
{
    return _aligned_malloc(s, 64);
}

void __stdcall XxxFree(void* data)
{
    _aligned_free(data);
}

and of course size of object (block) must be multiple of object(block) align


Inserting a user sign-up record using mysqli My Code:
Connectivity(php)

<?php

$conn=mysqli_connect("localhost","root","","webchat_data");
//if($conn)
//{
//  echo "hi";
//}
if(!$conn){
    die("CONNECTION FAILED" .mysqli_connect_error());
}

?>

Signup

<?php
include 'connectivity.php';
$uname=($_POST['uname']);
$email=($_POST['Email']);
$pass=($_POST['Password']);

$sql = "INSERT INTO `sign-up`(USERNAME,EMAIL_ID,PASSWORD) VALUES('$uname','$email','$pass')";
//$result=$conn->query($sql);
$result = mysqli_query($conn, $sql);
//$query=mysqli_query($conn,$sql);

header("Location:index.php");

?>

Someone told me that my query is good for a learner, but not for real world applications. Please make some recommendations to make this code more suitable for real world scenarios.

Like it was said in the comments, this code needs not a review but just some basic practices. That said, good basic practices are a rare specimen in the wild, so you cannot be blamed, given the number of awful tutorials out there. Luckily I am the renowned collector of good practices and here you are

Connectivity

There ate many things that could be improved in your connectivity file. To name a few

the connection character set must be configured to avoid issues with characters
the proper error reporting mode for mysqli must be set
the connection error smust be not bluntly echoed out

All these issues are covered in my article How to connect properly using mysqli: so let's take the code from there:

$host = '127.0.0.1';
$db   = 'webchat_data';
$user = 'root';
$pass = '';
$charset = 'utf8mb4';

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
try {
    $conn = new mysqli($host, $user, $pass, $db);
    $conn->set_charset($charset);
} catch (\mysqli_sql_exception $e) {
     throw new \mysqli_sql_exception($e->getMessage(), $e->getCode());
}

Password hashing

In two words, never store passwords in plain text. Use password_hash() function instead. This topic is thoroughly explained in the question: https://stackoverflow.com/questions/1581610/how-can-i-store-my-users-passwords-safely

Prepared statements

Just never add a variable to SQL query directly, but mark its place with a question mark instead. Then bind the actual variable and finally call execute()

In detail this matter is explained in this question: https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php

So here is your signup code reviewed

<?php
include 'connectivity.php';

$uname = $_POST['uname'];
$email = $_POST['Email'];
$pass  = password_hash($_POST['Password'], PASSWORD_DEFAULT);

$sql = "INSERT INTO `sign-up`(USERNAME,EMAIL_ID,PASSWORD) VALUES(?,?,?)";
$stmt = $conn->prepare($sql);
$stmt->bind_param("sss", $uname,$email,$pass);
$stmt->execute();

header("Location:index.php");