Script to print weather report from OpenWeatherMap API This script prints the weather (fetching the information from a public API).

If anyone could review my code and answer my question about code structure and how should I code in the right way, I really appreciate that very much.

Questions:

I used many functions to do just one specific things, and return their value to be used in the next functions. Is this right?
In main I used composed way for calling functions, Is this right or there is a better way?

Note: You need to obtain your API key to try it.

import datetime
import json
import urllib.request

def time_converter(time):
    converted_time = datetime.datetime.fromtimestamp(
        int(time)
    ).strftime('%I:%M %p')
    return converted_time

def url_builder(city_id):
    user_api = ''  # Obtain yours form: http://openweathermap.org/
    unit = 'metric'  # For Fahrenheit use imperial, for Celsius use metric, and the default is Kelvin.
    api = 'http://api.openweathermap.org/data/2.5/weather?id='     # Search for your city ID here: http://bulk.openweathermap.org/sample/city.list.json.gz

    full_api_url = api + str(city_id) + '&mode=json&units=' + unit + '&APPID=' + user_api
    return full_api_url

def data_fetch(full_api_url):
    url = urllib.request.urlopen(full_api_url)
    output = url.read().decode('utf-8')
    raw_api_dict = json.loads(output)
    url.close()
    return raw_api_dict

def data_organizer(raw_api_dict):
    data = dict(
        city=raw_api_dict.get('name'),
        country=raw_api_dict.get('sys').get('country'),
        temp=raw_api_dict.get('main').get('temp'),
        temp_max=raw_api_dict.get('main').get('temp_max'),
        temp_min=raw_api_dict.get('main').get('temp_min'),
        humidity=raw_api_dict.get('main').get('humidity'),
        pressure=raw_api_dict.get('main').get('pressure'),
        sky=raw_api_dict['weather'][0]['main'],
        sunrise=time_converter(raw_api_dict.get('sys').get('sunrise')),
        sunset=time_converter(raw_api_dict.get('sys').get('sunset')),
        wind=raw_api_dict.get('wind').get('speed'),
        wind_deg=raw_api_dict.get('deg'),
        dt=time_converter(raw_api_dict.get('dt')),
        cloudiness=raw_api_dict.get('clouds').get('all')
    )
    return data

def data_output(data):
    m_symbol = '\xb0' + 'C'
    print('---------------------------------------')
    print('Current weather in: {}, {}:'.format(data['city'], data['country']))
    print(data['temp'], m_symbol, data['sky'])
    print('Max: {}, Min: {}'.format(data['temp_max'], data['temp_min']))
    print('')
    print('Wind Speed: {}, Degree: {}'.format(data['wind'], data['wind_deg']))
    print('Humidity: {}'.format(data['humidity']))
    print('Cloud: {}'.format(data['cloudiness']))
    print('Pressure: {}'.format(data['pressure']))
    print('Sunrise at: {}'.format(data['sunrise']))
    print('Sunset at: {}'.format(data['sunset']))
    print('')
    print('Last update from the server: {}'.format(data['dt']))
    print('---------------------------------------')

if __name__ == '__main__':
    try:
        data_output(data_organizer(data_fetch(url_builder(2172797))))
    except IOError:
        print('no internet')

Output:

---------------------------------------
Current weather in: Cairns, AU:
14.81 °C Clear
Max: 17, Min: 10.56

Wind Speed: 1.91, Degree: None
Humidity: 100
Cloud: 0
Pressure: 1015
Sunrise at: 11:41 PM
Sunset at: 10:50 AM

Last update from the server: 11:35 PM
---------------------------------------

For your first question, yes, breaking down functions to do really specific things is always good. Makes it easier to use the same functions again in other situations etc.

For your second question, I would personally only call one function at a time, for me it makes it easier to read and understand.

Also, you can have a single print statement for a multi-line string for example:

print('''' Hello
world
      isn't this cool?'''

and that would print:

Hello
world
      isn't this cool?

but in your circumstance it might be a little bit difficult with all those format().


Spurious wakeups in Java's BlockingQueue.take() I wrote this code where I trap InterruptedException's in a blocking queue and try again to consume from the same queue thinking of spurious wakeups. I don't know if in this case I should be thinking of spurious wakeups since I am not calling wait() directly.

Basically, the worry that someone raised is that if the take throws an InterruptedException, I should stop processing (so the while should be inside the try, the opposite of what I did). There is already a mechanism to stop processing by calling a public method in the class, but I don't know if processing should also be stopped by an InterruptedException.

Does anyone have any thoughts?

private BlockingQueue<MyMessage> queue;
...
@Override
public void run() {
   log.debug("[run] Starting processing");

   while (runningController.isRunning()) {
      try {
         MyMessage message = queue.take(); // will block if empty

         assert message != null; // a blocking queue can never return null

         RuntimeMessage parsedMsg = processMessage(message);

         if (parsedMsg != null) {
            outputConsumer.messageArrived(parsedMsg);
         } else {
            log.warn("[run] Unparsed message 0x" + Integer.toHexString(message.getCommandType()));
         }

      } catch (InterruptedException exc) {
         log.error("[run] Interrupted while reading from queue", exc);
      } catch (MessageConfigurationException exc) {
         log.error("[run] Error finding structure configuration for received message", exc);
      }
   }
}

When you write, that message cannot be null, why are you asserting for it then?
That code is IMO noise. I'd rather remove it. Other than that, the comment on it is even more noisy. Your assert says exactly what your comment says. Why keep it?

Same should go for the //will block if empty. It's noise, as it adds no value to the understanding of the code. It's clear that BlockingQueue.take() will block when there's no messages, why would there be a need to write a comment on that?

Currently you catch inside your while loop. this means your execution continues, even when InterruptedException or MessageConfigurationException is thrown. You should keep that, given the case it is desired behavior. You have to decide on a case to case basis. We miss the context to decide on that, that's why its your job, isn't it ;)

If you want to stop execution when you caught an exception, just call that public method you mentioned. It's not like calls from other public members are forbidden ;)

Oh and a minor nitpick, the name for your BlockingQueue could be better, I'd probably go for messages.


Dynamic number of runnable tasks I want to execute a number of tasks that may enqueue other tasks, and do all that in parallel, ending only once all tasks are finished.
For example, some sort of crawler that starts with node A, discovers B, C, and D, then visits each of those and discovers other adjacent nodes.
If one task has an error, that error should be raised on the main thread.
My primary concerns are correctness and simplicity.
AFAIK, this is simplest way to do that, even with the new standard libraries like concurrent.futures.
import queue
import threading

class WorkerRunner:
    """
    Runner

    :param int parallelism: Number of workers to run in parallel
    :param handler: Handler that receives work and optionally yields other work
    """

    def __init__(self, parallelism: int, handler):
        self._handler = handler
        self._parallelism = parallelism

    def run(self, items):
        queue_ = queue.Queue()
        threads = []
        for _ in range(self._parallelism):
            thread = WorkerThread(queue_, self._handler)
            thread.start()
            threads.append(thread)
        for item in items:
            queue_.put(WorkItem(item))
        queue_.join()
        for _ in range(self._parallelism):
            queue_.put(END_ITEM)
        for thread in threads:
            thread.result()

"""
End item
"""
END_ITEM = None

class WorkItem:
    def __init__(self, value):
        self.value = value

class WorkerThread(threading.Thread):
    def __init__(self, queue: queue.Queue, handler):
        super().__init__()

        self._queue = queue
        self._exception = None
        self._handler = handler

    def run(self):
        try:
            self._work()
        except Exception as e:
            self._exception = e
            while True:
                try:
                    self._queue.get(False)
                except queue.Empty:
                    break
                else:
                    self._queue.task_done()
            print("here")

    def result(self):
        self.join()
        if self._exception is not None:
            raise self._exception

    def _work(self):
        while True:
            item = self._queue.get()
            try:
                if item is END_ITEM:
                    break
                result = self._handler(item.value)
                if result is not None:
                    for item in result:
                        self._queue.put(WorkItem(item))
            finally:
                self._queue.task_done()

Example
def work(x: int):
    print(x)
    while 0 < x:
        x -= 1
        yield x

runner = WorkerRunner(2, work)
runner.run([3])

# 3
# 2
# 1
# 0
# 0
# 1
# 0
# 0

Perhaps I got off track somehow, but I found run() and
_work() difficult to understand. I was puzzled by the decision to use
two methods rather than one, I did not immediately grasp why two layers
of exception handling were needed, and the intent behind
the while True loop in run() wasn't immediately obvious.
Here are a few possible edits intended to simplify and clarify. Unless
I overlooked a detail, the edits:

do little more than move your
lines of code around, putting everything under run() other than
the completely separable stuff moved to _drain_queue();
rename
result to work_items for clarity; and
add a few short comments to
provide some simple narrative guidance to any future code readers.

def run(self):
    while True:
        # Get next work item, stopping on the sentinel.
        item = self._queue.get()
        if item is END_ITEM:
            break

        # Run the handler, enqueuing any work items it yields.
        try:
            work_items = self._handler(item.value)
            if work_items:
                for item in work_items:
                    self._queue.put(WorkItem(item))

        # On error, store exception and drain queue.
        except Exception as e:
            self._exception = e
            self._drain_queue()

        # Either way, mark current task done.
        finally:
            self._queue.task_done()

def _drain_queue(self):
    while True:
        try:
            self._queue.get(False)
        except queue.Empty:
            break
        else:
            self._queue.task_done()


generic load testing tool A part of my day job is load-testing of our infrastructure. I wanted a tool to quickly write a load scenario for some application (DB, HTTP, ...).  I have tried to write something in Haskell (a fairly new language for me). Would someone help me improve by reviewing the code?
This is the main module:
module Load.Core
  ( runLoadN,
    Config (..),
  )
where

import Control.Concurrent (ThreadId, forkIO)
import Control.Concurrent.STM (STM, atomically, newEmptyTMVar, putTMVar, readTMVar)
import Control.Concurrent.STM.TBMQueue (TBMQueue, closeTBMQueue, newTBMQueue, readTBMQueue, writeTBMQueue)
import Control.Concurrent.STM.TSem (TSem, newTSem, signalTSem, waitTSem)
import Control.Concurrent.TokenBucket (newTokenBucket, tokenBucketWait)
import Control.Exception.Lifted (finally)
import Control.Monad (replicateM_, void)
import Control.Monad.Base (MonadBase, liftBase)
import Control.Monad.Trans.Control (MonadBaseControl, liftBaseWith)
import Data.Default (Default, def)
import Data.Word
import Numeric.Natural

data Config m a b c = Config
  { supplier :: m a,
    performer :: a -> IO b,
    collector :: b -> c -> c,
    workers :: Natural,
    queue :: Natural,
    rate :: Double,
    burst :: Integer
  }

