Shortening and optimizing diamond-printing code I'm working through a Java textbook by Paul and Harvey Deitel and I have come across a review question (Chapter 2 - Intro to Java, Exercise 2.18 for those that have the book) which asks the user to create the following diamond:

    *
   * *
  *   *
 *     *
*       *
 *     *
  *   *
   * *
    *

I have the following code which does this but, as always, I can't help but think there's a better way to do this. Any suggestions on a shorter way?

// Prints the start and end of the diamond
private static void printStartEnd(int numLines, char sign, char filler) {
    int xPosition = numLines / 2;

    // Loop through
    for (int i = 0; i < numLines; i++) {
        if (i == xPosition) {
            System.out.print(sign);
        } else {
            System.out.print(filler);
        }
    }
}

// Prints the other lines
private static void printOtherLines(int numLines, int positionOne,
        int positionTwo, char sign, char fill) {
    int i = 0;

    // Loop through
    while (i < numLines) {
        if (i == positionOne || i == positionTwo) {
            System.out.print(sign);
        } else {
            System.out.print(fill);
        }
        i++;
    }
}

// Draw the diamond to the console
public static void drawDiamond( int numLines, char sign, char fill )
{
    // Diamonds can only be created for odd numbers
    if( numLines % 2 != 0 )
    {
        boolean limitReached = false;

        int halfWayPointMinus = numLines / 2 - 1;
        int halfWayPointPlus = numLines / 2 + 1;

        for( int i = 0; i < numLines; i++ )
        {
            if( i == 0 || i == numLines-1 )
            {
                printStartEnd( numLines, sign, fill );
            }
            else
            {
                printOtherLines( numLines,halfWayPointMinus,halfWayPointPlus,sign,fill );
                if( halfWayPointMinus == 0 || limitReached )
                {
                    limitReached = true;

                    // Invert values and move back in
                    halfWayPointMinus++;
                    halfWayPointPlus--;
                }
                else
                {
                    halfWayPointMinus--;
                    halfWayPointPlus++;
                }

            }
            // Move to next line
            System.out.println();
        }
    } else
    {
        System.out.println( "Sorry, the number of lines entered must be an odd number..." );
    }
}

Error handling

When you detect an error condition, it's usually better to put the error handler immediately after the if-condition, then bail out.  The error handler is usually shorter than the main code, so it's less mental workload to get it out of the way instead of having to look for the matching else many lines further down.  The advantage of putting the error handler first becomes more apparent when multiple errors are involved:

if (SUCCESS != validate1()) {
    return handleError1();
}
if (SUCCESS != validate2()) {
    return handleError2();
}
if (SUCCESS != validate3()) {
    return handleError3();
}
// The main processing can proceed here

… is more readable than:

if (SUCCESS == validate1()) {
    if (SUCCESS == validate2()) {
        if (SUCCESS == validate3()) {
            // Do the main processing here
        } else {
            handleError3();
        }
    } else {
        handleError2();
    }
} else {
    handleError1();
}

Secondly, it is usually more appropriate to print error messages to System.err.  You don't want unexpected junk contaminating your output.

Consider changing the behaviour such that it throws an IllegalArgumentException rather than printing an error message.  Throwing an exception allows the function's caller to programatically handle the error, making it more flexible and reusable.

Flow of Control

Using boolean variables (like limitReached) to control the flow of your program is usually a sign that you could structure the code better.  In this case, you always take the else-branch when printing the top half of the diamond, then always take the if-branch for the bottom half.  You might as well write it as two for-loops.

On the other hand, there is a competing Don't-Repeat-Yourself principle regarding the body of the loop.  In my opinion, you would still be better off clarifying the flow-of-control by splitting it into two loops.  To avoid copy-and-pasting the loop body, you want to push the special cases into your printing function, so that the body is just one function call.  That would be a smart move anyway, as it makes your printing function more resilient and versatile.

Printing Function

It turns out that your printOtherLines() can be used to print the apex and the base just fine.  You don't need printStartEnd() at all.

I would rename the numLines parameter to size or width.  Calling it numLines in the context of printOtherLines() is a bit confusing, since you are only printing one line per function call.

Object-Oriented Thinking

I don't know whether your exercise specifies a particular interface, but I don't like drawDiamond(int numLines, char sign, char fill).  A more object-oriented approach would be:

Diamond d = new Diamond(/* size= */ 5);
d.draw(System.out, '*', ' ');

In other words, the diamond knows how to draw itself.

Suggested Solution

import java.io.PrintStream;

public class Diamond {
    private int size;

    /**
     * Constructor.
     * @param size The size of the diamond in lines (or columns)
     * @throw IllegalArgumentException if size is negative or even
     */
    public Diamond(int size) {
        if (size <= 0) {
            throw new IllegalArgumentException("Diamond size must be positive");
        }
        if (size % 2 == 0) {
            throw new IllegalArgumentException("Diamond size must be odd");
        }
        this.size = size;
    }

    public void draw(PrintStream out, char sign, char fill) {
        int posL = this.size / 2,
            posR = posL;
        while (posL > 0) {
            // Handle the top half
            this.printLine(out, sign, fill, posL--, posR++);
        }
        while (posL <= posR) {
            // Handle the bottom half, including the middle (widest) line
            this.printLine(out, sign, fill, posL++, posR--);
        }
    }

    private void printLine(PrintStream out, char sign, char fill, int posL, int posR) {
        // Loop condition could be (col <= posR) to omit trailing filler
        for (int col = 0; col < this.size; col++) {
            out.print((col == posL || col == posR) ? sign : fill);
        }
        out.println();
    }

    public static void main(String[] args) {
        try {
            Diamond d = new Diamond(Integer.parseInt(args[0]));
            d.draw(System.out, '*', ' ');
        } catch (IllegalArgumentException e) {
            System.err.println("Sorry, the number of lines must be an odd number...");
        }
    }
}


Checking the strength of a password using regexes I'm a beginner in Python and I made a password checker. I'm following these rules:

I have to use regex
password must be at least 8 characters
must have at least one lower case character
must have at least one upper case character
must have at least one special character
must have at least one number

I also wanted to display a message letting the user know what conditions weren't met.

I'm a bit concerned by the succession of ifs.

#strong password checks
import re

#ask the user for the password to check
pwtocheck = input('please enter the password to be checked:')

#Error message to be
Err_mess = ''

#first regex : at least one lower case characters
pw_lc = re.compile(r'([a-z]+)')

#second regex : at least one upper case characters
pw_uc = re.compile(r'([A-Z]+)')

#third regex: at least one special character
pw_sc = re.compile(r'([\-_\?!@#$%^&\*]+)')

#fourth regex : password must have at least one number
pw_d = re.compile(r'([0-9])+')

#create a list to be given to all() to evaluate if all conditions are met or not
cond_pw = [len(pwtocheck) > 8, pw_lc.search(pwtocheck), pw_uc.search(pwtocheck), pw_sc.search(pwtocheck), pw_d.search(pwtocheck)]

#building an error message
#is the password at least 8 characters
if len(pwtocheck) < 8:
    Err_mess = Err_mess + 'password is too short\n'

#first regex is none
if pw_lc.search(pwtocheck) is None:
    Err_mess = Err_mess + 'password needs at least one lower case character\n'

#second regex is None
if pw_uc.search(pwtocheck) is None:
    Err_mess = Err_mess + 'password needs at least one upper case character\n'

#third regex is None
if pw_sc.search(pwtocheck) is None:
    Err_mess = Err_mess + 'password needs at least one special character\n'

#fourth regex is none
if pw_d.search(pwtocheck) is None:
    Err_mess = Err_mess + 'password needs at least one number\n'

if all (cond_pw) is True:
    print('password is ok')
else:
    print(Err_mess)

Concept

Current security research recommends eliminating character composition requirements. It's more important to blacklist common weak passwords. Also, passphrases are a good alternative to passwords. For the purposes of this code review, though, I'll play along with your original premise.

Style

PEP 8, the official Python style guide, recommends lower_case_with_underscores for variable names. Err_mess violates that guideline for no good reason.

Regexes

You don't need the capturing parentheses, if you're not going to do anything with the capture groups. You also don't need the + modifiers, since the requirement is for just one character of each class.

Within a character class (inside square brackets), most characters lose their special meanings. You don't need to escape ? and * with backslashes.

Design

You're performing each check twice: once to check the password validity, and a second time to gather all of the failure reasons.

This code suffers from the fact that the rules are encoded in multiple places:

the regular expressions (and their variable names)
the comments
the if statements (note especially the length check, which breaks the pattern)
the error message strings

It would be better to treat all of the rules as data, uniformly, and have a single statement that performs all of the checks.

import re

pw_to_check = input('please enter the password to be checked:')

rules = [
    ('password is too short', r'.{8}'),
    ('password needs at least one lower case character', r'[a-z]'),
    ('password needs at least one upper case character', r'[A-Z]'),
    ('password needs at least one special character', r'[-_?!@#$%^&*]'),
    ('password needs at least one number', r'[0-9]'),
]

err_msg = '\n'.join(
    req
    for req, regex in rules
    if not re.search(regex, pw_to_check)
)

print(err_msg or 'password is ok')


Downloading images from URLs I'm just beginning with Python and programming, so been trying to get as much experience reading code as possible.

The script mentioned below do grab images from URLs and put them in into a folder. The code is tested and works fine. Is there a way to shorten/improve this code? Any ideas/suggestions are appreciated.

import urllib2
from os.path import basename
from urlparse import urlsplit
from bs4 import BeautifulSoup # for HTML parsing

global urlList
urlList = []

# recursively download images starting from the root URL
def downloadImages(url, level): # the root URL is level 0
    print url
    global urlList
    if url in urlList: # prevent using the same URL again
        return
    urlList.append(url)
    try:
        urlContent = urllib2.urlopen(url).read()
    except:
        return

    soup = BeautifulSoup(''.join(urlContent))
    # find and download all images
    imgTags = soup.findAll('img')
    for imgTag in imgTags:
        imgUrl = imgTag['src']
        try:
            imgData = urllib2.urlopen(imgUrl).read()
            fileName = basename(urlsplit(imgUrl)[2])
            output = open(fileName,'wb')
            output.write(imgData)
            output.close()
        except:
            pass

    # if there are links on the webpage then recursively repeat
    if level > 0:
        linkTags = soup.findAll('a')
        if len(linkTags) > 0:
            for linkTag in linkTags:
                try:
                    linkUrl = linkTag['href']
                    downloadImages(linkUrl, level - 1)
                except:
                    pass