runLoadN :: (MonadBase IO m, MonadBaseControl IO m, Default c) => Natural -> Config m a b c -> m c
runLoadN nat cfg = do
  let n = toInt nat
  let w = toInt $ workers cfg
  let q = toInt $ queue cfg
  let b = fromIntegral $ burst cfg
  let r = toInvRate $ rate cfg
  let perf = performer cfg
  let coll = collector cfg
  let supp = supplier cfg

  jobq <- atomically' $ newTBMQueue q
  resq <- atomically' $ newTBMQueue q
  mvar <- atomically' newEmptyTMVar
  sema <- atomically' $ newTSem 0
  tobu <- liftBase newTokenBucket

  startWorkers w perf jobq resq sema tobu r b
  _ <- forkIO' $ closing resq $ \_ -> waitForWorkers w sema
  _ <- forkIO' $ closing jobq $ supplyRequests n supp
  _ <- forkIO' $ collectResults coll def resq mvar
  atomically' . readTMVar $ mvar
  where
    collectResults f z resq mv = do
      mbNxt <- atomically' $ readTBMQueue resq
      case mbNxt of
        Nothing -> atomically' $ putTMVar mv z
        Just nxt -> let z' = f nxt z in seq z' $ collectResults f z' resq mv

    supplyRequests n supp q = replicateM_ n (supp >>= \req -> atomically' $ writeTBMQueue q req)

    waitForWorkers w sema = atomically' (replicateM_ w (waitTSem sema))

    startWorkers w perf jobq resq sema tobu r b = do
      replicateM_ w $ forkIO' $ signaling sema $ performRequests perf jobq resq tobu r b

    -- todo: what about exceptions?
    performRequests perf jobq resq tobu r b = run
      where
        run = do
          mbreq <- atomically' (readTBMQueue jobq)
          case mbreq of
            Nothing -> return ()
            Just req -> do
              res <- liftBase $ tokenBucketWait tobu b r >> perf req
              atomically' $ writeTBMQueue resq res
              run

-- performs f on the queue and closes it afterwards
--
-- use for sending requests, closing when done
closing :: (MonadBaseControl IO m) => TBMQueue a -> (TBMQueue a -> m ()) -> m ()
closing q f = f q `finally` atomically' (closeTBMQueue q)

-- performs f and signals the the semaphore afterwards
--
-- use for worker threads, signaling that they are done
signaling :: (MonadBaseControl IO m) => TSem -> m () -> m ()
signaling sem f = f `finally` atomically' (signalTSem sem)

-- some convenience methods
--
--

forkIO' :: MonadBaseControl IO m => m () -> m ThreadId
forkIO' m = liftBaseWith $ \runInBase -> forkIO (void $ runInBase m)

atomically' :: (MonadBaseControl IO m) => STM a -> m a
atomically' stm = liftBase $ atomically stm

toInt :: Natural -> Int
toInt = fromIntegral . toInteger

toInvRate :: Double -> Word64
toInvRate r = round (1e6 / r)

Repository can be found here: https://gitlab.com/michoffmann/load

From a quick glance, I would say you're on the right track. It might be worthwhile to bust up the work being done by Config into two components:

Config: the pure configuration elements; workers, queue, rate, burst; and
Env: the (effectful) environment state; supplier, performer, collector.

And then think about creating a few boiler-plate functions (not including the polymorphic Env bits):
new :: Config -> IO Env
close :: Env -> IO ()
check :: Env -> IO Text

withConfig :: Config -> (Config -> Env -> IO ()) -> IO ()
withConfig c cont = bracket (new c) close (cont c)

Once you have this, you can create a new environment, play around with it, and properly close it in a robust way. As it stands, and given my workflow preferences, I would think that runLoadN is too monolithic for easy maintenance and extension.
General descriptions of these refactors include the handle pattern and the service pattern. The three layer cake is classic also though I suspect you don't need the extra complexity.
With runLoadN freed of environment setup and teardown you'll be able to see more clearly the key components of the process.
A few vague guesses about other aspect of the code:

I would work towards refactoring away the convenience functions. Do you really need the extra monadic complexity? Can it all be IO?

the collector looks like a WriterT or logger, and might be simplified by using those patterns.

There are a lot of polymorphic types being thrown around. Both m and IO, in particular, exists in Config, which might suggest some refactoring needed. I reckon you could aim for just an Env without any polymorphism.

If you're always altering pure Config types (with toInt), then it's better to change the Config types instead.

I would suggest that best practice is to define a defaultConfig :: Config explicitly, rather than run through Data.Default and use the anonymising def pattern


Python: Use of class methods to change data Could you please give me some feedback on the use of class methods in this Python script?
The code retrieves time entries from the Toggl API and works as expected. My question is, do the methods make sense? The methods first_api_call and make_subsequent_calls change self.data. Is this okay or should all the changes to self.data be made in retrieve_data?
from datetime import datetime

import requests

today = datetime.today().date()
since = datetime(today.year, 1, 1)

class Toggl:
    def __init__(self):
        self.data = []
        self.total_calls = None
        self.params = {
            'workspace_id': '5604761',
            'since': since,
            'until': today,
            'user_agent': 'john@doe.com',
            'page': 1,
        }

    def retrieve_data(self):
        r = self.first_api_call()
        self.total_calls = self.calculate_number_of_extra_calls_needed(r)
        self.make_subsequent_calls()

    def first_api_call(self):
        r = requests.get('https://toggl.com/reports/api/v2/details',
                         params=self.params,
                         auth=('APIKEY', 'api_token'))
        r.raise_for_status()
        r = r.json()
        self.data = r['data']
        return r

    def calculate_number_of_extra_calls_needed(self, r):
        """
        Calculate the number of pages that need to be retrieved.
        The second part is True/False to round up.
        Finally minus 1 because we got the first page already.
        """
        total_calls = r['total_count'] // 50 + (r['total_count'] % 50 > 0) - 1
        return total_calls

    def make_subsequent_calls(self):
        """Make the new API calls and store the data"""
        for i in range(self.total_calls):
            self.params['page'] = self.params['page'] + 1
            r = requests.get('https://toggl.com/reports/api/v2/details',
                             params=self.params,
                             auth=('APIKEY', 'api_token'))
            r.raise_for_status()
            r = r.json()
            for d in r['data']:
                self.data.append(d)

if __name__ == "__main__":
    toggl = Toggl()
    toggl.retrieve_data()
    print(toggl.data)

It might be best to move
today = datetime.today().date()
since = datetime(today.year, 1, 1)

into the class's __init__. As it is, the code will just be run once when the module loads, whereas it would make more sense to work out the date when you construct Toggl.
I'd also say that having two methods for what is essentially the exact same task could be streamlined. Perhaps something like this?
import math

# Default page size in the Toggl API.
PAGE_SIZE = 50

class Toggl:
    def __init__(self):
        today = datetime.today().date()
        since = datetime(today.year, 1, 1)
        self.data = []
        self.total_calls = None
        self.params = {
            'workspace_id': '5604761',
            'since': since,
            'until': today,
            'user_agent': 'john@doe.com'
        }

    def fetch_page(self, page_number):
        params = self.params
        params['page'] = page_number
        r = requests.get('https://toggl.com/reports/api/v2/details',
                         params=params,
                         auth=('APIKEY', 'api_token'))
        r.raise_for_status()
        return r.json()

    def retrieve_data(self):
        r = self.fetch_page(1)
        self.data = r['data']
        pages = self.total_pages(r['total_count'])
        for i in range(2, pages + 1):
            r = self.fetch_page(i)
            self.data += r['data']

    def total_pages(self, total_count):
        return math.ceil(total_count / PAGE_SIZE)

In particular, any "magic number" constants should be taken out and put at the top as constants (by convention in SCREAMING_SNAKE_CASE). I tweaked the function names to be more descriptive, and replaced first_api_call and make_subsequent_calls with a function that doesn't affect the state: instead it just makes a call to fetch the required page and retreive_data decides what to do with it. I don't think there's anything inherently wrong with doing it as you did though, I just personally thought fetch_page seemed cleaner and reduced duplication.
You can simplify the calculation to work out how many pages you need using math.ceil as I demonstrate above too: for example 101 / 50 = 2.02, which is mapped to 3 by math.ceil.
To make the code even better, you could avoid storing the API key hard coded: take it as a parameter in the constructor and store it in self. You could also take since and today as parameters so the user can choose the range that they want to obtain.


Combining two indexOf and regex in to one I have the following code that splits a combination of names on either the word and or the & ampersand:

    var name = 'Ron & Peggy Sue'; //or 'Ron and Peggy Sue';

    if ( name.indexOf('&') > -1 ){
        names = name.split(/ & /g);
    }

    if ( name.toLowerCase().indexOf(' and ') > -1 ) {
        names = name.split(/ and /gi);
    }

Its returns an array of names like:

names = ['Ron', 'Peggy Sue']; //regardless of `&` or `and` separator

I was trying to combine these two conditions into one, but the results were not what I wanted. I know there must be a better way. How can the above code be improved?

You were almost there. You can use pipe (a|b) to designate the regex to look for both a and b. With that knowledge your split is as easy as:

var name = 'Ron & Peggy Sue and Darin Douglass';
names = name.split(/\s(?:and|&)\s/ig);

EDIT: The syntax (?:...) is whats called non-capturing parens. These match the regex provided as '...' but it does not remember the matches. The problem with the simple /\s(and|&)\s/ig/ was that the words and and & would be matched. This new syntax 'matches' them but does not remember (i.e. report) them. Thanks to @cbojar for the pointer.

Also, there is no need to check for either delimiter in name. If none exist it will simply return a single-element array with name as its element.


Optimize a regex validating a version string I develop an application which installs updates. These updates have a version id (this version id is created as a part of the application and the format is a fixed requirement) and I use a regex to parse/validate their id. Each group must be separated by a dot. The id must have 2 digits in the first group, one or two digits in the second group and the third group and at least one digit in the last group.

The regex I use is the following:

^[0-9]{2}\.[0-9]{1,2}\.[0-9]{1,2}\.[0-9]{1,}$

The regex works like expected but when I look at it I have a feeling telling me that I can make it better, smarter and and and and ...

Should I stay with this solution or how would you make it better?

The regex should work with the .NET-Framework Regex class.

Since the second and third segment are similar, you can put them in a non-capturing group with quantifier.

If you allow/want the first digit to be \$ 0 \$ in your first group, the expression is ok as it is. Otherwise, you can restrict the first digit to be [1-9].

You can have + quantifier instead of {1,} as they are essentially the same :)

^[0-9]{2}(?:\.[0-9]{1,2}){2}\.[0-9]+$

and for unpadded version matches:

^[1-9][0-9](?:\.[0-9]{1,2}){2}\.[0-9]+$


Kotlin extension functions with null checks Kotlin defines extension functions like also but they are not null checked. That's fine because you might not need the check. In case you do though, do you see any issue with the solution below?

inline fun <T> T?.ifNotNull(block: (T) -> Unit): T? {
    return this?.also {
        block(it)
    }
}

inline fun <T> T?.otherwise(block: () -> Unit) {
    this?: run {
        block()
    }
}

fun main(args: Array<String>) {
    "it".ifNotNull {
        println("You can use $it")
    }.otherwise {
        // will not happen
    }

    null.ifNotNull {
        // will not happen
    }.otherwise {
        println("You don't see it")
    }

}

OK, I'll give my answer. One problem I had trying this approach is that there are too many combinations: there are cases when you want to use also cases when you want to use let etc.
It looks like having the operators and the extension functions independently defined gives you the advantage of a kind of strategy pattern when you can combine different strategies to have an exponential number of configuration without having to define an exponential number of functions.

So you have

Operators:

 1. `?.`
 2. `?:`

Extension functions

a) `also`
b) `let`
c) `run`
d) etc

then you can combine operator 1. with extension fun b) or operator 2. with extension fun c) etc

If you try to define extension funs with null checks then you need to define one function for each combination: it's even difficult just to find names for the functions

So I will keep the otherwise helper fun but I will not use if ifNotNull


Boost::range::for_each + functor I would like your comments on my code in C++ (C++98 + Boost)

I have written some code to use the functors with the boost::range::for_each algorithm.
My main problem was to add some "external" parameters to the functor that are not in the vector on which I am using a for_each loop.

In main() we have:

std::vector<Person> vec;
vec.reserve(MAX_SIZE);
std::map<std::string, int> result;
FillPersons(vec);
boost::range::for_each(vec,Treatment(&result));

The functor is:

struct Treatment {
    Treatment(std::map<std::string, int> *p_result):result_(p_result){}
    void operator ()(Person &person)
    {
        if (NULL != result_)
        {
            int age = person.getAge();

            if (age < AGE_LOW_LEVEL)
            {
                age += 2;
            }
            else if ((age >= AGE_LOW_LEVEL) && (age < AGE_HIGH_LEVEL))
            {
                age += 1;
            }
            else if (age >= AGE_HIGH_LEVEL)
            {
                age -= 5;
            }
            result_->insert(std::pair<std::string, int> (person.getName(), age));
        }
    }
    private:
    std::map<std::string, int> *result_;
    Treatment(const std::map<std::string, int> &);
    Treatment& operator=(const Treatment&);
};

Is there any other way to do that better or is it good?
How would it be using lambdas?
How would it be in C++11?

For your information, the Treatment function is naive because my focus was only on adding external parameters to the functors.

First of all I would not use boost::range::for_each . There is already standard algorithm std::for_each in C++.

Secondly I would not use a lambda-expression if the algorithm with the predicate is used more than one time.

And if it is used only one time I would use the range-based for statement. For example

for ( const Person &person : vec )
{
    int age = person.getAge();

    if ( age < AGE_LOW_LEVEL )
    {
        age += 2;
    }
    else if ( age < AGE_HIGH_LEVEL )
    {
        age += 1;
    }
    else
    {
        age -= 5;
    }

    result.insert( { person.getName(), age } );
}


Read JPEG dimensions properly I need to extract the dimensions (width and height) of a jpeg file for inserting in an html document. According to the JPEG image compression FAQ, cautions against what's commonly out there,

  Some people have recommended just searching for the byte pair
  representing SOFn, without paying attention to the marker block structure.
  This is unsafe because a prior marker might contain the SOFn pattern, either
  by chance or because it contains a JPEG-compressed thumbnail image.

It said to look up rdjpgcom.c, which is part of the open-source ImageMagick. It seems to agree with the advice on this answer. However, this file prints out jpeg comments, so I wrote one to specifically get the dimensions.

#include <stdlib.h> /* EXIT */
#include <stdio.h>  /* [f]printf fopen fclose fread */
#include <assert.h> /* assert */
#include <errno.h>  /* errno */

/** Attempt to read the size of a jpeg.
 @param[file] File that has been opened in binary mode and rewound; required.
 @param[width, height] Pointers that get overwritten on success; required.
 @return Success, otherwise `errno` will (probably) be set.
 @license <http://www.faqs.org/faqs/jpeg-faq/part1/> said to look up
 [rdjpgcom.c](https://github.com/ImageMagick/jpeg-turbo/blob/master/rdjpgcom.c),
 which has
 [IJG License](https://github.com/ImageMagick/jpeg-turbo/blob/master/README.ijg)
 and possibly related [ImageMagick](https://imagemagick.org/script/license.php).
 I used as a reference to write this function. */
static int jpeg_dim(FILE *const fp, unsigned *const width,
    unsigned *const height) {
    unsigned char f[8];
    unsigned skip;
    assert(fp && width && height);
    /* The start of the file has to be an `SOI`. */
    if(fread(f, 2, 1, fp) != 1) return 0;
    if(f[0] != 0xFF || f[1] != 0xD8) return errno = EDOM, 0;
    for( ; ; ) {
        /* Discard up until the last `0xFF`, then that is the marker type. */
        do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] != 0xFF);
        do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] == 0xFF);
        switch(f[0]) {
        case 0xC0: case 0xC1: case 0xC2: case 0xC3: case 0xC5: /* _sic_ */
        case 0xC6: case 0xC7: case 0xC9: /* _sic_ */ case 0xCA: case 0xCB:
        case 0xCD: /* _sic_ */ case 0xCE: case 0xCF:
            /* `SOF` markers. */
            if(fread(f, 8, 1, fp) != 1) return 0;
            if((skip = (f[0] << 8) | f[1]) != 8u + 3 * f[7])
                return errno = EDOM, 0;
            *width  = (f[5] << 8) | f[6];
            *height = (f[3] << 8) | f[4];
            return 1;
        case 0xD8: case 0xD9:
            /* Image data `SOS, EOI` without image size. */
            return errno = EDOM, 0;
        default:
            /* Skip the rest by reading it's size. */
            if(fread(f, 2, 1, fp) != 1) return 0;
            if((skip = (f[0] << 8) | f[1]) < 2) return errno = EDOM, 0;
            if(fseek(fp, skip - 2, SEEK_CUR) != 0) return 0;
        }
    }
}

int main(int argc, char **argv) {
    FILE *fp = 0;
    unsigned width, height;
    int success = EXIT_FAILURE;
    if(argc != 2 || !(fp = fopen(argv[1], "rb"))
        || !jpeg_dim(fp, &width, &height)) goto catch;
    printf("Jpeg width: %u, height: %u.\n", width, height);
    success = EXIT_SUCCESS;
    goto finally;
catch:
    if(errno) {
        perror("jpeg size");
    } else {
        fprintf(stderr, "Use with jpeg filename to see the size.\n");
    }
finally:
    if(fp) fclose(fp);
    return success;
}

It seems to work, but I'd like to review it to make sure that it's correct.

(kind of nitpicking here, it looks quite solid to me)

Code:

    /* Discard up until the last `0xFF`, then that is the marker type. */
    do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] != 0xFF);
    do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] == 0xFF);

While we might have extra 0xff fill to read, I'm not sure we should be reading and discarding non-0xff bytes. If the data in the file doesn't follow the expected format our assumptions about the marker / chunk structure might be wrong too, which could lead to invalid results.

Outputting the wrong size could cause more problems than returning an error, so I'd be inclined to pick the latter instead.

        if((skip = (f[0] << 8) | f[1]) != 8u + 3 * f[7])
            return errno = EDOM, 0;

No need to set skip here, since we return in both branches.

We're checking the SOF chunk length matches with the number of image components? It might be nice to have a comment or something to mention this.

Using errno seems unnecessary if we return success / failure in the return value (and the only error code used  EDOM).

Style:

Some of the compound statements seem a bit forced. e.g.

e.g.:

if(argc != 2 || !(fp = fopen(argv[1], "rb"))
    || !jpeg_dim(fp, &width, &height)) goto catch;

If we split this up, the flow becomes much simpler, we can introduce local variables later, and we don't need multiple branches in the error handling:

int main(int argc, char **argv) {

    if (argc != 2) {
        fprintf(stderr, "Use with jpeg filename to see the size.\n");
        return EXIT_FAILURE;
    }

    FILE* fp = fopen(argv[1], "rb"); // note: we only need this if argc was correct
    if (!fp) {
        perror("jpeg size"); // note: fopen() may not set errno on non-POSIX platforms
        return EXIT_FAILURE;
    }

    unsigned width, height; // note: we only need these if fopen worked
    if (!jpeg_dim(fp, &width, &height))
    {
        perror("jpeg size"); // note: errno isn't very helpful here... we could return an error code instead
        return EXIT_FAILURE;
    }

    printf("Jpeg width: %u, height: %u.\n", width, height);

    fclose(fp);

    return EXIT_SUCCESS;
}