# main
downloadImages('http://www.yahoo.com', 1)

Python version 2.6.

Notes about your code

You can also simplify the way you look for the links to follow:

for link in soup.select("a[href]"):
    downloadImages(link["href"], level - 1)

Here, we are enforcing the a elements to have href values, not checking for elements to be found (since the loop body would just not be executed in this case).
As far as managing the urls you've already visited:

don't use globals
use a set instead of a list for faster lookups

It's also a good practice to always specify the parser BeautifulSoup uses under-the-hood:

soup = BeautifulSoup(urlContent, "lxml")
# or soup = BeautifulSoup(urlContent, "html.parser")
# or soup = BeautifulSoup(urlContent, "html5lib")

follow PEP8 recommendations - specifically, naming is agreed to be in lower_case_with_underscores format, not camelCase
no need to join the urlContent - simply pass urlContent to BeautifulSoup

Alternative solution

I would implement it this way:

try:
    from urlparse import urljoin
except ImportError:
    from urllib.parse import urljoin

import requests
from bs4 import BeautifulSoup

class Scraper:
    def __init__(self):
        self.visited = set()
        self.session = requests.Session()
        self.session.headers = {"User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.109 Safari/537.36"}

        requests.packages.urllib3.disable_warnings()  # turn off SSL warnings

    def visit_url(self, url, level):
        print(url)
        if url in self.visited:
            return

        self.visited.add(url)

        content = self.session.get(url, verify=False).content
        soup = BeautifulSoup(content, "lxml")

        for img in soup.select("img[src]"):
            image_url = img["src"]
            if not image_url.startswith(("data:image", "javascript")):
                self.download_image(urljoin(url, image_url))

        if level > 0:
            for link in soup.select("a[href]"):
                self.visit_url(urljoin(url, link["href"]), level - 1)

    def download_image(self, image_url):
        local_filename = image_url.split('/')[-1].split("?")[0]

        r = self.session.get(image_url, stream=True, verify=False)
        with open(local_filename, 'wb') as f:
            for chunk in r.iter_content(chunk_size=1024):
                f.write(chunk)

if __name__ == '__main__':
    scraper = Scraper()
    scraper.visit_url('http://www.yahoo.com', 1)

Aside from things mentioned above, here are some other applied changes:

using requests third-party library with a shared session
Python 2 and 3 compatible
using class for sharing "session" and a set of visited urls
separate methods to visit urls and download images
if __name__ == "__main__": is used to avoid the code being executed on import
changed the way image filename is determined (probably still not the best way)
handling relative urls as well as absolute


javascript for controllers of multiple audio tags I am trying to learn to create controllers for my own HTML5 audio tags, which I often have several of on the same page. The js.jQuery code below is a simple beginning to this; it works to apply a play/pause button to each corresponding audio tag with a class of my_audio.

I'm a new coder though, so I imagine there are problems to my approach. Can anyone point out those problems, or suggest a more efficient way to do this?

(Btw, yes I know about jPlayer, etc. Just wanting to learn to do it myself :^)

$(document).ready(function(){

    $(".my_audio").each(function(index){
        $( this ).attr('id', "audioId" + index);
        var playPauseId = "playPause" + index;
        $( this ).after(
            '<div class="audio_controls_bar">\
                <button id="' + playPauseId + '">Pause</button>\
            </div>'
        );
        doAudios(index);
    });

});

function doAudios(index) {
    var audio, playbtn;
    function initializePlayer(){
    //set object references
        audio = document.getElementById("audioId"+index);
        console.log(audio);
        playbtn = document.getElementById("playPause"+index);
        console.log(playbtn)
    // add event listeners
        playbtn.addEventListener("click", playPause, false);
    }
    initializePlayer();

    function playPause(){
        if(audio.paused){
            audio.play();
            playbtn.innerHTML = "Pause";
        } else {
            audio.pause();
            playbtn.innerHTML = "Play";
        }
    }

}

This is good code for a new coder.

Some observations:

I am not sure why you add a pause button through JavaScript, you could have this button declared simply in the HTML.
doAudios -> misnomer, since it deals with 1 audio at a time, so doAudio makes more sense.
You go through a lot of effort to give everything an id so that you can wire everything together, it is not needed if you use your closure inside the main loop.

I would counter-propose something like this to get rid of the id centered code:

$(document).ready(function(){

    $(".my_audio").each(function(index){
        var audio = this,
            playPause = $( '
<button>Pause</button>
' ),
            button = $( audio ).after( playPause ).children().first();

        button.addEventListener("click", function(){
            if(audio.paused){
              audio.play();
              this.innerHTML = "Pause";
            } else {
              audio.pause();
              this.innerHTML = "Play";
            }
        }, false);
    });
});

If you were willing to drop the div and put the styling on the button, it would be a bit cleaner :

$(document).ready(function(){

    $(".my_audio").each(function(index){
        var audio = this,
            button = $( '<button class="audio_controls_bar">Pause</button>' ),
            $( audio ).after( button );

        button.addEventListener("click", function(){
            if(audio.paused){
              audio.play();
              this.innerHTML = "Pause";
            } else {
              audio.pause();
              this.innerHTML = "Play";
            }
        }, false);
    });
});

Since here you can drop playPause and the .children().first();.


Finding the greatest common factor of a list of numbers in JavaScript The goal of this code is to take an array of two or more positive integers and return their greatest common factor, or GCF. (This is the biggest number that can be divided into all the numbers in the array without any remainder).

I feel like my code is much too long and complicated. Surely there's a more efficient way to do this. Also, I know it's best practice to avoid global variables, and I used one here.

Codepen

/* Test whether a number is a factor of another number */
function isFactor(num, fact){
    if(num % fact == 0){
        return true;
    }
    else{
        return false;
    }
}

/* List all the factors of a number */
function listFactors(number){
    factors = [1];
    var i = 2;
    while(i <= number){
        if(isFactor(number, i)){
            factors.unshift(i);
        }
        i++;
    }
    return factors;
}

var toTest = 1;

/* Find the GCF (greatest common factor) of the numbers in an array */
function GCF(intList){
    var GCF = 1;
    var factorsOfEach = [];
    for(item in intList){
        var num = intList[item];
        var factors = listFactors(num);
        factorsOfEach.push(factors);
    }
    var count = 0;
    factorsOfFirst = factorsOfEach[0];
    var length = factorsOfFirst.length;
    while(count < length){
        var toTest = factorsOfFirst[count];
        var passTest = factorsOfEach.every(arrayContains);
        if(passTest){
            GCF = toTest;
            return GCF;
        }
        else{
            count += 1;
        }
    }
    return GCF
}

/* Check whether an array contains the variable "toTest" */
function arrayContains(array){
    if(array.indexOf(toTest) != -1){
        return true}
    else{
    return false}
}

Formatting

Before addressing optimizations, let's address some formatting problems

while vs. for loops

It is preferred to use a for loop when you:

Have an incrementer
The incrementer is not used outside of the loop
The incrementer is either strictly increasing or strictly decreasing

This is the case with both of your while loops. They can be refactored to the following:

In listFactors():

for (var i = 2; i <= number; i++){
    if(isFactor(number, i)){
        factors.unshift(i);
    }
}

In GCF():

for (var count = 0; count < factorsOfFirst.length; count++){
    var toTest = factorsOfFirst[count];
    var passTest = factorsOfEach.every(arrayContains);
    if (passTest) {
        return toTest;
    }
}

Avoid explicit booleans when possible

Instead of:

if(num % fact === 0){
    return true;
}
else{
    return false;
}

it is preferable to write:

return num % fact === 0;

and the same for your other functions.

Avoid global variables

As you correctly pointed out, you should try not to use mutable global variables; it makes the code much harder to understand. To avoid this, you could make the arrayContains function merely an arrow function and pass in toTest as well:

var testPassed = factorsOfEach.every(arr => arr.indexOf(toTest) !== -1);

A Cleaner Solution

Using the Euclidean Algorithm to find the GCD/GCF of two numbers (assuming non-negative numbers) is a much more cleaner and optimized approach:

function GCF(a, b) {
    if (b === 0) return a;
    else         return GCF(b, a % b);
}

Then, just use a reduce statement to apply GCF to all of the numbers in the array:

function findGCFofList(list) {
    return list.reduce(GCF);
}


Implementing a Property Searcher I want to search some or all public properties of a class and for this requirement I have implemented a Property Searcher.

The Input for a Search is always a string. The Output contains the SearchString, the SearchContent in which the SearchString is contained and the searched Property.The Output is wrapped in a SearchResult object.

This is what i have implemented:

SearchResult.cs

public class SearchResult
{
    public PropertyInfo Property { get; private set; }

    public string SearchString { get; private set; }

    public string SearchContent { get; private set; }

    public ISearchable Item { get; private set; }

    public SearchResult(ISearchable item,
                        PropertyInfo property,
                        string searchString)
    {
        this.Property = property;
        this.SearchString = searchString;
        this.Item = item;
        this.SearchContent = property.GetValue(item).ToString();
    }
}

ISearchable.cs

public interface ISearchable
{
    IEnumerable<SearchResult> Search(string searchString);
}

Extension Methods for easier usage:

Extension.cs

public static class Extension
{
    public static SearchResult Search(this ISearchable source, string searchString, PropertyInfo info)
    {
        if (info.GetValue(source).ToString().Contains(searchString, StringComparison.OrdinalIgnoreCase))
        {
            return new SearchResult(source, info, searchString);
        }

        return null;
    }

    public static IEnumerable<SearchResult> Search(this IEnumerable<ISearchable> source, string searchString)
    {
        return source.SelectMany(searchable => searchable.Search(searchString)).Where(x => x != null);
    }

    public static bool Contains(this string source, string toCheck, StringComparison comp)
    {
        return source.IndexOf(toCheck, comp) >= 0;
    }
}

I also want to show you the example and test code for e.g. searching in the property of a Book with a Description (the code is simplified and more complex in the original project).

Book.cs

public class Book : ISearchable
{
    public DescriptionInfo DescriptionInfo { get; set; }
    public string Title { get; set; }

    public IEnumerable<SearchResult> Search(string searchString)
    {
        yield return this.Search(searchString, this.GetType().GetProperty(nameof(this.Title)));
        foreach (var searchResult in this.DescriptionInfo.Search(searchString))
        {
            yield return searchResult;
        }
    }
}

Description.cs

public class DescriptionInfo : ISearchable
{
    public string Description { get; set; }

    public string ShortDescription { get; private set; }

    public DescriptionInfo(string description)
    {
        if (!string.IsNullOrEmpty(description))
        {
            this.Description = this.ShortDescription = description;
        }
    }

    public IEnumerable<SearchResult> Search(string searchString)
    {
        yield return this.Search(searchString, this.GetType().GetProperty(nameof(this.Description)));
    }
}

Program.cs

internal class Program
{
    private static void Main(string[] args)
    {
        var one = new Book
        {
            Title = "This Test Title is Amazing",
            DescriptionInfo = new DescriptionInfo("This i a nice Test Book Description")
        };
        var two = new Book
        {
            Title = "This second Test Title is Amazing",
            DescriptionInfo = new DescriptionInfo("This i a nice Book Description")
        };
        var three = new Book {Title = "Boring Title", DescriptionInfo = new DescriptionInfo("No Description")};

        var list = new List<Book>(new[] {one, two, three});
        var results = list.Search("Test").ToList();
    }
}

In results are three SearchResult objects. The string "Test" was found two times in a Title Property and one time in the Description Property of the Description class.

I do not know if this is the best solution to solve the problem. If you have a better one in mind please let me know.

I don't have a C# compiler at hand and cannot test the following.

It looks like the Search() always returns the sum of results of these 3 things:

delegate the call to any property that's ISearchable.
search the value of some properties
do not search the value of some other properties

The method will likely always consist of a list of these types of code blocks:

the delegate type

foreach (var searchResult in this.PROPERTY_THAT_IS_ISEARCHABLE.Search(searchString))
{
    yield return searchResult;
}

for the direct search

yield return this.Search(searchString, this.GetType().GetProperty(nameof(this.PROPERTY_THATS_NOT_ISEARCHABLE)));

no code for excluded properties

This smells like copy&paste. But you are already doing reflection, so why not use it to your advantage?

I think you can create a generic Search() method, then slap that onto everything that implements ISearchable via extension, getting you rid of explicitely implementing the method in those classes. Here's how:

You should be able to filter out all properties that implement ISearchable and delegate the method call to them.
For non-ISearchable properties that should be searched, you should decide if they are the minority1. If so, define an attribute to opt-in a property. You'd then ignore all non-ISearchable by default and only search the values of those that are explicitly opted-in with the attribute.
For non-ISearchable properties that should not be searched, do pretty much the opposite of (2.). If they are the minority, create and add to them an attribute explicitly to opt-out of the default which would be to search all non-ISearchable properties.

1 The minority thing is only to reduce future work, minimizing the amount of attributes that you have to add. Attributes are optional, so you have to define some default behaviour anyway.

Let's say the default behaviour is to search all properties and opt-out via attribute. Here's how your classes could look like:

public class Book : ISearchable
{
    public DescriptionInfo DescriptionInfo { get; set; }
    public string Title { get; set; }
}

As you can see in the Book class: ISearchable is more or less a marker interface.

public class DescriptionInfo : ISearchable
{
    public string Description { get; set; }

    [IgnoreThisDuringSearch]
    public string ShortDescription { get; private set; }

    public DescriptionInfo(string description)
    {
        if (!string.IsNullOrEmpty(description))
        {
            this.Description = this.ShortDescription = description;
        }
    }
}

The generic extension method would perform the following two steps then:

delegate to every property that it can be delegated to
search every property that got opt-in via attribute or search every property except those opt-out via attribute

Some additional thoughts:

The attribute makes the search independent from the actual names of the properties. If you change Title to Totle in the Book class, you current code breaks. The suggested solution is solely based on reflection and existance (lack of) an attribute, which should always work, given a sensible default behaviour.

If you wish to perform the searching/delegating in a special order, you could do that, too. For example via a property of another attribute. See [DataMember(Order = n)] for example.


Converting Java int-based amount to Coin metrics I was tasked with writing a method which would take in an int (for amount in coins) and then convert the coins, accordingly, by counting how many quarters, dimes, nickels, and pennies were located in the amount. Afterwards, it would return a String which stated each count of individual coin.

public class Coins {

    private static final int QUARTER_VALUE = 25;
    private static final int DIME_VALUE = 10;
    private static final int NICKEL_VALUE = 5;
    private static final int PENNY_VALUE = 1;

    public static String convert(int amount) {
        int currentAmount = amount;
        int quarters = 0;
        int dimes = 0;
        int nickels = 0;
        int pennies = 0;

        String quarterUnit = new String();
        String dimeUnit = new String();
        String nickelUnit = new String();
        String pennyUnit = new String();

        if (amount != 0) {
            quarters = amount / QUARTER_VALUE;

            currentAmount =
                     amount - (quarters * QUARTER_VALUE);

            dimes = currentAmount / DIME_VALUE;

            currentAmount =
                     amount - (quarters * QUARTER_VALUE) - (dimes * DIME_VALUE);

            nickels = currentAmount / NICKEL_VALUE;

            currentAmount =
                     amount - (quarters * QUARTER_VALUE)
                     - (dimes * DIME_VALUE) - (nickels * NICKEL_VALUE);

            pennies = currentAmount / PENNY_VALUE;
        }

        if (quarters == 1) {
            quarterUnit = " quarter";
        }

        else if (quarters > 1 || quarters == 0){
            quarterUnit = " quarters";
        }

        if (dimes == 1) {
            dimeUnit = " dime";
        }

        else if (dimes > 1 || dimes == 0) {
            dimeUnit = " dimes";
        }

        if (nickels == 1) {
            nickelUnit = " nickel";
        }

        else if (nickels > 1 || nickels == 0) {
            nickelUnit = " nickels";
        }

        if (pennies == 1) {
            pennyUnit = " penny";
        }

        else if (pennies > 1 || pennies == 0) {
            pennyUnit = " pennies";
        }

        StringBuilder conversion = new StringBuilder();

        return conversion.append(quarters)
                         .append(quarterUnit)
                         .append("\n")
                         .append(dimes)
                         .append(dimeUnit)
                         .append("\n")
                         .append(nickels)
                         .append(nickelUnit)
                         .append("\n")
                         .append(pennies)
                         .append(pennyUnit)
                         .append("\n")
                         .toString();
    }

    public static void main(String[] args) {
        String conversionString = convert(89);

        System.out.println(
              new StringBuilder().append("Converted amount equates to: ")
                                 .append("\n"));

        System.out.println(conversionString);
    }
}

Here's the output:

Converted amount equates to:

3 quarters
1 dime
0 nickels
4 pennies

Is there a more efficient (fewer lines of code or a better OO style) way to implement this?

Highlighting what a few helper classes can do :

Imagine this enum

private enum Coin {
    QUARTER(25, "quarter"), DIME(10, "dime"), NICKEL(5, "nickel"), PENNY(1, "penny", "pennies");
    ...

and a tuple of quotient and remainder :

private static class QuotientAndRemainder {
    private final int quotient;
    private final int remainder;

so the enum can sport this method :

public QuotientAndRemainder divide(int pennies) {
    return QuotientAndRemainder.quotient(pennies / pennyValue).withRemainder(pennies % pennyValue);
}

and this one :

public String format(int amount) {
    return Integer.toString(amount) + ' ' + (amount == 1 ? singular : multiple);
}

then the convert method simply becomes :

public static String convert(int amount) {
    return print(minimalChange(amount));
}

private static String print(Map<Coin, Integer> amounts) {
    StringBuilder builder = new StringBuilder();
    for (Coin coin : amounts.keySet()) {
        builder.append(coin.format(amounts.get(coin))).append('\n');
    }
    return builder.toString();
}

private static Map<Coin, Integer> minimalChange(int amount) {
    int penniesLeft = amount;
    Map<Coin, Integer> amounts = new EnumMap<Coin, Integer>(Coin.class);
    for (Coin coin : Coin.values()) {
        QuotientAndRemainder quotientAndRemainder = coin.divide(penniesLeft);
        amounts.put(coin, quotientAndRemainder.quotient());
        penniesLeft = quotientAndRemainder.remainder();
    }
    return amounts;
}

Adding larger units of currency now simply comes down to adding them to the enum.


Optimizing pagination of HTML tables I'd like to share my implementation to you to confirm if it's a correct way to solve my problem. I have a list of products which have an attributes table and a description table (both in html_encoded strings). Every product starts at a new print-page using page break. When the attributes and description tables don't fit in one page I want to print the description in the next page.

My solution is to print the attributes table and then print the description div twice: once in the same page with the attributes and once at the next page using a page break; After the page has loaded using JavaScript, I measure the height of both attributes+description and if that is higher than 800 pixels (which means that two pages will be needed) I hide the first description. If their height is lower than 800 pixels (which means they can fit in the same page) I hide the second one.

(I know for certain that neither of the two tables will exceed one page by itself)

Is this a solid solution or are there possible cases in which it will lead to a big mess?

.
.
.
<input id="total_product_count" type="hidden" value="<?php echo $total_product_count; ?>" />

<?php foreach ($products as $product) { ?>

...small image + title....

    <div  class="attribute" id="attributes[<?php echo $product['product_count']; ?>]" ><?php echo $product['attributes']; ?>

    <div id="description[<?php echo $product['product_count']; ?>]" class="tab-content"> <?php echo $product['description']; ?>

    <div id="second_description[<?php echo $product['product_count']; ?>]" class="tab-content">

          <?php echo $product['description']; ?>

<?php }?>

This isn't an actual solution to your problem, but I noticed that you aren't using var to declare your JavaScript variables. Doing so automatically declares them in the global scope, which is a bad idea as a general rule of thumb and can quickly lead to unpredictable behavior. Declaring variables should be done like var myvar = myvalue:

<script>
    function measure(){
        var $total_count=document.getElementById("total_product_count").value;
        for(var i = 1; i <= $total_count; i++){
            var $description_id='description['+i+']';
            var $second_description_id='description['+i+']_second';
            var $attributes_id='attributes['+i+']';
            $combined_height = document.getElementById($description_id).clientHeight + document.getElementById($attributes_id).clientHeight;

            if($combined_height>800){
                document.getElementById($description_id).style.display = 'none';
            }
            else{
                 document.getElementById($second_description_id).style.display = 'none';
            }

        }
    }
</script>

...


Exercism assignment word-count in Python Can someone review my code which I put on exercism.io for the word-count exercise (found here)?

class Phrase:
    def __init__(self, phrase):
        self.phrase = phrase.strip().lower().split()

    def word_count(self):
        word_dict = {}
        for word in self.phrase:
            word_input = ''.join(e for e in word if e.isalnum()).strip()
            if word_input:
                word_dict[word_input] = word_dict.get(word_input, 0) + 1
        return word_dict

My first advice is stop writing classes if you don't need to : your class has only two methods an init and a proper method. This could probably be written as a simple function.

def word_count(phrase):
    word_dict = {}
    for word in phrase.strip().lower().split():
        word_input = ''.join(e for e in word if e.isalnum()).strip()
        if word_input:
            word_dict[word_input] = word_dict.get(word_input, 0) + 1
    return word_dict

Also, a lot of string processing you are doing is not useful, complicated and/or potentially wrong :

On phrase, as noticed by Gareth Rees, there's not point calling strip() since you call split().

As I wasn't aware that the result would be the same, here a proof of concept :

a='  this  is    a little      test'
a.split() == a.strip().split()
-> True

and here's a link to the documentation for strip and split.

On individual words : the way you get word_input from word looks a bit convoluted to me. Among other things, there's no point in calling strip() on a string that only contains alphanumerical characters. Also, just removing "special" characters does not sound like the best option : it's and its will be considered the same way, its annoying :-). Maybe some characters like ' should be taken into account during the splitting. Maybe, depending on the language you are to handle, other characters like - should be kept (for instance in French, "basketball" is "basket-ball" but neither "basket" nor "ball" are French words so splitting would be quite awkward and so would be removing the dash).

Except for that, your code looks go to me!

However, if you wanted to make things even more pythonic, you could use defaultdict. This example will probably look familar to you.


Refactoring Logger interface to open and close logs I asked a question about an implementation of a logger using syslog and error_log as fallback that made me re-evaluate my Logger interface.  Before it simply forced a log($timestamp, $message) to be implemented but after reading over the answer to the question I'm not sure that this is the best API possible to work with all the different logging options available.

Here's my new interface that allows logs to be opened and closed.

<?php

/**
 * @file
 * @brief Holds the interface to be implemented by objects responsible for logging
 * messages and data.
 */

namespace SprayFire\Logger;

interface Logger {

    /**
     * @brief We have left the parameters for this method blank on purpose; the
     * implementations should be responsible for passing the appropriate arguments.
     *
     * @details
     * It should be noted that if your implementation defines the method signature
     * with parameters you may assign a default value to that parameter and your
     * implementation will satisfy this method signature.
     *
     * @return true if log opened, false if didn't
     */
    public function openLog();

    /**
     * @param $timestamp the timestamp for the given message
     * @param $message The string message that should be appended to the end
     *        of the log
     * @return true if the message was logged, false if it didn't
     */
    public function log($timestamp, $message);

    /**
     * @return true if the log was closed, false if it didn't.
     */
    public function closeLog();

}

My problem is keeping the interface generic enough while allowing the various different options needed in opening a log to be satisfied.  For example, NullLogger wouldn't really need any parameters passed to openLog() where as SysLogLogger would probably need some parameters passed.  How would I accomplish this?

What I'm thinking now

That parameters will be passed as variable arguments to the function and the implementation uses func_get_args() to retrieve the necessary information to open the log, if necessary.

Alternatively, the implementation could simply define the parameters needed and ensure there are default values for all parameters.  This would ultimately supply the needs of the interface while still communicating to the user that parameters are needed.

What I could be doing

I could declare the parameter passed to openLog() as an array that holds the appropriate option information.  This means though that I would need to create an array for every Logger, whether it is needed or not.  I'm not one to micro-optimize but at the same time I don't want to be creating unused/needless resources.

I could declare the three parameters passed to openlog in Logger::openLog, and then declare default values for those in the method signature signaling that you could simply not provide these arguments.  But, this really doesn't "feel" right, you'd also be signaling to the user that you could pass arguments to say, NullLogger, that might actually influence the way the implementation works and it really wouldn't.

I'm not sure what would be the best solution here and would appreciate any feedback on the interface's API and the solutions proposed.

NullLogger wouldn't really need any parameters passed to
  openLog() where as SysLogLogger would probably need some
  parameters passed. How would I accomplish this?

Pass these parameters to the constructor, store in a private field and use it in the openLog (or any other) function.

If you use func_get_args() you wouldn't be able to replace the logging system without modifying the client code.

SLF4J is a great example for a logging framework. Check it's Logger interface. It contains some methods but none of them depend on any implementation. Logback, Log4j, JCL and other implementations implements this simple interface and their external configuration determines the details.

A note: maybe timestamp shouldn't be passed to the log function, it should be the internal of the logger implementation.


Rapid color replacement for in-memory BMP A function that relatively quick replaces one color with another, in a memory-buffered BMP.
I am trying to improve the performance as much as it is possible. The previous implementation of this function was even faster.. but then I ran into some padding complications.

union
{
    unsigned long ulColor;
    unsigned char byteColor[4];
} oldColor;
union
{
    unsigned long ulColor;
    unsigned char byteColor[4];
} newColor;

typedef unsigned char BYTE;
typedef unsigned short int WORD;
typedef unsigned long int DWORD;
typedef unsigned long long int DDWORD;

DWORD
bitfox_find_xy
(DWORD WIDTH, DWORD xp, DWORD yp)
{
    DWORD w = WIDTH;
    DWORD channels = 3;
    DWORD bpp = 8;
    DWORD single = (channels*bpp)/8;
    DWORD offset = (2+sizeof(BMP)+sizeof(DIB));
    DWORD rowsize = w*single;

    DWORD pixAddress;

    if (rowsize % 4 != 0) rowsize += 4 - (rowsize % 4); // account for padding
    pixAddress = offset + yp*rowsize + xp*single; // finding the address
    return pixAddress;
}
DDWORD
bitfox_color_replace_data
(BYTE *buff, BYTE old_r, BYTE old_g, BYTE old_b, BYTE new_r, BYTE new_g, BYTE new_b)
{
    #define OFFSET_OF_SIZE      0x2
    #define OFFSET_OF_PIXELS    0xA
    #define OFFSET_OF_WIDTH     0x12
    #define OFFSET_OF_HEIGHT    0x16

    DWORD* buffSize     = (DWORD*)&buff[OFFSET_OF_SIZE];
    DWORD* buffPixels   = (DWORD*)&buff[OFFSET_OF_PIXELS];
    DWORD* buffWidth    = (DWORD*)&buff[OFFSET_OF_WIDTH];
    DWORD*  buffHeight  = (DWORD*)&buff[OFFSET_OF_HEIGHT];

    DDWORD pixels_replaced = 0;
    DDWORD ln, col;

    oldColor.byteColor[0] = old_b;  newColor.byteColor[0] = new_b;
    oldColor.byteColor[1] = old_g;  newColor.byteColor[1] = new_g;
    oldColor.byteColor[2] = old_r;  newColor.byteColor[2] = new_r;

    for(ln = 0; ln < *buffHeight; ln++)
    {
        for(col = 0; col <*buffWidth; col++)
        {
            DDWORD offset = bitfox_find_xy(*buffWidth, col, ln);

            if(!memcmp(buff + offset, oldColor.byteColor, 3))
            {
                memcpy(buff + offset, newColor.byteColor, 3);

                pixels_replaced++;
            }
        }
    }
    return pixels_replaced;
}

Computing rowsize inside bitfox_find_xy (that is, for each and every pixel)seems wasteful: row size never changes during the bitfox_color_replace_data invocation, and shall only be computed once.


Get Users and related roles and show on View I have 3 tables. Users, Roles and RolesToUsers

Here is User model:

public partial class User
{
    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2214:DoNotCallOverridableMethodsInConstructors")]
    public User()
    {
        this.UserToRoles = new HashSet<UserToRole>();
    }

    [Key]
    public int Id { get; set; }
    public string Name { get; set; }

    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")]
    public virtual ICollection<UserToRole> UserToRoles { get; set; }
}

Here is Role Model:

public partial class Role
{
    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2214:DoNotCallOverridableMethodsInConstructors")]
    public Role()
    {
        this.UserToRoles = new HashSet<UserToRole>();
    }

    [Key]
    public int Id { get; set; }
    public string RoleName { get; set; }

    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")]
    public virtual ICollection<UserToRole> UserToRoles { get; set; }
}

And here is UserToRole model:

public partial class UserToRole
{
    [Key]
    public int Id { get; set; }
    [ForeignKey("Role")]
    public Nullable<int> RoleId { get; set; }
    [ForeignKey("User")]
    public Nullable<int> UserId { get; set; }

    public virtual Role Role { get; set; }
    public virtual User User { get; set; }
}

I need to show all Users and related to it Roles.

I write this repo method

public List<UserViewModel> GetUsers()
{
    var users = db.Users
        .Include(u => u.UserToRoles.Select(ur => ur.Role))
        .Select(u => new UserViewModel
        {
            UserName = u.Name,
            Roles = u.UserToRoles.Select(ur => ur.Role.Name)
        })
        .ToList();

    return users;
}

After it I need to show data from database in this format Ronald - Admin,User, where Ronald,etc is Name of Users, after - is Roles related to user.

Repo method is working great

And in View I show it using this code.

function GetUsers() {
let url = "/Home/GetUsers";
$.ajax({
    type: 'GET',
    url: url,
    dataType: 'json',
    success: function (data) {
        var separator = "";
        for (var i = 0; i < data.length; ++i) {
            var userRow = data[i].UserName + " - " + data[i].Roles.join(", ");

            $('#taskresult3').append(separator + userRow);
            separator = ",";
        }
    }
});

Any suggestions or improvements I can do to this code?

partial is meant to be used when the class spans multiple files, not when the class is to be extended. If you want to use virtual there is no need to mark the class as partial unless it really does exist in multiple files (a bad design for the most part anyway, with few good uses). If you want to force the class to be inherited, use abstract instead of partial.

You can remove the default constructor (and the warning suppression) by using default values:

public class User
{
    [Key]
    public int Id { get; set; }
    public string Name { get; set; }

    [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2227:CollectionPropertiesShouldBeReadOnly")]
    public virtual ICollection<UserToRole> UserToRoles { get; set; } = new HashSet<UserToRole>();
}

It is now functionally the same but has less code to maintain.

Nullable properties can also be defined shorthand:

public class UserToRole
{
    [Key]
    public int Id { get; set; }
    [ForeignKey("Role")]
    public int? RoleId { get; set; }
    [ForeignKey("User")]
    public int? UserId { get; set; }

    public virtual Role Role { get; set; }
    public virtual User User { get; set; }
}

The int? is effectively the same as Nullable<int>, it is a little easier on the eyes without having to mentally parse generics.

The last comment is about your repo method. It could be shortened up a little bit:

public List<UserViewModel> GetUsers()
{
    return db.Users
        .Include(u => u.UserToRoles.Select(ur => ur.Role))
        .Select(u => new UserViewModel
        {
            UserName = u.Name,
            Roles = u.UserToRoles.Select(ur => ur.Role.Name)
        })
        .ToList();
}

Or, if you really want to shorten it up:

public List<UserViewModel> GetUsers() => db.Users
                                           .Include(u => u.UserToRoles.Select(ur => ur.Role))
                                           .Select(u => new UserViewModel
                                           {
                                               UserName = u.Name,
                                               Roles = u.UserToRoles.Select(ur => ur.Role.Name)
                                           }).ToList();

The only reason I would use the intermediate users variable in the method is to do debugging inside the method. You could add a breakpoint on the return users; line and debug the value. If you are confident in the operation of the query and the results, shortening it up helps with code maintenance.


Selection Sort Java and Analysis I have written a selection sort code in java. I know its very elementary algorithm, but since i am learning, so wanted your input about the quality of the code. Please have a look at the code:
package selection_sort;
import java.util.Scanner;

public class SelectionSort {

int [] arrayToBeSorted;
Scanner scan=new Scanner(System.in);

SelectionSort(){
    System.out.println("Enter the number of elements");
    int total=scan.nextInt();
    this.arrayToBeSorted=new int[total];
    for(int i=0;i<total;i++) {
        System.out.println("Enter the "+i+" number of elements");
        this.arrayToBeSorted[i]=scan.nextInt();
    }

}

public void Sort() {
    int minIndex,min;
    boolean swapRequired=false;
    for(int i=0;i<arrayToBeSorted.length;i++) {
        minIndex=i;
        min=this.arrayToBeSorted[i];
        for(int j=i+1;j<this.arrayToBeSorted.length;j++) {
            if(this.arrayToBeSorted[j]<min) {
                min=this.arrayToBeSorted[j];
                minIndex=j;
                swapRequired=true;
            }
        }
        if(swapRequired) {
            swap(i,minIndex);
        }

    }

    for(int x:this.arrayToBeSorted) {
        System.out.print(x+" ");
    }

    }

public void swap(int i,int j) {

    //System.out.println("swap called, pos="+i+"and minindex="+j);
    int temp=this.arrayToBeSorted[i];
    this.arrayToBeSorted[i]=this.arrayToBeSorted[j];
    this.arrayToBeSorted[j]=temp;

    //System.out.println("------------");

}
public static void main(String[] args) {
    SelectionSort s=new SelectionSort();
    s.Sort();
}

}

Also about Analysis, as selection is O(n2), is it because I am using nested for loop. Is there any tool which tells us about complexity of code .Thanks in advance and please be patient if its very naive

int [] arrayToBeSorted;
Scanner scan=new Scanner(System.in);

Without any modifier, members and functions are package-private. Which means that they are accessible from the same package, but not by instances extending the class. That's an extremely odd thing, actually, when looking at it from an object-oriented point of view. You want to either make them private, or if extending classes should be able to access them, protected.
Same goes for the constructor.

    System.out.println("Enter the number of elements");
    int total=scan.nextInt();
    this.arrayToBeSorted=new int[total];

You could use a List/ArrayList instead of an array, which would mean that you can receive as many numbers from the users as they want without having to specify the count beforehand. An empty input could be used for declaring the end of the list. However, that has the downside that you must use Integer instead of int, so it' is kinda hard to tell what is better. It also has the downside that detecting an empty input is that readily available from Scanner, as you'd need to use nextLine for that, with manual conversion to int.
You could also emulate a List by dynamically growing the array. Either by doing it for every item, performance does not matter in this use-case, or by doubling the size when needed.

this.arrayToBeSorted=new int[total];

You only require the this modifier if you have a variable with the same name in the same scope, for example in a constructor:
public ValueContainer(int value) {
    this.value = value;
}

It's common practice to omit this if not needed. If you ever find yourself in the situation that it is confusing whether an instance, static or local variable is used, you did something wrong there anyway.

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

I'm a very persistent advocate for using "real" names for variables in loops.
for (int counter = 0; counter < total; counter++) {
// or
for (int index = 0; index < total; index++) {

public void Sort() {

The Java naming conventions state that methods/functions should be lowerCamelCase.

int minIndex,min;

Personally I'd avoid declaring multiple variables on the same line. It makes it easy to miss declaration.

boolean swapRequired=false;

That's a great example of a great name for a variable, thank you!

    for(int x:this.arrayToBeSorted) {
        System.out.print(x+" ");
    }

That, unfortunately, is a bad name for a variable. Only use "x", "y" and "z" when working with dimensions. "value" or "number" would be a great name here.

Overall, it looks valid. I did not run it to test the functionality, though. What you should change is that you have logic in the constructor. Nobody expects constructors to be filled with logic, because you can't see the intent of logic from them. They should be as nothing doing as possible. That means that you should move the logic into sort or, even better, into main. Your Sorter should not be concerned with input at all, it should only be concerned with sorting values, ideally, and another class (InputReader?) should be concerned with getting the input. So what you could do is something like this:
public static final void main(String[] args) {
    InputReader inputReader = new InputReader();

    int[] values = inputReader.readValues();

    Sorter sorter = new Sorter();
    sorter.sort(value);

    // TODO Print sorted output.
}

That also has the upside that you don't need to hold state in Sorter at all, which would make it thread-safe by default.


CS50x Readability improvement I am brand new to writing my own code. I just started the CS50x intro class and have so far made it to pset 2 readability. It's getting easier to understand what's going on. I just wanted to get some opinions on the code that I wrote to see if there were possible improvements/changes that I could make.

#include <stdio.h>
#include <cs50.h>
#include <string.h>
#include <ctype.h>
#include <math.h>

//initialize lenght function
int length(string str, char check);

int main(void)
{
    //Get user input
    string input = get_string("Text: ");

    //Calculate and set grade level. Take user input and run it thorugh the lenght function. l for letters, w for words and s for sentances.
    float gradeLevel = (0.0588 * (100 * (float) length(input, 'l') / (float) length(input, 'w')) - 0.296 * (100 * (float) length(input, 's') / (float) length(input, 'w'))) - 15.8;

    //output grade level
    if (round(gradeLevel) >= 16)
    {
        printf("Grade 16+\n");
    }

    else if (round(gradeLevel) < 1)
    {
        printf("Before Grade 1\n");
    }

    else
    {
        printf("Grade %i\n", (int) round(gradeLevel));
    }

    //print lengths for testing
    /*
    printf("%i letter(s)\n", length(input, 'l'));
    printf("%i word(s)\n", length(input, 'w'));
    printf("%i sentance(s)\n", length(input, 's'));
    */
}

//function to get length of letters, words and sentences
int length(string str, char check)
{
    int letterCount = 0;
    int wordCount = 1;
    int senCount = 0;
    int n = strlen(str);

//check letter count
    if (check == 'l')
    {
        for (int i = 0; i < n; i++)
        {
            if (isalpha(str[i]))
            {
                letterCount++;
            }
        }
        return letterCount;
    }
//check word count
    else if (check == 'w')
    {
        for (int i = 0; i < n; i++)
        {
            if (str[i] == ' ' || str[i] == '\n' || str[i] == '\t')
            {
                wordCount++;
            }
        }
        return wordCount;
    }
//check sentance count
    else if (check == 's')
    {
        for (int i = 0; i < n; i++)
        {
            if (str[i] == '.' || str[i] == '!' || str[i] == '?')
            {
                senCount++;
            }
        }
        return senCount;
    }
    return 0;
}

Thank you for the detailed feedback. Here's the specs of the program

Your program should count the number of letters, words, and sentences in the text. You may assume that a letter is any lowercase character from a to z or any uppercase character from A to Z, any sequence of characters separated by spaces should count as a word, and that any occurrence of a period, exclamation point, or question mark indicates the end of a sentence.

Your program should print as output "Grade X" where X is the grade level computed by the Coleman-Liau formula, rounded to the nearest integer.

If the resulting index number is 16 or higher (equivalent to or greater than a senior undergraduate reading level), your program should output "Grade 16+" instead of giving the exact index number. If the index number is less than 1, your program should output "Before Grade 1".

The algorithm used was the Coleman-Liau index

You're off to a great start!  This is reasonably readable, and I'm sure you'll get better with practice.

lenght

Typo.  I make tons of typos myself.  I highly recommend getting an IDE with a spell checker.

string input = ...

This confused me at first.  It has to be a typedef, but what kind of typedef makes sense?  Turns out in <cs50.h>, they typedef char *string;.  This is a little weird to me because char* is a very familiar type -- I instantly know a few things to look for/what operations you can do with that.  string is an unknown entity.  I think char* input = ... is clearer.

float gradeLevel = (0.0588 * (100 * (float) length(input, 'l') / (float) length(input, 'w')) - 0.296 * (100 * (float) length(input, 's') / (float) length(input, 'w'))) - 15.8;

This line is so long!  Breaking it down and simplifying a bit:

float l_over_w = (float) length(input, 'l') / (float) length(input, 'w');
float s_over_w = (float) length(input, 's') / (float) length(input, 'w');
float gradeLevel = (5.88 * l_over_w - 29.6 * s_over_w) - 15.8;

I have no idea where the numbers 5.88, 29.6, and 15.8 came from.
Maybe the div function would help you instead of casting to float?  floats are slow and have a lot of idiosyncrasies... IMO they are best to avoid unless you absolutely MUST use them.
I find the general idea here non-obvious.  How about some comments?
This could be a function float gradeLevel(char* input);

int length(string str, char check)

You've used check to determine what this function should do.  That's generally a bad idea, but if you cannot avoid it, at least assert(false) if check is invalid.  That way you get a runtime error if check is wrong.  As you've written it, you'll silently get the wrong answer if check is wrong.

In this case, it is trivial to do better.  You could have several functions: int letterCount(char* str);, int wordCount(char* str);, and int sentenceCount(char* str);.  This way you get a compiler error if you write the wrong function name (and you can't get check wrong since check doesn't exist).

int letterCount = 0;
int n = strlen(str);
for (int i = 0; i < n; i++) {
    if (isalpha(str[i])) {
        letterCount++;
    }
}
return letterCount;

This is OK, but you go over the string once to compute strlen and once to compute letterCount.  You can do them at the same time:

int letterCount = 0;
for (int i = 0; str[i]; i++) {
    if (isalpha(str[i])) {
        letterCount++;
    }
}
return letterCount;

I think this is also nicer since you don't have an n to worry about.  You could further simplify this to get rid of the index and just have the pointer:

while (str) {
    letterCount += isalpha(*str++);
}

but maybe that's overkill.

N.B. at least you didn't write for (int i = 0; i < strlen(str); i++) which goes over the string once per character!

(str[i] == ' ' || str[i] == '\n' || str[i] == '\t')

How about isspace(str[i])?

(str[i] == '.' || str[i] == '!' || str[i] == '?')

How about ispunct(str[i])?  Not an exact match so be careful.

Here's a list of similar functions available https://linux.die.net/man/3/ispunct.

//print lengths for testing
/* ...

This is totally fine for a personal project, but there are lots of testing frameworks that will always compile your test code but only run it under certain conditions.  Might be worth researching.  Here's a list to get you started https://en.wikipedia.org/wiki/List_of_unit_testing_frameworks#C.

Do you need to deallocate the string returned by get_string?

Is the number of words equal to the number of spaces?  What about this string with         lots of white-spaces.....  How many sentences are in that string?  I don't know the answer to these questions (maybe the exercise tells you what to compute?), but I think it's interesting to think about.


Newton Raphson and polynomials in C I have the following code, that defines:

A polynomial struct with some useful functions.
The newton Raphson algorithm for polynomials.

and calculates sqrt(2).
What can I improve?

There are some things I am unsure about myself:

Is size_t a good datatype for my purposes? I needed to be very careful when writing the loop condition in the eval function. Is this a good place to switch to signed arithmetics?
Is my definition of my_nan a good way to ensure portability when switching to another floating point type?

Things I am aware of:
I know, that the Newton Raphson for polynomials does not require the explicit construction of a derived polynomial and I am a bit wasteful on the memory.

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <math.h>

typedef double real;
const real my_nan = ((real) 0.0) / ((real) 0.0);

typedef struct Polynomial {
    real* coeffs;
    size_t capacity;
} Polynomial;

Polynomial* create_poly(size_t capacity)
{
    real* coeffs;

    coeffs = malloc(capacity * sizeof(real));
    if (coeffs == NULL) return NULL;

    Polynomial* poly = malloc(sizeof(Polynomial));

    if (poly == NULL) {
        free(coeffs);
        return NULL;
    }
    poly->coeffs = coeffs;
    poly->capacity = capacity;

    return poly;
}

void delete_poly(Polynomial* p)
{
    free(p->coeffs);
    free(p);
    p = NULL;
}

size_t deg(const Polynomial*const p)
{
    for (size_t i = p->capacity - 1; i > 0; i--) {
/* Here we actually want to compare reals exactly instead of |a - b| < eps */
        if (p->coeffs[i] != 0.0) return i;
    }
    return 0;
}

void print(const Polynomial*const p)
{
    size_t i;
    for (i = 0; i < deg(p); ++i) {
        printf("%f * x^%zu + ", p->coeffs[i], i);
    }
    printf("%f * x^%zu\n", p->coeffs[i], i);
}

real eval(const Polynomial*const p, real x)
{
/* Use Horner Scheme for evaluation */
    size_t i = deg(p);
    real res = p->coeffs[i];

    for (; i-- > 0;) {
        res = res * x + p->coeffs[i];
}
    return res;
}

Polynomial* derive(const Polynomial*const p)
{
    Polynomial* Dp = create_poly(p->capacity);
    if (Dp == NULL) return NULL;

    for (size_t i = 1; i < p->capacity; ++i) {
        Dp->coeffs[i - 1] = ((real) i) * p->coeffs[i];
    }
    return Dp;
}

real newton_raphson_poly(const Polynomial*const p, real x0, real eps)
{
    Polynomial* Dp = derive(p);
    real x, prev = x0;
    const int max_iter = 100;

    for (int i = 0; i < max_iter; ++i) {
        x = prev - eval(p, prev) / eval(Dp, prev);
        if (fabs(x - prev) < eps) {
            return x;
        } else {
            prev = x;
        }
    }

    return my_nan;
}

int main()
{
    Polynomial* p;
    const real EPS = pow(10, -7);
    p = create_poly(3);

    p->coeffs[0] = -2;
    p->coeffs[1] = 0;
    p->coeffs[2] = 1;

    printf("The result of sqrt(2) is given by the root of\n");
    print(p);
    printf("Its value is: %f \n", newton_raphson_poly(p, 1.0, EPS));

    delete_poly(p);
}
```

Overall Impression

The code displays some good programming practices or habits. It is generally well structured, with well thought out functions. Good definitions of typedefs. The type size_t is used to index arrays instead of int.

There are several potential bugs in the code. They are potential because they could happen, not that they would happen as the code is written currently.

Error Checking

While the return values of malloc() are checked, there should be some additional error checking in 2 places in the code. The first is that in Polynomial* create_poly(size_t capacity) before any memory allocation takes place, the value of the parameter capacity should be checked that is larger than zero, if not the memory allocation should not take place.

The second place for additional error checking is in main(). If p after the call to create_poly() is NULL, assignments to p will fail, possibly causing a catastrophic error. This is one potential bug.

Memory Allocation

It might be better to use calloc() rather than malloc() in the following statement:

    coeffs = malloc(capacity * sizeof(real));

The memory allocation function calloc(size_t count, size_t size) was written with arrays in mind. In addition to being slightly more readable, calloc() sets all of the values to zero when the array is allocated, which means the values in the array are initialized.

    real* coeffs = calloc(capacity, sizeof(*coeffs));

Note that in the example of calloc() usage the sizeof() argument is what coeffs points to. This allows who ever is maintaining the code to change the type of coeffs without have to modify more than the type itself. If the type real was used in the statement, there would be 2 places to change the code and not one.

Unnecessary Statement

In the function void delete_poly(Polynomial* p) the statement p = NULL; is unnecessary. Since p was passed in instead of a pointer to p it only affects the local value of p, it does not affect the value of p in main().

Initialize Variables When They are Declared

In at lest 2 places in the code variables are declared on one line and then initialized on another line like the initialization is an after thought. A better habit to get into is to initialize the variables as they are declared. This can lead to less bugs and less debugging of code.

In main():

    Polynomial* p;
    const real EPS = pow(10, -7);
    p = create_poly(3);

Versus

    const real EPS = pow(10, -7);
    Polynomial* p = create_poly(3);

In create_poly()

    real* coeffs;

    coeffs = malloc(capacity * sizeof(real));
    if (coeffs == NULL) return NULL;

Versus

    real* coeffs = malloc(capacity * sizeof(real));
    if (coeffs == NULL) return NULL;

Magic Numbers

There are Magic Numbers in the main() function (10 and -7), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

The code already has a symbolic constant defined in real newton_raphson_poly(const Polynomial*const p, real x0, real eps).

    const int max_iter = 100;

Write the code consistently.

Possible Optimizations

The code in main() could be more flexible or extensible if it modified to create an array of coefficients and get the size of the array as the capacity. The code to copy the coefficients into the Polynomial struct could be an additional function, or it could also be added to create_poly(size_t capacity, real coeffs[]).

int main()
{
    real coeffs[] = {-2, 0, 1};
    size_t capacity = sizeof(coeffs) / sizeof(*coeffs);

    const real EPS = pow(10, -7);
    Polynomial* p = create_poly(capacity);

    real* poly_coeffs_ptr = &p->coeffs[0];
    real* coeffs_ptr = &coeffs[0];
    for (size_t i = 0; i < capacity; i++)
    {
        *poly_coeffs_ptr = *coeffs_ptr;
    }

    printf("The result of sqrt(2) is given by the root of\n");
    print(p);
    printf("Its value is: %f \n", newton_raphson_poly(p, 1.0, EPS));

    delete_poly(p);
}

Reordering of malloc()s might lead to less code.

Polynomial* create_poly(size_t capacity)
{
    if (capacity > 0)
    {
        Polynomial* poly = malloc(sizeof(Polynomial));
        if (poly != NULL)
        {
            poly->coeffs = calloc(capacity, sizeof(real));
            if (poly->coeffs == NULL)
            {
                free(poly);
                poly = NULL;
            }
        }
        return poly;
    }

    return NULL;
}


Python coding style from Java background I am pretty new to Python and just wanted to make sure I am going in the right direction with my coding style.

After reading this, I'm not so sure.

The following is a convenience class that has operations used a lot in my main code.

import requests

try: import simplejson as json
except ImportError: import json

"""
Basic operations involving users
"""

#The URL of the API
api_root = "http://api.stuff.com/v1/core";

class UserOperations(object):
    def __init__(self, headers):
        self.headers = headers

    def move_user(self,source_account,destination_account, user_id):

        user_id = str(user_id)

        #Get user
        user = self.get_user(source_account, user_id)

        if user is not None:

            #Remove user from source account
            self.remove_user(source_account, user_id)

            #Add user to destination account
            self.add_user(destination_account, user)

    def get_user(self, account_id, user_id):

        #Get user
        user = requests.get(api_root + "/accounts/" + account_id + "/users/" + user_id, headers=self.headers)

        #Throw exception if non-200 response
        user.raise_for_status()

        print "\nGet User " + user_id + ": " + user.text

        #Check user exists
        if user.json()['Data'] is None:
            return None

        #Return user
        return user.json()['Data']

    def remove_user(self,account_id, user_id):

        #Delete a user
        result = requests.delete(api_root + "/accounts/" + account_id + "/users/" + user_id, headers=self.headers)
        #Throw exception if non-200 response
        result.raise_for_status()
        #Result of delete operation
        print "\nDelete Result " + result.text

    def add_user(self,account_id, user):

        #Add user to new account
        result = requests.post(api_root + '/accounts/' + account_id + '/users', data=json.dumps(user), headers=self.headers)
        #Throw exception if non-200 response
        result.raise_for_status()
        #Result of add operation
        print "\nAdd User: " + result.text

Does this seem on the right track or are there any Python mantra's I'm missing out on.

Edit

Function to replace below code duplication

variable = requests.get(api_root + "/accounts/"....)
variable.raise_for_status()

Function:

def request(self, request, account_id, user):

    if request is 'get':
        #Get user
        result = requests.get(api_root + "/accounts/" + account_id + "/users/" + user_id, headers=self.headers)
    elif request is 'post':
        #Add user to new account
        result = requests.post(api_root + '/accounts/' + account_id + '/users', data=json.dumps(user), headers=self.headers)
    elif request is 'delete':
        #Delete user from account
        result = requests.delete(api_root + "/accounts/" + account_id + "/users/" + user_id, headers=self.headers)

    #Throw exception if non-200 response
    result.raise_for_status()
    return result

Edit
Function attempt 2:

def request(self, request, account_id, user):

    function_dict = {}
    function_dict['get'] = requests.get(api_root + "/accounts/" + account_id + "/users/" + user, headers=self.headers)
    function_dict['post'] = requests.post(api_root + '/accounts/' + account_id + '/users', data=json.dumps(user), headers=self.headers)
    function_dict['delete'] = requests.delete(api_root + "/accounts/" + account_id + "/users/" + user, headers=self.headers)

    result = function_dict.get(request)

    #Throw exception if non-200 response
    result.raise_for_status()
    return result

Unlike in Java, not everything in Python needs to be a class. In Python we tend to reserve classes for representing things with individual state and common behaviour. If you just want to group some functions together, you can put them in a module.
You have no documentation. What do these functions do and how am I supposed to call them? In Python you should give every public class, function and method a docstring.
Why do you try importing simplejson? This is just the externally maintained version of the built-in json module. You don't seem to use any simplejson-specific feature, so what's the point?
Your code is not portable to Python 3, but this would be easy to fix by putting parentheses around the arguments to print.
This code:

#Check user exists
if user.json()['Data'] is None:
    return None

#Return user
return user.json()['Data']

can be simplified to:

return user.json()['Data']

You have code like this:

VARIABLE = requests.METHOD(api_root + '/accounts/' + account_id + STUFF, headers=self.headers)
VARIABLE.raise_for_status()

in three places. You should make this a function.
It's not clear what the point of this line is:

user_id = str(user_id)

All you ever do with user_id is add it to strings, where it will be converted to a string in any case.
I prefer to use str.format to build up strings, rather than +: the former tends to result in clearer code. For example:

api_root + "/accounts/" + account_id + "/users/" + user_id

would become:

'{}/accounts/{}/users/{}'.format(api_root, account_id, user_id)

Comment on updated question

You have failed to avoid the duplicated code: you have just moved it around.


Calls one column of a dataframe, turns it into an array and plots it My code calls 1 column of a dataframe, turns it into an array and plots it.
I want to able to do this for all the columns without having to repeat the code many times.

How can I do this?

#Code to create fake data
df = pd.DataFrame(np.random.rand(10,5), columns=list('abcde'),
     index=list('nopqrstuvw'))
df.columns.name = 'level_0'
df.index.name = 'level_1'

#I load an array of shape (10, 2)
sensors_pos = sio.loadmat('/home/Coord_Slp_2D.mat')['Cor']

# I extract column "a"
df1=df['Total Acc'].round(4)*100
df2=df1.reset_index().pivot('level_1', 'level_0', 'Total Acc')

df3=df2['a'].reset_index() #extracts the column "a"

#Turns column "a" into an array
data=df3['a'].values

#Plots column "a"
fig = plt.figure(figsize = (8,5))
ax,_ = plot_topomap(data, sensors_pos, cmap='viridis', show=False)
plt.title('a')
fig.colorbar(ax)
fig.show()

#------------------------------
#This is where the repetition starts. I will have to do this for each column

 # I extract column "b"

df4=df2['b'].reset_index() #extracts the column "b"

#Turns column "b" into an array
data=df3['b'].values

#Plots column "b"
fig = plt.figure(figsize = (8,5))
ax,_ = plot_topomap(data, sensors_pos, cmap='viridis', show=False)
plt.title('b')
fig.colorbar(ax)
fig.show()

I added a "for" loop. :)

#Code to create fake data
df = pd.DataFrame(np.random.rand(10,5), columns=list('abcde'),
index=list('nopqrstuvw'))
df.columns.name = 'level_0'
df.index.name = 'level_1'

#I load an array of shape (10, 2)
sensors_pos = sio.loadmat('/home/Coord_Slp_2D.mat')['Cor']

# I extract each column. From "a" to "e"
df1=df['Total Acc'].round(4)*100
df2=df1.reset_index().pivot('level_1', 'level_0', 'Total Acc')

for column in df2:
    data=df2[column].values
    fig = plt.figure(figsize = (8,5))
    ax,_ = plot_topomap(data, sensors_pos, cmap='viridis', show=False)
    fig.colorbar(ax)
    fig.show()


Compute intersections of all combinations of vectors in a list of vectors in R My goal is to compute the intersections of several vectors (sets of identifiers, gene-names to be specific). I start with a list of vectors and run the function below, which loops through 1:n where n is the number of sets and then uses combn to generate all combinations of my sets taken m at a time.

I paste together a name and then reduce by intersection over my sets yielding a named list of character vectors holding the elements in common between each combination of sets.

My question is, of course, is there a better way to accomplish this?

## Compute the intersection of all combinations of the
## elements in the list of vectors l. Might be useful
## for generating Venn/Euler diagrams.
## There might be better ways to do this!
overlap <- function(l) {
  results <- list()

  # combinations of m elements of list l
  for (m in seq(along=l)) {

    # generate and iterate through combinations of length m
    for (indices in combn(seq(length(l)), m, simplify=FALSE)) {

      # make name by concatenating the names of the elements
      # of l that we're intersecting
      name <- do.call(paste, c(as.list(names(l)[indices]), sep="_"))

      # adding the init=l[indices[1]] parameter helps with the case
      # where we're only dealing with one set, i==1 and length(indices)==1,
      # and we want only unique items in that set.
      # Reduce(intersect, list(c(1,2,3,3))) => c(1,2,3,3)
      # Reduce(intersect, list(c(1,2,3,3)), init=l[[indices[1]]]) => c(1,2,3)
      results[[name]] <- Reduce(intersect, l[indices], init=l[[indices[1]]])
    }
  }
  results
}

overlap( list(foo=c('a','b','c','d','e','e'),
              bar=c('a','c','e','f','g'),
              bat=c('a','b','c','d','g')))

Your approach seems reasonable, but there are some simplifications you can make.

First, your construction of name is needlessly complex.  This works just as well:

name <- paste(names(l)[indices], collapse="_")

Second, you can call unique on each element of l at the outset which eliminates the need to specify an init value to Reduce (and thus reducing all the calculations by one call to intersect). It also shortens the arguments to intersect since duplicates have already been eliminated.

l <- lapply(l, unique)

These two give a function

overlap <- function(l) {
  results <- list()
  # Remove duplicates within each entry of l
  l <- lapply(l, unique)

  # combinations of m elements of list l
  for (m in seq(along=l)) {

    # generate and iterate through combinations of length m
    for (indices in combn(seq(length(l)), m, simplify=FALSE)) {

      # make name by concatenating the names of the elements
      # of l that we're intersecting
      name <- paste(names(l)[indices], collapse="_")

      results[[name]] <- Reduce(intersect, l[indices])
    }
  }
  results
}

Further elimination of duplicate work would involve recognizing that higher order interactions, as you are determining them now, are repeating the intersections of the lower orders (that is foo_bar_bat first intersects foo and bar and then intersects that with bat, but the intersection of foo and bar was already determined). And "first" order interactions are just the arguments passed through unique (as they were simplified in the previous iteration).

overlap <- function(l) {
  results <- lapply(l, unique)

  # combinations of m elements of list l
  for (m in seq(along=l)[-1]) {

    # generate and iterate through combinations of length m
    for (indices in combn(seq(length(l)), m, simplify=FALSE)) {

      # make name by concatenating the names of the elements
      # of l that we're intersecting
      name_1 <- paste(names(l)[indices[-m]], collapse="_")
      name_2 <- names(l)[indices[m]]
      name <- paste(name_1, name_2, sep="_")

      results[[name]] <- intersect(results[[name_1]], results[[name_2]])

    }
  }
  results
}

If you really want to eliminate more duplicate calculations, you can assign names(l) outside both loops and length(l) outside the outer loop.


C++ determinant calculator - follow-up After following some suggestions you can find here, I'd like to show you the result:

#include <iostream>
#include <string>
#include <vector>

double getDeterminant(std::vector<std::vector<double>> vect, int dimension);

int main() {

    //First, the user has to enter the dimension of the matrix
    int dimension;
    std::cout << "Please enter dimension of Matrix: ";
    std::cin >> dimension;
    std::cout << std::endl;

    if(dimension < 0) {
        std::cout << "ERROR: Dimension cannot be < 0." << std::endl;
        return -1;
    }

    //Now, the user has to enter the matrix line by line, seperated by commas
    std::vector<std::vector<double>> vect(dimension, std::vector<double> (dimension));
    std::string str;
    for(int i = 1; i <= dimension; i++) {
        std::cout << "Enter line " << i << " only seperated by commas: ";
        std::cin >> str;
        std::cout << std::endl;
        str = str + ',';
        std::string number;
        int count = 0;
        for(int k = 0; k < str.length(); k++) {
            if(str[k] != ',') {
                number = number + str[k];
            }
            else if(count < dimension) {
                if(number.find_first_not_of("0123456789.-") != std::string::npos) {
                    std::cout << "ERROR: Not only numbers entered." << std::endl;
                    return -1;
                }
                vect[i - 1][count] = std::stod(number);
                number = "";
                count++;
            }
            else {
                std::cout << "ERROR: Too many numbers entered." << std::endl;
                return -1;
            }
        }
    }

    //Output
    for(int i = 0; i < dimension; i++) {
        for(int j = 0; j < dimension; j++) {
            std::cout << vect[i][j] << " ";
        }
        std::cout << std::endl;
    }

    std::cout << "Determinant of the matrix is : " << getDeterminant(vect, dimension) << std::endl;
    return 0;
}

double getDeterminant(std::vector<std::vector<double>> vect, int dimension) {

    if(dimension == 0) {
        return 0;
    }

    if(dimension == 1) {
        return vect[0][0];
    }

    //Formula for 2x2-matrix
    if(dimension == 2) {
        return vect[0][0] * vect[1][1] - vect[0][1] * vect[1][0];
    }

    double result = 0;
    int sign = 1;
    for(int i = 0; i < dimension; i++) {

        //Submatrix
        std::vector<std::vector<double>> subVect(dimension - 1, std::vector<double> (dimension - 1));
        for(int m = 1; m < dimension; m++) {
            int z = 0;
            for(int n = 0; n < dimension; n++) {
                if(n != i) {
                    subVect[m-1][z] = vect[m][n];
                    z++;
                }
            }
        }

        //recursive call
        result = result + sign * vect[0][i] * getDeterminant(subVect, dimension - 1);
        sign = -sign;
    }

    return result;
}

Do you have any more suggestions to improve the code?

You can find the follow-up question here.

if(dimension == 0) {
    return 0;
}

Mathematically, that is not correct. The determinant of an empty (i.e. zero-dimensional) matrix is one, see for example What is the determinant of []? on Mathematics Stack Exchange.

With respect to efficiency: Your program computes the determinant recursively using the Laplace formula, which requires \$ O(n!) \$ arithmetic operations, and the creation of many temporary “submatrices”.

A better method (at least for larger matrices) is Gaussian elimination which requires \$ O(n^3) \$ arithmetic operations, and can operate on a single copy of the original matrix.


C String Processing -- magnet link example I'm trying to get my head around C-string processing. I've decided to try my hand at making a dirt-simple magnet link generator.  The format that a magnet link needs is

magnet:?xt=<hash>&tr=tracker1url&tr=tracker2url&....&tr=trackerNurl

This program takes a hash as an argument on the command line, and takes lines on standard input which are either empty lines or the URLs of trackers.

Am I doing this in a sane way? What can I do better? Please note that I'm fairly new to C, so if you see anything weird or dangerous, let me know about that as well.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define SIZE 4096

/*
 * Exit with the given code, and print an error
 * message if ec != 0
 */
void die(int ec, char *message) {
    if (ec) {
        fprintf(stderr, "ERROR: %s\n", message);
    }

    exit(ec);
}

/*
 * takes one input argument -- the hash code
 * of the thing you're looking to generate a
 * magnet link for, on the command line, and
 * then a list of trackers on the stdin stream.
 */
int main(int argc, char **argv) {
    // If there is no
    if (argc < 2)
        die(1, "No hash code given!");

    // buf is the internal buffer. bufEnd is supposed
    // to be a pointer to the very end of the string
    // to prevent linear time string concatenation.
    char buf[SIZE];
    char *bufEnd;

    // line is a buffer for each line of standard in
    char line[SIZE];

    // 20 is the number of chars in "magnet:?xt=..."
    // magic number is bad but i don't know a better way
    if (20 + strlen(argv[1]) >= SIZE)
        die(1, "buffer overflow detected");

    // Here we initialize buffer
    bufEnd = buf + snprintf(buf, SIZE, "magnet:?xt=urn:btih:%s", argv[1]);

    // keep reading input lines, adding "&tr=..." params
    // to the magnet link for non-empty lines
    while (fgets(line, SIZE, stdin)) {
        int len = strlen(line); // find out how big this line is

        // check for empty line. skip it if it's empty
        if (!(strcmp(line, "\n"))) continue;

        // check to see if there's room left in the array
        // and if so, then add the next arg.
        if (SIZE - (bufEnd - buf + len + 4) > 0) {
            bufEnd += sprintf(bufEnd, "&tr=%s", line) - 1;
        }
        else die(1, "buffer overflow detected");
    }

    // change the trailing newline into a nul byte
    if (bufEnd - buf < SIZE) {
       *bufEnd = '\0';
    }
    else die(2, "crazy buffer overflow detected");

    // output it with backwards brackets (to show whitespace...
    // debugging purposes)
    fprintf(stdout, ">%s<\n", buf);
    fflush(stdout);
    return EXIT_SUCCESS;
}

Your code is nicely written and you have checked for errors well.  It
is perhaps a little over-commented, many of the comments explaining
the obvious (and hence being just 'noise').

String handling in C is messy and there are always many ways of
achieving the same thing.  There's nothing really wrong with using
snprintf although it can be less efficient than using strcat and
friends.  Since you are using snprintf I'll concentrate on
how you use it and ignore any alternatives.  Someone else might like
to comment on them.

You first append the command line parameter:

if (20 + strlen(argv[1]) >= SIZE)
    die(1, "buffer overflow detected");

bufEnd = buf + snprintf(buf, SIZE, "magnet:?xt=urn:btih:%s", argv[1]);

In this you have traversed argv[1] twice, once in the strlen call
and then again in snprintf.  You could avoid one of these:

const char *limit = buf + sizeof buf;
char *in = buf;

in += snprintf(in, limit-in, "magnet:?xt=urn:btih:%s", argv[1]);
if (in >= limit) {
    die(1, "buffer overflow detected");
}

This is more efficient and also avoids embedding the magic number 20
(embedded constants are usually best avoided).  It uses the fact the
snprintf returns the number of bytes needed for the string it wanted
to print but doesn't overflow the buffer.  So if it returns a number
greater than or equal to the space available , you know there was an
overflow.  I also changed bufEnd to in, used limit to mark the
end of the buffer and added braces around the call to die

Within your while loop you also traverse each string twice.  The
same technique as above can be used to avoid this.  Also your check
for blank lines does not trap lines that contains white-space as well
as a \n.  For that you might use:

const char *s = line + strspn(line, " \t\r\n");
if (*s == '\0')
    continue;


Project Euler problem 26 in Python Question statement:

  A unit fraction contains 1 in the numerator. The decimal representation of the unit fractions with denominators 2 to 10 are given:

1/2   =   0.5
1/3   =   0.(3)
1/4   =   0.25
1/5   =   0.2
1/6   =   0.1(6)
1/7   =   0.(142857)
1/8   =   0.125
1/9   =   0.(1)
1/10  =   0.1

  Where 0.1(6) means 0.166666..., and has a 1-digit recurring cycle. It can be seen that 1/7 has a 6-digit recurring cycle.

  Find the value of d < 1000 for which 1/d contains the longest recurring cycle in its decimal fraction part.

Link: https://projecteuler.net/problem=26

I tried to solve it in Python:

My approach:

For every d, assume you are solving for 1 / d by hand.
In case the 1 is completely divisible by d, it means the cycle length is 0.
If a value you have already divided appears again, it means you've found the cycle.
The cycle would have length of the number of times you've repeated the first step minus the first time the cycled value appears as we have to remove non-cycling values.

Here's my code following the above steps in Python:

LIMIT = 1000

# The maximum length
maxi = 0

# The 'd' that has maximum length
maxi_d = 1

for d in range(1, LIMIT):
    quotient = [] # Stores the decimal quotient
    cur_value = 1 # Variable used to perform division as if by hand

    len_recur = 0 # Recurring length

    # Performing division as if by hand.
    while cur_value not in quotient:
        if not cur_value:   # If the value is not recurring:
            break           # break as the rucurring length must be 0

        len_recur += 1
        quotient.append(cur_value)

        cur_value = (cur_value % d) * 10

    if not cur_value:
        continue

    # Remove number of values that do not recur
    len_recur -= quotient.index(cur_value) + 1

    if len_recur > maxi:
        maxi = len_recur
        maxi_d = d

print(maxi_d)

I'd like to make this code as fast as possible. This currently takes ~3.5 seconds for LIMIT = 2000 and grows exponentially larger.

PS: I know len_recur can be replaced by len(quotient), but I want the code to be as fast as possible.

Boosting time performance:

Starting with minor naming issues:
maxi is better named as max_len (as stated in comment). maxi_d is renamed to max_d.
It's good to move the processing into a separate function, called like calc_longest_recur_cycle.

The main time-performance hits in your implementation is caused by quotient list and its expensive list operations:

quotient.append
quotient.index
cur_value not in quotient

According to that fact that quotient and len_recur are reset on each outer for loop iteration and on the level of while loop the fragment

len_recur += 1
quotient.append(cur_value)

tells that cur_value is appended synchronously with incrementing len_recur i.e. the cur_value will get the position equal to len_recur. That means that, on this level, cur_value can be mapped to its position contained in len_recur.Thus, quotient is initiated as dictionary (instead of list): quotient = {} or even quotient = {0: 0} as AJNeufeld suggested to eliminate cur_value evaluation on the while loop condition.

The construction:

while cur_value not in quotient:
    if not cur_value:  # If the value is not recurring:
        break

is essentially a verbose equivalent to:

while cur_value and cur_value not in quotient:

List indexing

len_recur -= quotient.index(cur_value) + 1

is now efficiently replaced with dict indexing (where values are positions)

len_recur -= quotient[cur_value]

Now, with quotient as dict, we have O(1) complexity for membership check (cur_value not in quotient) and indexing operation (len_recur -= quotient[cur_value])

The final version:

LIMIT = 5000

def calc_longest_recur_cycle():
    max_len = 0   # The maximum length
    max_d = 1     # The 'd' that has maximum length

    for d in range(1, LIMIT):
        quotient = {0: 0}  # Stores the decimal quotient
        cur_value = 1      # Variable used to perform division as if by hand
        len_recur = 0      # Recurring length

        # Performing division as if by hand
        while cur_value not in quotient:  # while the value is not recurring
            len_recur += 1
            quotient[cur_value] = len_recur
            cur_value = (cur_value % d) * 10

        if not cur_value:
            continue

        # Remove number of values that do not recur
        len_recur -= quotient[cur_value]
        # quotient.clear()

        if len_recur > max_len:
            max_len = len_recur
            max_d = d

    return max_d

What needs to be mentioned is that despite of its fast nature dict would take more space than list, but only at the time of one nominal loop iteration as it's reset and last quotient filled at the end will be garbage-collected on function exit.

Let's get to tests.
I've increased limit to LIMIT = 5000 to "turn up the heat" and have put the "old" implementation into separate function calc_recur_cycle_old for comparison.
As for resulting max_d value - both functions return the same result:

print(calc_recur_cycle_old())      # 4967
print(calc_longest_recur_cycle())  # 4967

And the most interesting time performance comparison:

from timeit import timeit

print(timeit('calc_recur_cycle_old()', 'from __main__ import calc_recur_cycle_old', number=1))
19.648109548998036

print(timeit('calc_longest_recur_cycle()', 'from __main__ import calc_longest_recur_cycle', number=1))
0.26926991600339534

The end ... )