There are quite a few repeated statements in jpeg_dim (fread and stuff like ((f[5] << 8) | f[6]). These are quite concise, but should probably still be factored into separate functions.

I'd suggest something like a jpeg_read_u8, jpeg_read_u16 and jpeg_skip:

static int jpeg_read_u8(FILE *const fp, unsigned char *const u8) {
    assert(u8);

    unsigned char b;
    if (fread(&b, 1, 1, fp) != 1)
        return 0;

    *u8 = b;
    return 1;
}

static int jpeg_read_u16(FILE *const fp, unsigned short *const u16) {
    assert(u16);

    unsigned char b[2];
    if (fread(&b, 1, 2, fp) != 2)
        return 0;

    *u16 = (b[0] << 8) | b[1];
    return 1;
}

static int jpeg_skip(FILE *const fp, unsigned bytes) {
    return fseek(fp, bytes, SEEK_CUR) == 0;
}

static int jpeg_dim(FILE *const fp, unsigned *const width, unsigned *const height) {
    assert(fp);
    assert(width && height);

    /* The start of the file has to be an `SOI`. */
    unsigned char soi0; if (!jpeg_read_u8(fp, &soi0) || soi0 != 0xff) return 0;
    unsigned char soi1; if (!jpeg_read_u8(fp, &soi1) || soi1 != 0xd8) return 0;

    for (; ; ) {
        /* Read first byte of next marker */
        unsigned char m0; if (!jpeg_read_u8(fp, &m0) || m0 != 0xff) return 0;
        /* Discard fill characters */
        unsigned char m1; do { if (!jpeg_read_u8(fp, &m1)) return 0; } while (m1 == 0xff);

        switch (m1) {
        case 0xC0: case 0xC1: case 0xC2: case 0xC3: case 0xC5: /* _sic_ */
        case 0xC6: case 0xC7: case 0xC9: /* _sic_ */ case 0xCA: case 0xCB:
        case 0xCD: /* _sic_ */ case 0xCE: case 0xCF: {
            /* `SOF` markers. */
            unsigned short Lf; if (!jpeg_read_u16(fp, &Lf)) return 0;
            if (!jpeg_skip(fp, 1)) return 0; /* skip Precision */
            unsigned short Y;  if (!jpeg_read_u16(fp, &Y)) return 0;
            unsigned short X;  if (!jpeg_read_u16(fp, &X)) return 0;
            unsigned char Nf;  if (!jpeg_read_u8(fp, &Nf)) return 0;
            /* Check that the chunk length (Lf) matches the number of image components (Nf) */
            if (Lf != 8u + 3 * Nf) return 0;
            *width = X; *height = Y;
            return 1;
        }
        case 0xD8: case 0xD9:
            /* Image data `SOS, EOI` without image size. */
            return 0;
        default: {
            /* Skip the rest by reading it's size. */
            unsigned short Lf;
            if (!jpeg_read_u16(fp, &Lf) || Lf < 2) return 0;
            if (!jpeg_skip(fp, Lf - 2)) return 0;
        }
        }
    }
}


Palindrome check in Haskell I am learning Haskell and I am doing 99 Haskell problems. There is a problem to check if a list if a palindrome. It's obvious solution is

isPalindrome x = x == reverse x

There are several other solutions with some kind of reverse. All of them use two passes -- one for building reverse list and one more for ==. I wanted to write a single pass palindrome check.

walk :: (Eq a) => [a] -> Int -> Either (Either Int [a]) Bool
walk (x:xs) n = case walk xs (n + 1) of
    Left (Left back) | (back + 1) == n -> Left (Right xs)
                     | back == n -> let (y:ys) = xs in if y == x then (if null ys then Right True else Left (Right ys)) else Right False
                     | otherwise -> Left (Left (back + 1))
    Left (Right (y:ys)) -> if y == x then (if n > 0 then Left (Right ys) else Right True) else Right False
    Left (Right []) -> error "Impossible"
    Right ans -> Right ans

walk [] _ = Left (Left (-1))

isPalindrome :: (Eq a) => [a] -> Bool
-- isPalindrome x = x == reverse x
isPalindrome [] = True
isPalindrome [_] = True
isPalindrome x = ans where (Right ans) = walk x 0

This looks quite ugly. Is there a better way to check if a string is a palindrome using only one pass? Is it worth trying to write a single pass check?

From the readability point of view, I'd suggest to keep some maximum line length, like 72 or 80 characters. Beyond that it's difficult to read. Also instead of having Either (Either X Y) Z, declare your own data type with 3 constructors. Not only it'll be simpler, but the constructor names will also be more descriptive and it'll be easier to understand what's going on. You should also document the function better, it's not clear what the Int argument is without thoroughly inspecting the code. A small improvement is also replacing nested ifs with a case statement.

In general, I'd say that it's not really possible to make a genuine single-pass solution.

Obviously we can't avoid checking the last element, if the list is a palindrome, and we need to have the first element to compare it to. So we'll always have to either get to the last element at the beginning, or keep the elements as we traverse the list, to have them for comparison when we get to the end,  So we can't get to the situation when we just traverse the list from the beginning to the end, releasing the elements already traversed.

In your case, you're also traversing the list twice, although it's somewhat hidden. The first pass is the recursive call to walk, which gets to the end of the list. And the second traversal is occurring in the line

Left (Right (y:ys)) -> if y == x then
                            (if n > 0 then Left (Right ys) else Right True)
                          else Right False

where we're traversing ys as walk returns to its parent call.

(The above nested ifs can be rewritten as

  ... -> case () of
           _ | y /= x    -> Right False
             | n > 0     -> Left (Right ys)
             | otherwise -> Right True

using case.)

Furthermore, since walk calls itself recursively and examines the result of the recursive call, it can't be optimized to tail recursion, so you're building a chain of n calls on the stack.

Another thing to notice is that the whole original list is kept in memory until the whole chain of walks finishes.

My attempt to solve this problem would look like this:

isPal' :: (Eq a) => [a] -> Bool
isPal' xs = f [] xs xs
  where
    f ss xs []              = ss == xs
    f ss (_:xs) [_]         = ss == xs
    f ss (x:xs) (_:_:es)    = f (x:ss) xs es

We're traversing the list at two different "speeds" at the same time in a tail-recursive loop, to get to the middle. During that we compute the reverse of the traversed part in ss. When we hit the middle after n/2 steps, we just compare the accumulated reversed first half with the rest. The combined length of ss and xs is always n and if we find out in the middle a difference, like in "aaabcaaa", we finish and release resources early.

Since all suggested algorithms are O(n), it's hard to decide which one will be more efficient just by reasoning. We'd have to do some measurements, for example using the criterion package.

Update: The problem could be actually solved in a genuine one pass using a rolling checksum, at the cost of having false positives in (rare) cases of hash collisions. Just compute two rolling checksums while traversing the list, one forwards and one backwards, and compare them at the end.

 import Control.Arrow ((&&&), second)
 import Data.Foldable (foldMap)
 import Data.Function (on)
 import Data.Hashable
 import Data.Monoid
 import Data.Word

 -- see https://en.wikipedia.org/wiki/Rolling_hash
 -- and https://en.wikipedia.org/wiki/Lehmer_random_number_generator#Parameters_in_common_use
 modulus :: Word64
 modulus = 2^31 - 1

 expG :: Word64
 expG = 7^5

 data RKHash = RKHash { rkExp :: Word64, rkVal :: Word64 }
   deriving (Eq, Show)

 inject :: (Hashable a) => a -> RKHash
 inject = RKHash expG . (`mod` modulus) . fromIntegral . hash

 instance Monoid RKHash where
     mempty = RKHash 1 0
     mappend (RKHash e1 v1) (RKHash e2 v2) =
         RKHash ((e1 * e2) `mod` modulus) ((v1 * e2 + v2) `mod` modulus)

 isPalindrome :: (Hashable a) => [a] -> Bool
 isPalindrome = uncurry (on (==) rkVal) . second getDual
                . foldMap ((id &&& Dual) . inject)


Views Counter made in Python, Gevent and MongoDB I've created a Views Counter in Python, Gevent and MongoDB (Flask is also included in the full stack as you can see from the context issue in the code).

My gut still tells me that it can be still somehow improved though.

What the code does is initializing a dict "buffer" (just a shelve persisted in memory [no writeback]), define a function where Mongo's bulk_op is initialized, a list comprehension that iterates over the view_count buffer and sets individual find-updates for them with their respective key values that is then ended by an execute with write concern disabled.

Then there's the run_buffer_op function that runs a loop infinitely, which checks if the buffer has over 5000 items, and in that case, flushes its content to the database (by executing the former function), otherwise it just waits 15 minutes before flushing.

This function is finally run (or better, spawned) by Gevent.

Do you see some possible further improvements?

'''
Views counter buffer
====================
'''
# Initialize vc buffer (ObjectId + Views_count pair). We don't need **writeback** as it should just be persisted in memory
view_count=shelve.open('view_count', writeback=False)

def flush_to_db():
  # Apparently you have to set the context to make the db bulk operation work, otherwise it'll return a *Working outside of context* error :(
  with app.app_context():
    bulk = mongo.db.test.initialize_unordered_bulk_op()
    # What this list comprehension does is iterating over the *view_count* buffer
    # and set individual find-updates for them with their respective key value.
    # Like: bulk.find({'_id': '7rhf3d32dh23jd78988ej8'}).update({'$set':{'count':2}})
    #       bulk.find({'_id': '7rhf3d32dh23dg48988ej8'}).update({'$set':{'count':10}})
    #       bulk.find({'_id': '7rhf3d32th23dg48988ek9'}).update({'$set':{'count':7}})
    #       ...

    [bulk.find({ '_id': k }).update({'$set':{'count': v }}) for k,v in view_count.iteritems()]

    # Execute the bulk update with no **write concern**. We can afford to lose some views count if that ever happens in change of better performance.
    bulk.execute(write_concern=None)

def run_buffer_op():
  # Run the loop infinitely
  while True:
    # If buffer has more than 5000 items flush it now to the db
    if len(view_count) > 5000:
      flush_to_db()
    # Else just wait 15 minutes
    gevent.sleep(900)
    flush_to_db()

# Spawn the loop
gevent.spawn(run_buffer_op)

Instead of using a list comprehension, I would simply use a for-loop. It will do the exact same thing and save having to allocate a temporary list.

Also, in run_buffer_op, you first check the number of views. If that value isn't >5000 (almost a DBZ joke) then you sleep for 15min. Once it wakes up, you IMMEDIATELY flush_to_db no matter what. Based on your comments,  it seems you only want to flush_to_db if there are >5000 views. Thus, the flush_to_db call immediately after the gevent.sleep(900) seems a little redundant.


What should I change in my currency converter? I wrote a simple currency converter in Python. Right now, my currency
  converter converts using the last available rate or converts using a
  specific date.

Looking for:

I'd appreciate any tips about how I can improve my little script.
What new features would be nice to add to my project?

Many thanks!

PS. I'm very sorry for my bad English, I'm not a native speaker.

from currency_converter import CurrencyConverter
from datetime import date
from colorama import Fore
import datetime

class CC():
    def __init__(self):
        self.features = ["1. Convert using the last available rate.", "2.
Convert using specific dates."]
        print("")
        print(Fore.GREEN + "-" * 76)
        print("")
        for x in self.features:
            print(x)
        print("")
        self.select = input("Select a feature: ")
        print("")
        print(Fore.GREEN + "-" * 76)
        print("")
        self.amount = input("Amount: ")
        self.c1 = input("Currency1: ")
        self.c2 = input("Currency2: ")
        self.c = CurrencyConverter()
        if self.select == "2":
            self.year = int(input("Year: "))
            self.month = int(input("Month: "))
            self.day = int(input("Day: "))

    def input_process(self):
        if self.select == "1":
            self.d = round(self.c.convert(self.amount, self.c1, self.c2), 2)
            self.x = datetime.datetime.now()
            print(f"{self.amount} {self.c1} is {self.d} {self.c2} on {self.x}")
        elif self.select == "2":
            self.d = round(self.c.convert(self.amount, self.c1, self.c2, date=date(self.year, self.month, self.day)))
            print(f"{self.amount} {self.c1} was {self.d} {self.c2} in {self.year}-{self.month}-{self.day}")

if __name__ == '__main__':
    currencies = ["USD", "SEK", "GBP", "EUR"]
    print(Fore.GREEN + f"This is a Currency converter, the main currencies is {', '.join(currencies)}")
    print("")
    print(Fore.GREEN + "-" * 76)
    print("")
    while True:
        test = CC()
        test.input_process()
        print("")
        print(Fore.GREEN + "-" * 76)
        print("")
        if input("Finished. Do another? (Y/N)\n").lower() != 'y':
            break

First, there's very little point in using a class like you are here. You're essentially just using the constructor as a function to ask for input. You're also needlessly making some variables attributes of the object, like self.features, self.x, and self.d. The only use of features is to be iterated over within the constructor, and the only use of the latter two are inside of input_process. Even if this were an appropriate use of a class, that data doesn't need to be retained in the object itself. That just wastes memory.

To remedy this, I'd take everything in __init__, and move it into a regular function. To store the data, I'd use a NamedTuple returned from the function.

Your prints are also needlessly verbose. You're using print("") as a way to print newlines. Newlines can simply be added using "\n" though.

print("")
print(Fore.GREEN + "-" * 76)
print("")

Can simply be:

print("\n" + (Fore.GREEN + "-" * 76) + "\n")

And since you use (Fore.GREEN + "-" * 76) multiple times, I'd save that into a string at the top of the file:

SEPARATOR = "\n" + (Fore.GREEN + "-" * 76) + "\n"

Your loop over features is also unnecessary. It can be done simply using join:

print("\n".join(features))

Your names are also poor. c1, c2, c, x and d are very vague and give little information about what they hold. I would make those names much longer.

After making the mentioned changes, and I ended up with:

from currency_converter import CurrencyConverter
from datetime import date
from colorama import Fore
import datetime
from typing import NamedTuple, Optional, Tuple

# These are constants. They should be defined externally, and have capitalized names
FEATURES = ["1. Convert using the last available rate.", "2. Convert using specific dates."]
SEPARATOR = "\n" + (Fore.GREEN + "-" * 76) + "\n"

class ConversionInformation(NamedTuple):  # A simple class to hold information
    feature_type: str
    amount: str
    source_currency: str
    target_currency: str
    conversion_date: Optional[Tuple]

def ask_for_input() -> ConversionInformation:
    print(SEPARATOR)
    print("\n".join(FEATURES))

    feature_type = input("Select a feature: ")

    print(SEPARATOR)

    amount = input("Amount: ")
    source_cur = input("Currency1: ")
    target_cur = input("Currency2: ")

    conversion_date = None

    if feature_type == "2":
        conversion_date = (int(input("Year: ")),
                           int(input("Month: ")),
                           int(input("Day: ")))

    return ConversionInformation(feature_type, amount, source_cur, target_cur, conversion_date)

def process_input(info: ConversionInformation):
    conv = CurrencyConverter()  # This wasn't needed when asking for input

    if info.feature_type == "1":
        # Note the more descriptive names
        result = round(conv.convert(info.amount, info.source_currency, info.target_currency), 2)
        cur_date = datetime.datetime.now()

        print(f"{info.amount} {info.source_currency} is {result} {info.target_currency} on {cur_date}")

    elif info.feature_type == "2":
        result = round(conv.convert(info.amount, info.source_currency, info.target_currency,
                                    date=date(*info.conversion_date)))

        print(f"{info.amount} {info.source_currency} was {result} {info.target_currency} in" +
              "-".join(str(d) for d in info.conversion_date))

    else:
        print("Illegal Feature Type:", info.feature_type)  # Tell the user that they entered an illegal type

# Also a constant
CURRENCIES = ["USD", "SEK", "GBP", "EUR"]

if __name__ == '__main__':
    print(Fore.GREEN + f"This is a Currency converter, the main currencies are {', '.join(CURRENCIES)}")
    print(SEPARATOR)

    while True:
        info = ask_for_input()
        process_input(info)

        print(SEPARATOR)

        if input("Finished. Do another? (Y/N)\n").lower() != 'y':
            break

It's far more verbose, but I believe it's much cleaner and easier to understand.

My main issue with what I've done here is package all the data into ConversionInformation just so it can be passed into process_input. You may find that it ends up being cleaner to make this all one large function.


Simple blog using react redux react-router I've implemented the logic of my app, but I'm not sure whether it appropriate way or not.

Task: create a react app using redux, react-router v4 which has a page with a form for adding new post and list of posts

and a page with a detail view of one of the posts

I've added a unique id for each post with help of uuid and pass id of each post to the corresponding link(react-router-dom link) in the post list

      <Link to={props.id} className='post__header'>{props.title}</Link>

When link is clicked it passes id to the DetailView component. DetailView component takes id and seek the post with the same id in the whole state then render the post

import React from 'react';
import { connect } from 'react-redux';

let DetailView = (props) => {
  console.log(props);
  var currentPost = props.posts.find(post => post.id === props.match.params.id);

  return (

{currentPost.topic}

{currentPost.text}

  )
}

const mapStateToProps = (state) => {
  return state;
};

DetailView = connect(mapStateToProps)(DetailView);

export default DetailView;

Github link: https://github.com/LKTN/Branch/tree/dev

Looks good after a quick skim. Just one point. I think this project could benefit from following the Container Component pattern so that the markup components don't directly depend on redux and are reusable.

Read more at https://medium.com/@learnreact/container-components-c0e67432e005


Very slow Project Euler Q3 (largest prime factor of a large number) My code returns the correct answer for the example (highest prime of  13195 is 29), but when I test against the number in the question my code basically is too slow for my liking.

  The prime factors of 13195 are 5, 7, 13 and 29.

  What is the largest prime factor of the number 600851475143?

Is there anything obviously wrong with my logic or any ways to improve?

(function(){

var theNumber = 600851475143; // testing this number, sub in 13195 to verify

var getHighestPrimeFactor = function(number){
    var upto = (number / 2) % 2 == 0 ? (number/2) : Math.floor(number/2);
    var factors = [];

    for(var i = upto; i > 1; i--){
        if(number % i == 0){
            factors.push(i);
            if(isPrimeFactor(i) == true){
                console.log("high prime = " + i);
                return i;
            }
        }
    }
} // end getHighestPrimeFactor

var isPrimeFactor = function(number){
    var upto = number / 2;
    var isItPrime = true;
    for(var i = 2; i < upto; i++){
        if(number % i == 0){
            isItPrime = false;
            break;
        }
    }
    return isItPrime;
} // end isPrimeFactor

var x = getHighestPrimeFactor(theNumber);
console.log(x);

})();

Similar questions have been asked before.  You are using the "test the largest candidate factors first" strategy, which does not scale well at all.  You should test the smallest candidate factors first.


Sublime Text plugin to modify text before it's pasted It's my first plugin and it's super simple but there could easily be some things worth knowing from this e.g. Python coding practices and potential bugs.

import sublime
import sublime_plugin

class EscapeAndPasteCommand(sublime_plugin.TextCommand):
    def run(self, edit, **kwargs):
        # Get the clipboard text
        originalText = textToPaste = sublime.get_clipboard()
        # Loop through the arguments making changes where possible
        for replacement in kwargs:
            # print("replacing '" + replacement + "'' with '" + kwargs[replacement] + "'")
            textToPaste = textToPaste.replace(replacement,kwargs[replacement])
        # Place the text back into the clipboard and paste in place
        sublime.set_clipboard(textToPaste)
        self.view.run_command("paste")

        # Restore the original text to the clipboard
        sublime.set_clipboard(originalText)

The idea is you feed this a dictionary of matches and replacements that it will perform on the clipboard object. Once those changes are made it restores the untouched text to the clipboard.

I made a key binding for this so that when I am pasting text, in my markdown files, that has file paths it will autoescape it for me, such as underscores and slashes that are meant to be just that.

{ "keys": ["ctrl+shift+v"],
    "command": "escape_and_paste",
    "args": {
        "\\": "\\\\",
        "_" : "\\_",
        "*" : "\\*"
    }
}

It works well for me and I think I would be lost without it now as it makes my documentation processes easier.

If your intention is to escape certain characters by prefixing them with backslashes, you would be better off with a function that performs that specific task, rather than a generalized function that accepts a dictionary of replacements.

In general, performing multiple substitution passes over a string is a bad idea.  Certain substitution dictionaries could result in non-deterministic behaviour, depending on the order in which the substitutions are performed.  In fact, you have just that problem:

If the _ escape is performed before the \ escape, then _ would become \_, then \\_.
If the \ escape is performed before the _ escape, then _ would remain _, then become \_.

You could overcome that problem by specifying the substitutions using a collections.OrderedDict, but I still recommend against it on principle.  Escaping / unescaping is always better done using a single left-to-right pass.


java code design puzzle for common function result module I am trying to solve a puzzle. I came up with a few classes to represent results. They looked similar, so I defined a common Result class. But what if I need to define different results according to different functions?

In other words, do I need TypeOneResult and TypeTwoResult or only CommonResult?

If I get rid of the different result classes and rely on CommonResult, how can I store some specific information about the result?

public class Result {
    private String id;
    private String data;

    public Result(String id, String data) {
        super();
        this.id = id;
        this.data = data;
    }
}

class CommonResult extends Result{
    private String type;

    public CommonResult(String id, String data, String type) {
        super(id, data);
        this.type = type;
    }
}

class TypeOneResult extends CommonResult{

    public TypeOneResult(String id, String data, String type) {
        super(id, data, type);
    }
}

class TypeTwoResult extends CommonResult{

    public TypeTwoResult(String id, String data, String type) {
        super(id, data, type);
    }
}

I'm not sure I understood your question fully, but from what I could understand, you do not need to have both TypeOneResult and TypeTwoResult.  In fact, you probably do not need either of them.  You can use the value of the type member of the CommonResult class to distinguish among the different types.


What is a better (more testable) way to structure this page-specific JS? I've created this (very) contrived example that demonstrates how we currently organize page specific JS in our app. Basically each page has a matching JS file that uses the module pattern to enclose page specific behaviors. Each module has an init() function that takes control IDs and wires up events, etc.

This has worked fairely well for us, however now we are wanting to add unit tests. Since many of our modules store state in private variables our unit tests end up stomping all over each other.

I think we need to stop using these singleton modules and instead use instances so that we can write better tests. What are some ways we could restructure this type of code so that it would be testable?

JS Fiddle here...

HTML

  <input type="file" />

  <input type="file" />

  <input type="file" />

  <input id="btnUpload" type="button" value="Upload Files" />
  or Clear files

JS

MyPage = (function() {
    // private members
    var _files = [];
    var _initOptions;

    // public members
    return {
        init: function(options) {
            _initOptions = options;

            $(function() {
                // wire up the events
                $(options.clearLink).click(MyPage.clear);
                $(options.uploadBtn).click(MyPage.doUpload);
                $(options.fileCtrls).change(MyPage.addFile);
            });
        },

        addFile: function() {
            _files.push($(this).val());
        },

        doUpload: function() {
            alert('uploading ' + _files.length + ' files...');
        },

        clear: function() {
            _files = [];
            $(_initOptions.fileCtrls).val('');
        }
    }
})();

MyPage = (function() {
    var Page = {
        addFile: function(e) {
            this._files.push($(e.target).val());
        },

        doUpload: function() {
            alert('uploading ' + this._files.length + ' files...');
        },

        clear: function() {
            this._files = [];
            $(this.options.fileCtrls).val('');
        }
    }

    // public members
    return {
        init: function(options) {
            var o = Object.create(Page, {
                options: { value: options },
                _files: { value: [] }
            });

            $(function() {
                // wire up the events
                options.clearLink.click($.proxy(o.clear, o));
                options.uploadBtn.click($.proxy(o.doUpload, o));
                options.fileCtrls.change($.proxy(o.addFile, o));
            });
        },

    }
})();

Using Object.create

Live Example

ES5-shim for browser support.

As for testing use any old testing framework you like. Use jQuery.sub() to mock out jQuery


Using a subquery to rename columns This is a pattern in SQL queries that I've found myself repeating recently:

SELECT
    w,
    x,
    y,
    (w + x) / y as z
FROM
  (SELECT
      <some gigantic and complicated query> as w,
      a + b as x,
      a - b as y
   FROM
      basetable) somealias;

The issue is:

a select query that does some complex operations, resulting in a column with a new name
directly reusing the "new" column doesn't work in MySQL (unsure about other RDBMS's)

This code fails:

SELECT
    <some gigantic and complicated query> as w,
    a + b as x,
    a - b as y,
    (w + x) / y as z
FROM
    basetable;

Because MySQL doesn't allow column aliases (x and y) to be re-used in the same select query.

The solution I'm using:

wrap the select in a subquery
this renames the columns
use the new column names from the outer query

I'm unsure whether it's a best practice, or a common practice, and thus easily understandable for other programmers.

As long as the variables are well-named, I think your way (subquery) will be clear and therefore quite acceptable, though I'm not quite sure whether it will affect performance or to what extent (try benchmarking).

That said, I'd still urge you to use a prepared query  or stored procedure or function, I think that would make it even more clear, and potentially faster.


Creates a Utilization Report I have this code that generates a utilization report for a company I am interning for that uses a time-tracking app called Harvest.

var config = require('./config');
var request = require('request');
var moment = require('moment');
var pageNumber, pages;
var userArray = [];
var iteration = 0;
var pageNumber = 1;

let reqURL = 'https://api.harvestapp.com/v2/users?page=1&per_page=100';
let reqURL2 = 'https://api.harvestapp.com/v2/time_entries?from=' + new moment().day(-1).format('YYYY-MM-DD') + '&to=' + new moment().day(5).format('YYYY-MM-DD') + '&page=1&per_page=100';

var options = {
    url: reqURL,
    headers: {
      'Authorization': 'Bearer' + ' ' + config.token,
      'Harvest-Account-Id': config.accountID,
      'User-Agent': config.userAgent
    }
  };

  var options2 = {
    url: reqURL2,
    headers: {
      'Authorization': 'Bearer' + ' ' + config.token,
      'Harvest-Account-Id': config.accountID,
      'User-Agent': config.userAgent
    }
  }

  function callback1(error, response, body) {

    if (!error && response.statusCode == 200) {
      info = JSON.parse(body);
      pages = info.total_pages;

      for(x = 0; x < info.users.length; x++) {
        userArray[iteration] = new Object(),
        userArray[iteration].id = info.users[x].id,
        userArray[iteration].name = info.users[x].first_name + ' ' + info.users[x].last_name
        userArray[iteration].billableHours = 0;
        userArray[iteration].totalHours = 0;
        iteration++;
      };

      if(pageNumber < info.total_pages) {
        pageNumber++;
        options.url = 'https://api.harvestapp.com/v2/users?page=' + pageNumber + '&per_page=100';
        request(options, callback1);
      } else {
        pageNumber = 1;
      }

    };

  };

  request(options, callback1);

  function callback2(error, response, body) {
    if (!error && response.statusCode == 200) {

      info = JSON.parse(body);
      pages = info.total_pages;

      for(y = 0; y < info.time_entries.length; y++){

        for(x = 0; x < userArray.length; x++) {

          if(info.time_entries[y].user.id == userArray[x].id) {
            userArray[x].totalHours += info.time_entries[y].hours;

            if(info.time_entries[y].billable) {
              userArray[x].billableHours += info.time_entries[y].hours;
            }
            break;

          }

    }

  }
  if(pageNumber < info.total_pages) {
    pageNumber++;
    options2.url = 'https://api.harvestapp.com/v2/time_entries?from=' + new moment().day(-1).format('YYYY-MM-DD') + '&to=' + new moment().day(5).format('YYYY-MM-DD') + '&page=' + pageNumber + '&per_page=100';
    request(options2, callback2);
  } else {
    removeUsers(userArray, 'totalHours', 0);
  }

}

 }

 request(options2, callback2);

 function removeUsers(array, attribute, value) {
 var i = array.length;

while(i--) {
  array[i].utilization = Math.ceil(100*(array[i].billableHours/40));

  if(array[i] && array[i].hasOwnProperty(attribute) && (arguments.length > 2 && array[i][attribute] === value)){
    array.splice(i,1);
  }

}
console.log(array);
}

I'm sorry about the spacing down here, for some reason it wouldn't recognize it as code. If anyone has any optimization ideas those would be much appreciated.

Appending to an array

The global iteration variable is used as the index to set objects in the global userArray. This implementation practically appends items to the array:

userArray[iteration] = new Object(),
userArray[iteration].id = info.users[x].id,
userArray[iteration].name = info.users[x].first_name + ' ' + info.users[x].last_name
userArray[iteration].billableHours = 0;
userArray[iteration].totalHours = 0;
iteration++;

A simpler alternative is to use the push method of arrays instead of the iteration variable.
Also, instead of setting the fields one by one,
it will be shorter to use an object literal:

userArray.push({
  id: info.users[x].id,
  name: info.users[x].first_name + ' ' + info.users[x].last_name,
  billableHours: 0,
  totalHours: 0
});

Data structures

In callback2,
for each time entry,
you loop over userArray until you find a user with the matching id.
You could avoid this nested loop by using a mapping of user ids to users.
If you don't need to preserve the ordering of users,
then you can drop userArray in favor of the map.
If you need to preserve the ordering, you can keep both.

Punctuation

The punctuation here is messy and potentially buggy:

userArray[iteration] = new Object(),
userArray[iteration].id = info.users[x].id,
userArray[iteration].name = info.users[x].first_name + ' ' + info.users[x].last_name
userArray[iteration].billableHours = 0;
userArray[iteration].totalHours = 0;
iteration++;

Each line should have ended with a ;, instead of , or nothing.

Don't repeat yourself

The options and options2 variables are almost identical.
Try to avoid copy-pasting,
use helper methods or benefit from JavaScript's prototypal inheritance instead.

Naming

Instead of numbering in names such as options1, callback1, and others,
it would be better to use more meaningful names.


Converting hex character to decimal Is it possible to make this code more efficient & elegant?

int HexToDecimal(char ch)
{
    ch = tolower(ch);
    if ('a' <= ch && ch <= 'f')
    {
        return ch - 'a' + 10;
    }
    else if ('0' <= ch && ch <= '9')
    {
        return ch - '0';
    }
    printf("Error\n");
    return 0;
}

Update: Printing error could be ignored. By returning 0 I only mean to show that the input char is not a hex digit.


NodeJS - Auto Module Loader I took some time to try and solve a problem. I am building a basic Node + Express API. In the app I have created a models folder and will be adding new models as I continue development.

I attempted to write a small autoloader for a directory to automatically load and export my models. This code snippet should work for any modules not just my models folders.

See the following code:

loader.js

const fs = require('fs');
// Autoload modules.
module.exports = fs.readdirSync(__dirname)
                    // Ignore current file (loader.js)
                    .filter(f => !__filename.includes(f))
                    // Require all files in directory
                    .map(f => require( `./${f}` ))
                    // Add to final module.exports object using class name
                    .reduce((prev, curr) => ({...prev, [curr.name]: curr}), {});

My folder structure for the models:

How I import the models into my Routes file:

author.route.js

const { AppResponse, Author } = require('../models/loader');

The question:

Is this a good idea to do something like this? My reasoning is that it would minimize changes when I commit and not require any modifications to my code should I decide to add new functionality to my routes and models folders.

What caveats are there to doing this and would there be a better approach?

Thanks for taking the time to look at this!

tl;dr

The major reason for not doing this is that dynamically requiring modules breaks static analysis.

In a pure Node.js project, this does not have major repercussions aside from breaking IDE's autocompletion. Everything else will still work as expected.

diving deeper

Should this pattern be used in a webpack project for instance (targetting server or client), it would fail, as bundlers need to statically compute the dependency tree at compile-time.

The require() function  (CommonJS) was implemented in Node because, at the time, the core Javascript language didn't have the concept of modules. However this has changed, as ES modules are now available in all evergreen browsers, and in Node 13.x without a feature flag.

The new keywords import and export do not support variable interpolation for the purpose of static analysis, and are even required to be declared on top of the module. In practice, this makes the patterns if (dev) { require('debug') } or  require(name) unusable, once adapted to ES modules.

recommendation

In my opinion, this is a case where anything “clever” you try to make will just make the code harder to read and maintain. I've been down that path too, and I can tell you that the long-term costs are not worth the 2 seconds it will take you to add manual exports to your files. In my experience, reading code is 10 times more costly than writing it.

You also don't even have to create index.js loaders; you can require the files directly. Ryan Dahl, creator of Node, even said at JSConf 2018 that he considered the addition of index.js a mistake because “it needlessly complicated the module loading system.”

Good luck, and be kind to future-you.


WebGL (Three.js) simple application I have done the source code for my lesson, which I shall publish later on my website:

<script src="three.min.js"></script>
<script>

var renderer, scene, camera, geometry, material, mesh, light, axisRotation;

function createScene() {
    renderer = new THREE.WebGLRenderer();
    renderer.setSize( window.innerWidth, window.innerHeight );
    document.body.appendChild( renderer.domElement );

    scene = new THREE.Scene();

    camera = new THREE.PerspectiveCamera( 45, window.innerWidth / window.innerHeight, 1, 10000 );
    camera.position.set( 3, 3, 3 );
    camera.lookAt( scene.position );
    scene.add( camera );

    geometry = new THREE.CubeGeometry( 1, 1, 1 );

    material = new THREE.MeshLambertMaterial( { color: 0xff0000 } );

    mesh = new THREE.Mesh( geometry, material );
    scene.add( mesh );

    light = new THREE.PointLight( 0xffff00 );
    light.position.set( 10, 10, 10 );
    scene.add( light );
}

function rotateCube( axis ) {
    switch ( axis ) {
        case 'x':
            mesh.rotation.x += 0.02;
            break;
        case 'y':
            mesh.rotation.y += 0.02;
            break;
        case 'z':
            mesh.rotation.z += 0.02;
            break;
    }
}

function toggleObjectWireframe() {
    material.wireframe = !material.wireframe;
}

function renderScene() {
    renderer.render( scene, camera );
}

function animateScene() {
    requestAnimationFrame( animateScene );
    renderScene();
    rotateCube( axisRotation );
}

function getChar( event ) {
    var inputChar = null;

    if ( event.which === null ) {
        if ( event.keyCode < 32 ) inputChar = null;
        else inputChar = String.fromCharCode( event.keyCode );
    }

    if ( event.which != 0 && event.charCode != 0 ) {
        if ( event.which < 32 ) inputChar = null;
        else inputChar = String.fromCharCode( event.which );
    }

    if ( ~['x', 'y', 'z'].indexOf( inputChar ) ) {
        axisRotation = inputChar;
    }
    else if ( inputChar == 'w' ) toggleObjectWireframe();
    else inputChar = null;
}

function initWebGLApp() {
    axisRotation = 'x';
    createScene();
    animateScene();
}

function setCssStyle() {
    document.body.style.width = "100%";
    document.body.style.height = "100%";
    document.body.style.margin = 0;
    document.body.style.padding = 0;
    document.querySelector('canvas').style.display = 'block';
}

window.onload = function() {
    initWebGLApp();
    setCssStyle();
    document.onkeypress = getChar;
}

</script>

You can see the working results on JSFiddle.

This code is good for tutorial use.

Some suggestion:

You are switching over 'x','y' and 'z' :

function rotateCube( axis ) {
    switch ( axis ) {
        case 'x':
            mesh.rotation.x += 0.02;
            break;
        case 'y':
            mesh.rotation.y += 0.02;
            break;
        case 'z':
            mesh.rotation.z += 0.02;
            break;
    }
}

You could simply use axis straight.

function rotateCube( axis ) {
  mesh.rotation[axis] += 0.02;
}

You could explain some of your magical constants, like 10000, it is those values that always throw me off when I experiment with this 3d library.

You should probably extract your rgb codes into well named variables like cubeColor, pointLightColor etc.

This line could use some comment: requestAnimationFrame( animateScene );

One liner functions that are called only once are wrong, I'd much rather you keep those lines in-line with a comment above them as to what is going on.

function renderScene() {
    renderer.render( scene, camera );
}

function animateScene() {
    requestAnimationFrame( animateScene );
    renderScene();
    rotateCube( axisRotation );
}

is less readable than

function animateScene() {
    requestAnimationFrame( animateScene );
    //Render scene
    renderer.render( scene, camera );
    rotateCube( axisRotation );
}

You could merge the treatment of keyCode and which.

var inputChar = null;

if ( event.which === null ) {
    if ( event.keyCode < 32 ) inputChar = null;
    else inputChar = String.fromCharCode( event.keyCode );
}

if ( event.which != 0 && event.charCode != 0 ) {
    if ( event.which < 32 )
      inputChar = null;
    else
      inputChar = String.fromCharCode( event.which );
}

you can change the above to

var keyCode   = event.keyCode || event.which,
    inputChar = keyCode < 32 ? null : String.fromCharCode( keyCode );

Though I will admit that might be too Golfic for a tutorial, you could turn the ternary into a regular if statement to keep things easy.

Furtermore, from a style perspective, your if's look bad

    if ( event.keyCode < 32 ) inputChar = null;
    else inputChar = String.fromCharCode( event.keyCode );

should be

    if ( event.keyCode < 32 )
      inputChar = null;
    else
      inputChar = String.fromCharCode( event.keyCode );

and

if ( ~['x', 'y', 'z'].indexOf( inputChar ) ) {
    axisRotation = inputChar;
}
else if ( inputChar == 'w' ) toggleObjectWireframe();
else inputChar = null;

should be either

if ( ~['x', 'y', 'z'].indexOf( inputChar ) ) {
    axisRotation = inputChar;
}
else if {
    ( inputChar == 'w' ) toggleObjectWireframe();
}
else {
    inputChar = null;
}

or

if ( ~['x', 'y', 'z'].indexOf( inputChar ) )
    axisRotation = inputChar;
else if ( inputChar == 'w' )
  toggleObjectWireframe();
else
  inputChar = null;

I would advocate the second approach.

Furthermore, the following 2 lines seem pointless, I took them out in the fiddle, and it works just fine;

document.body.style.width = "100%";
document.body.style.height = "100%";

Finally, this :

document.onkeypress = getChar;

is old skool, and should be avoided in general, I understand it is easy for a tutorial, but you should at least put a line of comment that this is not ideal.

All in all, I like your code, even though this review got a bit lengthy.


Call center dispatch using object-oriented Python I am looking for tips on improving my implementation of an OOP design problem in Python3

Here is the problem (paraphrasing to save space):

There is a call center with three levels of employees - Respondent, Manager and Director.
A call that goes to the center propagates as Respondent -> Manager -> Director depending on whether or not

a) The respondent is free and can handle the call

b) The manager is free and can handle the call

The call center needs to have a dispatchCall() method that assigns the call to the first available employee

Here is the code

#! /usr/bin/env python3
from abc import ABCMeta, abstractmethod
from random import random

class CallCenter():
    """
    A call center class that will be inherited from by the director

    Attributes:
        name: Name of the employee
        number: Mobile phone number of the employee
        level: Define level of the employee - respondent, manager or director
    """
    __metaclass__ = ABCMeta

    def __init__(self, name, number, level):
        self.name = name
        self.number = number
        self.level = level

    def canEmployeeHandle(self):
        canHandle = random()
        if canHandle > 0.5:
            return True
        else:
            return False

    def isEmployeeBusy(self):
        busy = True if random() <= 0.5 else False
        return busy

    @staticmethod
    def dispatchCall():
        return respondent.answerCall()

    @abstractmethod
    def answerCall(self):
        """Print a string indicating that this employee is currently
        answering the call or propogate up the chain"""
        pass

class Director(CallCenter):
    """
    A class definition for the director who will be the third in chain
    to answer the call
    """
    def answerCall(self):
        print("The director is now on call with you")

class Manager(Director):
    """
    A class definition for the manager who will be the second in chain to
    answer the call
    """
    def answerCall(self):
        if not self.isEmployeeBusy() and self.canEmployeeHandle():
            print("The manager is answering your call")
        else:
            super(Manager, self).answerCall()

class Respondent(Manager):
    canHandle = random() #class variable to check if respondent can handle call
    """
    A class definition for the respondent who will be the first in chain
    to answer the call
    """
    def answerCall(self):
        if not self.isEmployeeBusy() and self.canEmployeeHandle():
            print("The respondent is answering your call")
        else:
            super(Respondent, self).answerCall()

if __name__ == "__main__":
    director = Director("Alex", "+17654736791", "director")
    manager = Manager("Unai", "+331577285782", "manager")
    respondent = Respondent("Mark", "+16574872817", "respondent")

    CallCenter.dispatchCall()

Segragated the responsilibites of call center into call center and employee
from abc import ABCMeta, abstractmethod
from random import random

class CallCenter(metaclass=ABCMeta):
    """
    A call center class that will be inherited from by the director

    Attributes:
        name: Name of the employee
        number: Mobile phone number of the employee
        level: Define level of the employee - respondent, manager or director
    """

    @classmethod
    def dispatchCall(self, cls):
        return cls.answerCall()

class Employee(metaclass=ABCMeta):

    def __init__(self, name, number, level):
        self.name = name
        self.number = number
        self.level = level

    def canEmployeeHandle(self):
        canHandle = random()
        if canHandle > 0.5:
            return True
        else:
            return False

    def isEmployeeBusy(self):
        busy = True if random() <= 0.5 else False
        return busy

    @abstractmethod
    def answerCall(self):
        """Print a string indicating that this employee is currently
        answering the call or propogate up the chain"""
        pass

class Director(Employee):
    """
    A class definition for the director who will be the third in chain
    to answer the call
    """

    def answerCall(self):
        print("The director is now on call with you")

class Manager(Director):
    """
    A class definition for the manager who will be the second in chain to
    answer the call
    """

    def answerCall(self):
        if not self.isEmployeeBusy() and self.canEmployeeHandle():
            print("The manager is answering your call")
        else:
            print("Manager is busy, escalating the call to the Director")
            super(Manager, self).answerCall()

class Respondent(Manager):
    canHandle = random()  # class variable to check if respondent can handle call
    """
    A class definition for the respondent who will be the first in chain
    to answer the call
    """

    def answerCall(self):
        if not self.isEmployeeBusy() and self.canEmployeeHandle():
            print("The respondent is answering your call")
        else:
            print("Employee is busy, escalating the call to the Manager")
            super(Respondent, self).answerCall()

if __name__ == "__main__":
    director = Director("Alex", "+17654736791", "director")
    manager = Manager("Unai", "+331577285782", "manager")
    respondent = Respondent("Mark", "+16574872817", "respondent")
    CallCenter.dispatchCall(respondent)
    enter code here


Very basic singly linked list Here is implementation for singly linkedlist. Any suggestions are most welcome

package linkedlist;

import java.util.Scanner;

public class linkedlist {
    private int size=0;
    private Node newNode,oldNode,firstNode,displayNode;

    public void createNode(String value) {
        if(size==0) {
            newNode=new Node(value,null);
            firstNode=newNode;
            size++;
        }
        else {
            oldNode=newNode;
            newNode=new Node(value,null);
            oldNode.setNextNode(newNode);
            size++;

        }

    }

    public void DisplayLinklist() {
        displayNode=firstNode;
        while(true) {
            System.out.println(displayNode.getItem());
            displayNode=displayNode.next;
            if(displayNode.next==null) {
                System.out.println(displayNode.getItem());
                break;
            }

        }
    }

    private class Node{
        private String item;
        private Node next;

        Node(String item,Node next){
            this.item=item;
            this.next=next;
        }

        public String getItem() {
            return this.item;
        }

        public void setNextNode(Node address) {
            this.next=address;
        }
    }

    public static void main(String[] args) {
        linkedlist mylinklist=new linkedlist();
        Scanner in=new Scanner(System.in);
        System.out.println("Enter the string value you want to save or press 'n'");
        String input=in.next();
        while(!input.equalsIgnoreCase("n")) {
            mylinklist.createNode(input);
            System.out.println("Enter the string value you want to save or press 'n'");
            input=in.next();
        }

        mylinklist.DisplayLinklist();

    }

}

This is very basic version and first time I  am doing Linkedlist. I am not using any iteration, but just wanted to confirm am I on the right path?

size++ should be extracted from both branches and called only once
there is no need to cache instance variables where local method variables should be used instead: newNode,oldNode,displayNode
prefer while(currentNode != null) over while(true) and currentNode should start with firstNode and be chained inside the while-loop as currentNode = currentNode.next
you should also provide remove and iterator functionality
DisplayLinklist should not be inside this class and it should be camel cased
variable firstNode is usually called head
class linkedlist should be called LinkedList
get rid of unwanted blank lines


Counting duplicate tags in a string of many tags I have a photo list page where each photo has multiple tags (as spaced words). I'm trying to display most of the tags on the page after:

Removing duplicate tags
Sorting tags by having most occurring tags first

The code looks like this:

$tags1 = "red blue green yellow colors";
$tags2 = "blue green grass tree garden";
$tags3 = "pink blue colors paint sky"

addTags($tags1);
addTags($tags2);
addTags($tags3);
echo (processTags());

private $tagsArray;
public function addTags($param) {
    if($param == ""){ // no tags
        return;
    }
    $explode = explode(" ",$param);
    $this->tagsArray = array_merge($this->tagsArray, $explode);
}

private function processTags(){
    $processedTags = array_count_values($this->tagsArray);
    arsort($processedTags);
    $count = 0;
    foreach($processedTags as $tag => $value) {
        if(++$count>50) break; // don't print more than 50 tags
        $output .= "$tag";
    }
    return $output;
}

Each photo can have dozens of tags and there can be hundreds of tags total. Is there any way to improve the code to make it more efficient? Could it be done with less code? The above code works just fine, but I'm just curious to know if there is a better way to go about it.

I has been a long time since I used PHP, but what you want are multisets that are basically sets with a value -> number-of-occurrences mapping. I believe you can accomplish this in PHP via associative arrays as below:

$tags1 = "red blue green yellow colors blue red green yellow yellow";

function addTags($param) {

    // tagsArray should be a global variable
    // but put as local variable for illustration only
    $tagsArray = array();

    if($param == ""){ // no tags
        return;
    }
    $explode = explode(" ",$param);

    foreach ($explode as $value) {
       $val = 0;
       if (array_key_exists($value, $tagsArray)) {
          $val = $tagsArray[$value];
       }

       $tagsArray[$value] = 1 + $val;
    }

    // sort the array
    arsort($tagsArray);

    // for illustration purposes only
    foreach ($tagsArray as $key => $value) {
       echo $key.':'.$value.', ';
    }
}

addTags($tags1);


Calculating a base price with surcharge conditions The following code has a lot of conditionals. I am trying to write
it in a functional programming way.

val basePrice = {
  var b = 0.0

  if (runtime > 120)
    b += 1.5
  if ((day == Sat) || (day == Sun))
    b += 1.5
  if (!isParquet)
    b += 2
  if (is3D)
    b += 3
  b
}

I think the following code would be a good approach, but maybe I am
complicating this too much.

val basePrice = {
  List((runtime > 120, 1.5),
       (day == Sat || day == Sun, 1.5),
       (!isParquet, 2.0),
       (is3D, 3.0)).foldLeft(0.0)((acum, cond) =>
     if (cond._1) acum + cond._2 else acum)
}

How would you write the first snippet of code using functional
programming?

Each condition is a function. It might be that you could write it more concisely, but I think the code is clearer if you do this:

def priceFunction(cond: => Boolean)(mod: Double => Double) = (_: Double) match {
    case x if cond => mod(x)
    case y => y
}

val modRuntime = priceFunction(runtime > 120)(_ + 1.5)
val modWeekend = priceFunction(day == Sat || day == Sun)(_ + 1.5)
val modParquet = priceFunction(!isParquet)(_ + 2.0)
val mod3d = priceFunction(is3D)(_ + 3.0)
val modifiers = List(
    modRuntime,
    modWeekend,
    modParquet,
    mod3d
)
val modifierFunction = modifiers reduceLeft (_ andThen _)

val basePrice = modifierFunction(0.0)

The name of the identifiers here suck, and I could have written val modifiers = modRuntime andThen modWeekend andThen modParquet andThen mod3d without trouble. I choose putting them in a List because it shows how well it can scale.

One could also make PartialFunction and chain them with orElse, for the cases where you want only the first condition.

You see this kind of thing used in web frameworks, such as BlueEyes, Lift or Unfiltered, for example.


Matching a randomly made colour using sliders Below is a code which creates a random colour and shows it to the user in a 64 x 64 square. The user must then use the 3 sliders (R, G, B) to match the colour as closely as possible

from tkinter import *
import random

main = Tk()

photo = PhotoImage(width = 64, height = 64)
photo_2 = PhotoImage(width = 64, height = 64)

r_answer = random.randrange(256)
g_answer = random.randrange(256)
b_answer = random.randrange(256)

def pixel(image, pos, colour):
    r,g,b = colour
    x,y = pos
    for i in range(32):
        for j in range(32):
            image.put("#%02x%02x%02x" % (r,g,b),(y+i,x+j))

def show_values(self):

    r = w1.get()
    g = w2.get()
    b = w3.get()

    pixel(photo, (0,0), (r, g, b))

    label = Label(main, image = photo)
    label.grid(row = 0, column = 1)

w1 = Scale(main, from_=0, to=255, command=show_values)
w1.grid(row = 0, column = 0)
w2 = Scale(main, from_=0, to=255, command=show_values)
w2.grid(row = 1, column = 0)
w3 = Scale(main, from_=0, to=255, command=show_values)
w3.grid(row = 2, column = 0)

pixel(photo_2, (0, 0), (r_answer, g_answer, b_answer))

label_2 = Label(main, image = photo_2)
label_2.grid(row = 1, column = 1)

main.mainloop()

Here are the improvements to your program that I would suggest:

Performance issue

Whenever the user of your program presses one of the colors sliders, show_values() function is invoked and it create a new tk.Label() instance in addition to the formerly existing ones. This means your program consumes unnecessary memory that can be harmful when the user scrolls down and up, several times, the color sliders. This is clearly a performance issue that can incapacitate your program.

Solution to the performance issue

To resolve the performance issue, you can create a tk.Frame() instance to be the parent widget of the tk.Label() instances you create. This will allow you to clear the tk.Frame() parent widget before a new tk.Label() widget is created.

In my solution below, the color surface which results from the user's actions is wrapped by a tk.Frame() instance:

def create_estimated_color_area(self):
    self.frame = tk.Frame(self.master)
    self.frame.grid(row=0, column=1)

The callback function inherent to the tk.Scale() widgets clears the self.frame widget above:

def estimate_color(self, event):
    # Clear the frame before drawing a new label on it
    self.clear_widget(self.frame)
    # ...
    self.calculated_color_image = tk.PhotoImage(width=64, height=64)
    self.colorize_area(self.calculated_color_image, (r, g, b))
    self.calculated_color_area = tk.Label(self.frame, image=self.calculated_color_image)
    self.calculated_color_area.pack()

The clear_widget() function loops over the self.frame widget to look for its children and removes them away totally:

def clear_widget(self, widget):
    for child in widget.winfo_children():
        child.destroy()

Scalability issues

Your program, as designed, is not scalable. This is because you designed it using a mixture of sequential and functional approaches. But you are dealing with a GUI, and user graphical interfaces were the factor that soared the thrive of object-oriented design. Whether there are some philosophical dirty discussions about it, you should adopt an OOP approach to design your GUI: at least this is valid when it comes to Python and Tkinter. This will remedy to the scalability problem your program is suffering from.

Given this fact, my solution below consists in redesigning your program in an OOP approach. Of course, I end up by more code than you provided by now. But the difference is that my solution is open for adding any other future functionalities in the future, if you want to improve and scale your program. It is also a more disciplined and cleaner code. To understand my design below, you can refer to Tkinter best practices article.

Confusing names

I do not want to be rude, but honestly most of the variable and function names you used are confusing. This is the case, for example, of the pixel() function which does not redefine the notion of a pixel or anything like that. You rather used it to colorize a square area. In addition to this, as in all other programming languages, your function name should be a verb, not a noun. Why? Because a function does something. A function is a doer.

I could write a long text about the naming conventions regarding almost all your variables and functions names: no one, for instance, could find it easy to guess and understand without an additional effort, what are these variables supposed to do: w1, w2, and w3. I renamed everything accordingly in my solution (not to perfection, but to better, and surely you can choose better names than I provided below)

pixel() with one parameter less

The pos parameter in pixel() function is useless because, you always called it with the same starting position (0, 0). So simply get rid of it. Advantage? Less parameters in a function leads to simpler unit testing and cleaner code since parameters belong to a level of abstraction different than that of functions.

Solution

Taking in consideration the former remarks, but also other ones I did not mention such as the right way to import Tkinter, below are the new clothes to your program.

Note that the execution of the below program outputs the same result as yours:

As I said, I have more code below, but I provide you clan code which is scalable: which thing means, as your program becomes larger (in the future) you will write less code and in an easier way. Please, feel free if there is something you do not understand:

import tkinter as tk
import random

class ColorFinder(tk.Frame):

   def __init__(self, master):
       self.master = master
       tk.Frame.__init__(self, self.master)
       self.configure_gui()
       self.create_widgets()

   def configure_gui(self):
       self.master.title('Coulor Finder')

   def create_widgets(self):
       self.create_color_sliders()
       self.create_color_areas()

   def create_color_sliders(self):
       self.red_color_slider = tk.Scale(self.master, from_=0, to=255)
       self.green_color_slider = tk.Scale(self.master, from_=0, to=255)
       self.blue_color_slider = tk.Scale(self.master, from_=0, to=255)
       self.position_color_sliders()
       self.configure_color_sliders()

   def position_color_sliders(self):
       self.red_color_slider.grid(row=0, column=0)
       self.green_color_slider.grid(row=1, column=0)
       self.blue_color_slider.grid(row=2, column=0)

   def configure_color_sliders(self):
       self.red_color_slider.configure(command=self.estimate_color)
       self.green_color_slider.configure(command=self.estimate_color)
       self.blue_color_slider.configure(command=self.estimate_color)

   def create_color_areas(self):
       self.create_target_color_area()
       self.create_estimated_color_area()

   def create_target_color_area(self):
       r = random.randrange(256)
       g = random.randrange(256)
       b = random.randrange(256)
       self.target_color_image = tk.PhotoImage(width=64, height=64)
       self.colorize_area(self.target_color_image, (r, g, b))
       self.target_color_area = tk.Label(self.master, image=self.target_color_image)
       self.target_color_area.grid(row=1, column=1)

   def create_estimated_color_area(self):
       self.frame = tk.Frame(self.master)
       self.frame.grid(row=0, column=1)

   def estimate_color(self, event):
       self.clear_widget(self.frame)
       r = self.red_color_slider.get()
       g = self.green_color_slider.get()
       b = self.blue_color_slider.get()
       self.calculated_color_image = tk.PhotoImage(width=64, height=64)
       self.colorize_area(self.calculated_color_image, (r, g, b))
       self.calculated_color_area = tk.Label(self.frame, image=self.calculated_color_image)
       self.calculated_color_area.pack()

   def colorize_area(self, image, color):
       x = 0
       y = 0
       r, g, b = color
       for i in range(32):
           for j in range(32):
               image.put("#%02x%02x%02x"%(r , g, b), (y+i, x+j))

   def clear_widget(self, widget):
       for child in widget.winfo_children():
           child.destroy()

if __name__ == '__main__':
   root = tk.Tk()
   ColorFinder(root)
   root.mainloop()